00:02:09 <11O​dds> Thanks for checking. Pity 🙂 00:02:59 <04d​racoomega> I think the part you fixed is also really unlikely to happen, since it requires trying to place a monster on top of another monster (or the player), which I am pretty sure should already have been ruled out upstream ~all of the time for normal generation 00:03:19 <04d​racoomega> (I mean, I can't ensure it's always correct, but it normally has at least checked, when trying to determine where a monster can be placed) 00:04:11 <11O​dds> Yeah, I couldn't work out where it happens, I only repro'd the message by artificially triggering that bit of code on legal monsters 00:04:58 <04d​racoomega> This really is mysterious 00:06:04 <04d​racoomega> Oh, wait 00:06:12 <04d​racoomega> We have floors and seeds 00:06:23 <04d​racoomega> I may be able to specifically reproduce this 00:06:36 <04d​racoomega> And get more detailed information on what exactly is happening 00:07:23 <04d​racoomega> I'm going to try 00:08:21 <11O​dds> Oh right yes now it's build time that's much better! 00:08:27 <04d​racoomega> Yeah, vastly 00:08:51 <11O​dds> Never trust a bug report 🙂 00:10:04 <04d​racoomega> Okay, yes, easily reproducible from the seed. Going to pepper in a few debug statements and see if I can clean anything useful here 00:14:14 <11O​dds> I wonder if we should stop showing shadows when we see a monster again 00:14:15 <11O​dds> https://cdn.discordapp.com/attachments/747522859361894521/1529386070708453448/image.png?ex=6a61bf45&is=6a606dc5&hm=a4f40e2215c9d06b1e5849bc92bfb6238152f5d0bb8e6ec93b97cc0c2a754859& 00:19:46 <04d​racoomega> How in the world did you get that many trails? 00:19:52 <11O​dds> Kobold 00:19:56 <04d​racoomega> Oh 00:20:05 <11O​dds> With a few permanent allies a kobold does this ~every turn 00:20:13 <04d​racoomega> (There is in fact, code to remove the previous memory of a monster if you saw it just enter LoS again that way, I believe) 00:20:27 <11O​dds> Oh... 00:20:35 <04d​racoomega> I am guessing it's getting too far away? 00:21:01 <11O​dds> I think it's leaving LoS as I move, and re-entering as it moves, every turn 00:21:10 <04d​racoomega> Like, I believe if it enters LoS, it deletes a memory at the tile it just was 00:21:23 <04d​racoomega> Or that it's supposed to do something like that 00:21:32 <11O​dds> Ah I see 00:22:01 <11O​dds> I'll have a look. Not sure there's any valid use for memories of a monster we can now see? And we'll get a fresh memory if we lose sight again 00:22:02 <04d​racoomega> Look at monster::check_redraw() 00:28:14 <04d​racoomega> I am definitely learning things here... 00:30:15 <04d​racoomega> (Still too fuzzy to give a good explanation, but at least it isn't aimless) 00:30:43 <04d​racoomega> It is looking like it somehow involves shadow wraiths, which is a neat trick since they don't have items 00:30:53 <11O​dds> I was wondering about them and their lurking code 00:31:03 <11O​dds> But wrote them off because they don't have items 🙂 00:31:35 <04d​racoomega> Yes, but at some point in time, a deep elf zephymancer and a dancing hand axe end up in the same mindex slot and then get reset without dying fully, and it's their equipment that is unlinked 00:31:43 <04d​racoomega> I am not sure how any of this happens yet 00:32:13 <11O​dds> Nice. Shout if you get stuck and want another pair of eyes 00:32:13 <04d​racoomega> (There's really quite a confusingly large amount of monster resetting happening during this level generation) 00:32:32 <04d​racoomega> So I am trying to put in a few more markers of 'What part of generation are we even at' 00:33:05 <04d​racoomega> (I don't have the full picture yet, but at the moment it feels like something where that's only a matter of time) 00:52:37 <04d​racoomega> Okay, I believe I have solved it 00:54:27 <04d​racoomega> So, a level is generated with some shadow wraiths. Those wraith automatically lurk as part of generation, killing them via monster_die(). Then the level they were on gets vetoed by the builder, and a different one is generated. Eventually flush_monster_reset() is called after generation, and whatever monsters at the mindex the shadow wraiths used to be on the previous version of the level get reset (possibly while still holding 00:54:27 items) 00:55:44 <04d​racoomega> So, the immediate problem here can probably also be fixed by having the monster_die() call for lurking reset immediately. Though it's a touch awkward since that can happen outside generation also (where we're not really 'supposed' to be doing it that way now, I think?) 00:56:28 <04d​racoomega> And I wonder in a general sense whether flush_monster_reset() could be unsafe in some other scenarios we haven't thought of 00:57:25 <04d​racoomega> Holding references here to monsters from another floor, effectively 00:59:17 <04d​racoomega> Maybe it should store not just the mindex of the monster to be reset, but its mid as well? And then before resetting, verifies that the monster pointed at is in fact the monster it was told to reset? 00:59:46 <04d​racoomega> This might remove the need for cancel_pending_monster_reset() as well? 01:00:39 <04d​racoomega> Since it should be far harder to ever reset the 'wrong' monster 01:00:48 <04d​racoomega> And it will just automatically skip them if they're not around 01:00:51 <11O​dds> Yeah this seems very sensible 01:03:30 <11O​dds> And yes I think this makes cancel_pending_monster_reset obsolete, it's exactly for clearing up this inconsistency 01:04:00 <04d​racoomega> Much nicer to have it 'guaranteed' automatically instead of having to manually flag any cases it might end up inconsistent 01:04:57 <11O​dds> Could we fail to reset a monster at all this way I wonder, and lose slots? Probably not, because it only triggers if the slot is reset/reused 01:06:14 <11O​dds> This is effectively "something else reset this slot while reset was pending", right? 01:06:21 <04d​racoomega> Yes 01:06:35 <04d​racoomega> (In this case, wiping the whole floor out to generate a new one) 01:06:44 <04d​racoomega> But there's very plausibly other means this could happen 01:06:53 <11O​dds> Yeah, seems safe and good to add this check 01:22:20 <07w​izardike> Would this have any problems with transiting monsters? Don't we kill those and then place them again with the same mid? Maybe we don't add these to the pending reset queue though 01:23:36 <04d​racoomega> They're manually reset, and thus don't interact with this, I think? 01:26:57 <11O​dds> Ah, the transiting case was when we needed cancel_pending_monster_reset 01:27:13 <11O​dds> So maybe we do still need it 01:30:19 <04d​racoomega> I'm not sure I see why? Is there any circumstance where a monster would have been killed and still be alive in the same mindex that it was when it died? 01:32:25 <07w​izardike> Oh looking closer at the code, a monster can die while on the transit list due to dactions and we don't want to add it the the pending reset list in this case. But its easier just to remove it from the list after running the dactions than make sure all the dactions don't add it to the list 01:33:09 <04d​racoomega> Well, I mean, if a transiting monster is killed, it by definition wasn't in env.mons in the first place, and should never get added to said list 01:33:38 <11O​dds> Right, but we could kill it in all sorts of ways, and it's hard to make them all reset it 01:33:57 <11O​dds> So we remove it immediately (this was https://github.com/crawl/crawl/commit/80f85d7f2857) 01:35:30 <04d​racoomega> Frankly, directly killing transiting monsters via daction was already kind of fraught 01:35:41 <04d​racoomega> %git ca4ab9d 01:35:42 <04C​erebot> DracoOmega * 0.35-a0-50-gca4ab9d3b7: Fix undefined behavior when killing a transiting bound soul via daction (5 months ago, 2 files, 10+ 3-) https://github.com/crawl/crawl/commit/ca4ab9d3b7e1 01:36:36 <11O​dds> (Actually wouldn't be that hard, it's like 4 kills in one function that are probably all fine to reset even in the non-transit case) 01:37:49 <04d​racoomega> Like, just in general, there's a variety of ways that using monster_die() on a monster that isn't actually here is scary 01:38:21 <04d​racoomega> Since the code very much makes certain assumptions that the monster actually exists on the level 01:38:44 <07w​izardike> Arguably we might actually need it on the pending reset list to if its possible for its death to queue a final effect that references it, but obviously this reference to it in the pending reset queue can't use mindex 01:40:17 <11O​dds> Final effects on monsters that aren't on this level sound especially scary 01:40:19 <04d​racoomega> Yes 01:40:52 <04d​racoomega> Like, it feels like bad things would only not happen by coincidence, frankly 01:44:58 <04d​racoomega> Really, the more I look at this again, the more uncomfortable I am with multiple things we do here. Like, this comment: C++ // Transiting monsters exist outside the normal monster list (env.mons or // env.mons for short). Be careful not to write them into the monster grid, by, // for example, calling monster::move_to() on them. It just kind of feels like it would be good to have a better way of doing this that didn't 01:44:59 have apparently a large number of ways to create undefined behavior if you're not careful 01:46:14 <04d​racoomega> Like if we could queue applicable dactions up on an individual monster and only perform them when the monster actually gets placed 01:49:10 <07w​izardike> It might actually not be that bad to make dactions killing monsters just not add them to the pending reset list. We actually already to be very careful how we kill transiting monsters as e.g. one dropping an item can crash 01:52:15 <07w​izardike> Although I'm not entirely sure way we aren't placing the monsters before running dactions on them 01:53:40 <04d​racoomega> Well, the current ordering here is that when something first happens to cause a daction, it is applied to all monsters still in the transit list (who may not attempt to place for a very long time - ie: something that got banished won't attempt to place until you go to the Abyss) 01:54:57 <04d​racoomega> And yes, as I said, there's a bunch of ways this would be nicer/safer if they only applied to monsters who actually existed in a real floor instead of those currently residing in limbo, with all the caveats thereof 01:55:26 New branch created: pull/5345 (2 commits) 13https://github.com/crawl/crawl/pull/5345 01:55:27 03Eivin Hatvik02 07https://github.com/crawl/crawl/pull/5345 * 0.35-a0-743-g118b37bb9e: feat: show owned consumables in shops 10(10 hours ago, 4 files, 94+ 1-) 13https://github.com/crawl/crawl/commit/118b37bb9eee 01:55:27 03Eivin Hatvik02 {GitHub} 07https://github.com/crawl/crawl/pull/5345 * 0.35-a0-746-gf1183489a6: Show owned potion and scroll counts in shops (#1) 10(4 minutes ago, 0 files, 0+ 0-) 13https://github.com/crawl/crawl/commit/f1183489a69b 01:58:20 <07w​izardike> Right I guess that makes sense. The messaging would definitely be better for things like pikel minions disappearing 01:58:26 <11O​dds> Oooh, interesting PR. I would enjoy that count. 02:43:19 Ah ah ah ah! Six lovely potions! 02:43:31 03WizardIke02 07* 0.35-a0-745-ga609f807e6: Fix placing a vault in wizmode making mimics real 10(24 minutes ago, 10 files, 29+ 12-) 13https://github.com/crawl/crawl/commit/a609f807e6be 03:33:36 Experimental (bcrawl) branch on underhound.eu updated to: 0.23-a0-5261-gd9800d219b 04:02:52 <12g​e0ff> It's a very interesting PR indeed. There are tests (!), the code is pretty minimal (except for an unnecessary "+ struct item_def;" line in shopping.h), but it's a commit from master instead of a dedicated branch. 04:12:46 <11O​dds> Oh, I wasn't thinking this was a 🤖 at all. Not wordy enough! 04:13:15 <12g​e0ff> 🤖s are improving! 04:13:42 <12g​e0ff> (also, the PR is useful) 04:15:51 <11O​dds> Yeah. IMO it's best to judge it on the merits and not get too hung up on how robotic it may or may not be 04:17:25 <11O​dds> (My own personal position is that there are many people using 🤖s and also retaining sufficient authorship of their code that I'm happy to get PRs from them. As well as obviously many people producing slop I never want to see) 04:18:32 <12g​e0ff> My only complaint is that it feels a bit unnecessary to add tests for a small UI change 04:20:35 <12g​e0ff> Tests are useful when they break, and these tests are more like a proof that the feature actually works 04:21:10 <11O​dds> TBH I wish we had waaaay more tests such that these fit in (except maybe the first one, which seems to exercise an impossible inventory) 04:22:09 <12g​e0ff> a TDD programmer detected! :kobold: 04:22:33 <11O​dds> In some past lives 🙂 04:24:22 <12g​e0ff> imagining a daily scrum meeting for DCSS devs, where everyone tells what they are gonna do this week 04:25:52 <11O​dds> (Yeah I'm not at all confident how useful more tests would actually be in crawlcode, because it's not very much like any codebase I've done much in before. Could well be we already have the balance right) 04:40:57 <12g​e0ff> There are some types of bugs that appear again and again, like something about targeters and usefulness of spells, which often break when the spells themselves are reworked. A whole bunch of recurring bugs was about invisibility or dead monsters, but there should be fewer of them now, 🤞. Maybe there are other reappearing bugs, and having tests for them would be useful. 04:44:22 <11O​dds> (I also think there's significant value in tests that are "a proof that the feature actually works", as you wrote above. We have a number of features that never worked as intended) 04:45:23 <11O​dds> But I do have the feelign that most of our regressions are complex interaction effects that are hard to test for without loads of end-to-end level tests that would be hard to set up 04:51:42 <12g​e0ff> that's what the 🦅-eyed players are for! 04:53:44 <11O​dds> Yeah, an actively used trunk where some instability is expected is a wonderful testing asset 06:24:19 03CrawlOdds02 07* 0.35-a0-746-g9ba5819276: Fix monster unlinked item messages from doubly-reset monsters 10(2 hours ago, 1 file, 15+ 6-) 13https://github.com/crawl/crawl/commit/9ba5819276d5 06:47:52 <09g​ammafunk> is the account name CrawlOdds? 06:48:21 <11O​dds> No it's just Ods 06:49:09 <09g​ammafunk> I see carefulodds registered as well fwiw 06:49:53 <09g​ammafunk> so you may have your saved password associated with that one (and it has no on-file email, which you should set!) 06:49:58 <09g​ammafunk> I'll send a reset link for Odds 06:50:20 <09g​ammafunk> oh and ditto for TallOdds 06:51:00 <11O​dds> Yeah, those accounts probably have a much-reused password, whereas Odds set something a bit more secure since it has some privileges 06:51:11 <11O​dds> But then forgot it 😛 06:52:00 <09g​ammafunk> yeah, I do use my browser password manager most of the time, but I also use e.g. keepassx to save important account passwords (also has a handy password generator) 06:53:36 <09g​ammafunk> sent 06:57:26 <11O​dds> Thanks, done! 08:04:10 03CrawlOdds02 07* 0.35-a0-747-g32e4f8b34f: Make unlinked item logs less rude 10(76 minutes ago, 3 files, 24+ 3-) 13https://github.com/crawl/crawl/commit/32e4f8b34f3f 09:33:58 03Aliscans02 {CrawlOdds} 07* 0.35-a0-748-g443fa8328d: Fix Mad Mage Mauler align 10(5 days ago, 1 file, 2+ 2-) 13https://github.com/crawl/crawl/commit/443fa8328d63 13:59:16 03CrawlOdds02 07* 0.35-a0-749-gb29fdba68a: Stop infinite waits for ancestors healing 10(19 minutes ago, 1 file, 10+ 3-) 13https://github.com/crawl/crawl/commit/b29fdba68a0e 14:01:25 03CrawlOdds02 07* 0.35-a0-750-g4e1d064212: Revert "Stop infinite waits for ancestors healing" 10(76 seconds ago, 1 file, 3+ 10-) 13https://github.com/crawl/crawl/commit/4e1d064212be 14:18:36 04Build failed for 08master @ 4e1d0642 06https://github.com/crawl/crawl/actions/runs/29957502140 15:43:08 Unstable branch on underhound.eu updated to: 0.35-a0-750-g4e1d064212 (34) 23:35:21 Unstable branch on cbro.berotato.org updated to: 0.35-a0-750-g4e1d064212 (34)