Options: set old options api before the world is created #2378
Conversation
…multiple times for duplicate world types
|
I wont have time to test this until Sunday/Monday but this is meant to resolve #2374 one concern I do have is the plans for depreciation in 1 version, out of experience it takes multiple versions to get a world PR merged |
Jarno458
added
is: bug/fix
|
I'm fine with the deprecation notice, since it is officially already deprecated. As for the While 1. sounds like cleaner OOP, I feel like 2. is more useful to our use-case, because that allows better scheduling, multiplexing, inspecting, multi-threading, etc. of the stages. |
It was a deliberate design decision to not do 1 because passing the options to the world isn't very clean (for how AP works) and I explicitly don't want people doing multi-world things in the constructor. With the way item links functions, I think doubling down on this behavior is the correct path forward, since a world doing a whole bunch of stuff to "set up" in its constructor is really awful since none of that information will be used for an item link world. Will add documentation for this in a separate PR. |
Co-authored-by: black-sliver <[email protected]>
There was a problem hiding this comment.
lgtm unless @Berserker66 has a problem with the deprecation spam
|
it's probably fine, my main worry is that this means explicitly having to be careful about using things that are created in generate early for item links filler, which I don't think we have a unittest for, and itemlinks aren't often tested ahead of borked. |
|
Given that the world instance is still in the process of being set up when its |
|
There is some stuff that you may want to have in Breaking access to options and random once the old API is gone should be good enough to make sure |
|
Finely got to testing it, works fine, options are available in constructor. the deprication warnings do be a bit spammy tho
|
LGTM |
|
I'm not sure I want to hit merge. On one hand, the spam is good to get the attention of world devs, on the other hand, APWorlds can't really update until the next version is out. Should we maybe split this PR into two and merge the jarno fix now, and the rest later? |
|
I can if that's what we really want, but that'll push deprecation back 2 versions. Users also won't get the spam, and world devs can mute it by running with -O |
|
Maybe make it log only 1 UserWarning per yaml or world type even? |
each option under the old API is a separate dictionary, so the most I could do is once per option per world, which won't reduce the spam by much, if any. |
|
So i added the release/blocker label to it as it broke for Timespinner on current main as Timespinner used to rely on reading the options in the constructor and i had no idea for what other worlds it might have been broken However Timespinner moved its options usage to generate_early so this is nolonger a show stopped for Timespinner and since no other games have reported broken i think the plan is to move on so the blocker label can be removed i guess |
What is this fixing or adding?
Old options API had options being set before worlds were created and some worlds relied on that. That behavior has been purposefully deprecated, but old API should still use it for back compat.
Also deprecates the old options API. these can be dropped and wait a version if really desired but we've already made the effort of making everyone aware and I'd prefer to properly deprecate it next version.
How was this tested?
unit tests. Also very low risk.
If this makes graphical changes, please attach screenshots.