Archipelago
Archipelago copied to clipboard
ArchipelagoMW
Core: Provide worlds a fill error hook for debugging
What is this fixing or adding?
adds a world method that gets called when fill fails.
How was this tested?
tested the current version by inspecting and then dumping the state from a random fill error from a test
Maybe a stupid question. What would a world authors put there? I feel like if it's about debugging in production, it would be better to make sure it's reproducible and dump yamls+seed instead.
i used it for debugging and dumping the state during dev. i trust my stuff well enough i probably wouldn't use it in prod and i highly doubt anyone else will either, but it's very convenient for finding issues during dev.
it could probably be modified to specifically dump yamls + seed instead but i'm not sure if it verifying it's reproducible is reasonable for prod because that would require it to store the seed before fill started, set it to that, and attempt fill again which might be thousands of items.
If it's supposed to only be used during dev, which is what i expected, I believe there should be a note about that in the doc string.
mmm. well it definitely could be used in prod. i don't think in its current iteration at least, but this could be used as a form of recovery for worlds that suffer from swap failures. i think i mused over using it before since i know what specific circumstances can cause messenger to fail to fill, but that'd require quite a bit more expansion on top of this to really be viable. even in its current state i think it can be useful for prod as a world can cater specific info to dump for user reports etc, i just don't think it'll be very widely used in that way. if you still just want the docstring changed for this i can do that though
I think I'd like the opinion of either Berserker or other world devs. I think postmortem analysis is good, however with rules in lambdas, they can't easily be dumped, making debugging very hard without a reproducible run, even if you can hook in.
I personally lean towards having the hook dev-only, explain that in the doc string, try to make postmortem from webhost nicer so you repro locally.
Reading the discussion here I am in agreement with sliver, I think it should be clearly dev only if it is to exist at all (I'm not too convinced it does need to exist but not a hill I'd die on)
For the record, my approval was from when I was a world dev, and more meant as an opinion of a world dev. Probably still good to get some more opinions
