Shivers: Add events and fix require puzzle hints logic #4018
Conversation
# Conflicts: # worlds/shivers/Items.py # worlds/shivers/Rules.py # worlds/shivers/__init__.py # worlds/shivers/data/locations.json # worlds/shivers/data/regions.json
# Conflicts: # worlds/shivers/__init__.py
github-actions
bot
added
the
waiting-on: peer-review
Exempt-Medic
added
the
waiting-on: world-maintainer
|
To quote the docs regarding adding a world maintainer:
|
|
@Exempt-Medic thanks for pointing that out, added more info. |
worlds/shivers/__init__.py
Outdated
| for name, data in item_table.items(): | ||
| if data.type == ItemType.GOAL: | ||
| goal = self.create_item(name) | ||
| self.get_location("Mystery Solved").place_locked_item(goal) |
There was a problem hiding this comment.
You should be doing this when you create the location/item. pre_fill can run into problems with plando
Exempt-Medic
added
is: refactor/cleanup
worlds/shivers/__init__.py
Outdated
| elif library_random == 2: | ||
| librarylocation.place_locked_item(self.create_item("Key for Library Room")) | ||
| # Lobby access: | ||
| if self.options.lobby_access == 1: |
There was a problem hiding this comment.
These should use strings (or well, something that isn't just magic numbers)
| if self.options.lobby_access == 1: | |
| if self.options.lobby_access == "early": |
There was a problem hiding this comment.
Ideally yeah, I have refactors that I wanted to do I just didn't want to overload this PR more than I already have (I already did this with ItemTypes at least). A larger refactoring + all these changes I already made might be a bit much.
I will fix this though since we do use the string names elsewhere.
There was a problem hiding this comment.
I think doing this for two lines is fine
There was a problem hiding this comment.
Oh yeah the strings for sure, I was just responding on the "something that isn't just magic numbers", because also just using strings like this isn't ideal though it is better, enumeration would make more sense for options to me.
There was a problem hiding this comment.
Overall, the changes LGTM. Some small comments above and some of the world-specific changes GodlFire would definitely need to look at, but it's waiting on their approval anyway. Did 100 solo generations, 100 10-player generations and 100 Shivers+DS3/Witness generations. No errors (or warnings) were encountered in these generations. Playthrough seems to still be fully intact, did not compare region graphs or anything like that before and after but presumably Kory Dondzila and GodlFire would know about those anyway. Did not test for client operations with these changes, but again, presumably they would/did.
|
This is out-of-scope, but I think it would make sense to eventually convert the item_table or at least create smaller tables for the item types so that something like: [self.create_item(name) for name, data in item_table.items() if data.type == ItemType.POT]Could be: [self.create_item(name) for name in pot_item_table] |
Yep |
There was a problem hiding this comment.
World Maintainer:
As already stated by Kory we have discussed all of these changes. Looking at the changes the region graphs do look correct Exempt-Medic, as does all of the rule changes.
I have also run a seed and it went fine.
I only see this one rule that could be changed to use the new events.
Exempt-Medic
removed
the
waiting-on: world-maintainer
| region_info = load_data_file("regions.json") | ||
| years_since_sep_30_1980 = relativedelta(datetime.now(), datetime.fromisoformat("1980-09-30")).years |
There was a problem hiding this comment.
Is there a reason this uses relativedelta instead of something simple like:
| years_since_sep_30_1980 = relativedelta(datetime.now(), datetime.fromisoformat("1980-09-30")).years | |
| years_since_sep_30_1980 = datetime.now().year - 1980 |
There was a problem hiding this comment.
Or maybe something like:
datetime.now().year - 1980 + (datetime.now().month < 10)There was a problem hiding this comment.
Sep 30 1980 is a very specific date in the game. It's the date that the mystery officially occurs, and the game takes place in 1995, sometime in October or November (The game was released in November) though not specified.
So the victory item is basically an in joke where you've finally solved the mystery... X years later from that date. Relative delta just for it's accuracy honestly. (Though sadly the item name won't change if the game starts before Sep 30th but ends after Sep 30th).
Exempt-Medic
added
waiting-on: core-review
worlds/shivers/Items.py
Outdated
|
|
||
| # Victory item | ||
| f"Mt. Pleasant Tribune: {Constants.years_since_sep_30_1980} year Old Mystery Solved!": ItemData( |
There was a problem hiding this comment.
Wait a minute, no.
Your location_name_to_id cannot change from one day to the next like this.
It is static "per run of AP", yes, but that is not enough. If it's a "website game" (i.e. supported), it has to be static across a frozen build of AP, no matter which day you run that build on.
This is because the website loads and caches location_name_to_id at different points of time / on different schedules for different purposes. Some things only happen once per server restart (If we didn't have the server crashes, options pages would still use the datapackage from the release date of 0.5.0), whereas for generation, a new instance of AP is loaded.
As such, we currently do not allow this for supported games.
If you really want this, you'll have to make different items per year somehow. E.g. reserve like 100 of them, and then make an item just called "over 100 years ago" in case AP lasts until 2080 and we don't add dynamic datapackage support until then.
There was a problem hiding this comment.
I have been informed this may be an event item. If it is code = None, then ignore everything I said.
There was a problem hiding this comment.
The item does appear to have a code in its ItemData, which makes me assume it's a real item
There was a problem hiding this comment.
Ah shit really? And here I was thinking I was doing something fun. What would that actually affect / break (In the window of being around Sep 30th each year)? I don't really know how everything works.
Hmm like I know it affects the web host in this way, though ugh I wish some items could be explicitly removed from these lists (since plenty of games have items that shouldn't be plando but aren't events).
Because I could generate a bunch of these items with static years and the generation picks the one it wants to use at run time. (I mean could that really just be restricted to a couple years, since if it's too old it just falls off the list and too new then it won't matter. i.e. current +-1 year 43 44 and 45, then when next year rolls around it's 44 45 and 46? Could keep or update the ids as well)
But again the webhost showing those is annoying.
And I know items show up in the tracker if received, but my assumption is that comes from whatever the generation said?
Actually this kind of makes me wonder why the name is the source of truth if this is the case... I would think that the ID would be.
There was a problem hiding this comment.
@NewSoupVi didn't see the other messages, this is not an even item, it could be but the point was to have it show up when the player goals. So in any case it's never an item that will be randomized
There was a problem hiding this comment.
Hmm... a rolling item buffer might be okay. The reason I'm a bit wary is because it would still mean that some parts of the website would be working with:
43 years ago: ####0001
44 years ago: ####0002
45 years ago: ####0003
where other parts would be working with
44 years ago: ####0001
45 years ago: ####0002
46 years ago: ####0003
So, while this would mean that any name that would actually be used is always in there as long as there is a server restart at least once a year (lol), but which name belongs to which ID could still be mismatched.
I don't think that causes any issues, but I'm not sure. I'd have to verify with other core devs
There was a problem hiding this comment.
I can associate the id with the year
43 years ago: ####00043
44 years ago: ####00044
45 years ago: ####00045
44 years ago: ####00044
45 years ago: ####00045
46 years ago: ####00046
So never having a mismatched name to id.
There was a problem hiding this comment.
Oh sure. At that point I don't really see the benefit over just reserving a bunch ahead of time, other than I guess "it works forever even when unmaintained", which I don't imagine will be relevant but it is technically safer. Lol
There was a problem hiding this comment.
I'm both lazy and don't want a bunch of extra stuff laying around that won't get used lol.
There was a problem hiding this comment.
@NewSoupVi ok now that I had time to do this, done. Have my rolling 3 item buffer and removed them from valid keys so they don't show on web.
…rom default options.
| @@ -107,6 +124,61 @@ class PuzzleCollectBehavior(Choice): | |||
| default = 1 | |||
|
|
|||
|
|
|||
| # Need to override the default options to remove the goal items and goal locations so that they do not show on web. | |||
There was a problem hiding this comment.
This whole process doesn't really fix everything. Plando still exists, among other things, so it's still going to run into problems, it's also kind of "hacky" to overwrite all of these. Can you really not do anything else?
There was a problem hiding this comment.
I mean this is what was suggested, if there's a better way I would love to know.
There was a problem hiding this comment.
Just using an event?
There was a problem hiding this comment.
As far as I can tell, the only thing this is being used for is the completion_condition, so I'm not sure why this isn't just an event (ID None)
There was a problem hiding this comment.
I think I mentioned it before but I was trying to be weird and have an in joke goal item that displays when you beat the game. Event items will not do that.
However something like the Map Complete items in the Dooms/Heretic would be similar and display in these options when they probably should not. afaik this is the only game so far attempting do this, but nothing says it can't. Plando is of course still a problem, but that requires deeper knowledge, the web based options is at least something that can be mitagated.
There was a problem hiding this comment.
Can't you just make the client display the item anyway? Just fake one, basically?
There was a problem hiding this comment.
Idk, I'll still leave my approval, but I'm starting to like less and less all the weird things worlds are doing with non-event "events" XD
There was a problem hiding this comment.
At least this cleans up all the other stuff that should actually have been events but weren't and added events to clean up rule logic. I just want my one small weird thing lol.
What is this fixing or adding?
Adds events to help clean up some logic and fixes require puzzle hints so that entrances are also accounted for and adds missing logic.
Also just some general cleanup.
How was this tested?
Ran multiple generations to make sure nothing failed and played a few games solo and with others, including GodlFire the existing world maintainer, to make sure the logic was correct.
New Maintainer
I have made a few small prior changes to Shivers AP code.
#2690
#2869
#3558
#3854
Though I have primarily been a maintainer for the Shivers client.
https://github.com/GodlFire/Shivers-Randomizer-CSharp/pulls?q=is%3Apr+author%3Akorydondzila+
I was asked by GodlFire to figure out how to get events working so that redundancy could be removed and logic improved. Also to remove the pot duplicates from the item pool so they don't show up in the advanced options. (these items are solely used to indicate where each pot goes when received but are important for progression)