Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DS3: Fix health locations setting not enabling #2147

Merged
merged 3 commits into from
Sep 16, 2023

Conversation

Zunawe
Copy link
Collaborator

@Zunawe Zunawe commented Sep 3, 2023

What is this fixing or adding?

Health items (estus shards and undead bone shards) are contained within the progressive item tables. So if progressive items aren't enabled, health items can never be added to the multiworld, even if the player has them turned on using enable_health_upgrade_locations. The only way to have health locations added is to turn on both enable_health_upgrade_locations and enable_progressive_locations.

This moves health item locations to their own table so they can be added to the list of progressive items separately from the rest of the consumables. This changes the location ids of just these locations, but nothing we know of checks these location ids. The client will work with any location ids out of the box, and the tracker doesn't monitor progressive locations.

How was this tested?

Generating a game for each combination of enable_health_upgrade_locations and enable_progressive_locations to verify that the expected locations are enabled.

Copy link
Collaborator

@nbrochu nbrochu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion to avoid future pain would be to decouple healing items from progressive items 1 (even though it would cause short term pain in affecting the location IDs). Right now, the fact that there is filtering happening in 2 places in world initialization based on the possible combinations of options means there is coupling that shouldn't be there at the data level. The options seem independent in the YAML but one is a subset of the other, which just leads to confusion.

The fix in this PR will work. We just have to be aware that it's likely "tech debt" if the integration is ever to be pushed further.

@Zunawe
Copy link
Collaborator Author

Zunawe commented Sep 4, 2023

Yeah, that's the more proper fix, but I'd like to avoid changing location ids for a bugfix if I can help it.

Though, I suppose the only consumers of location ids at the moment other than the data package are the mod and the tracker. And the mod won't care because it correlates location ids to memory addresses via slot_data, and the tracker won't care because it doesn't currently try to track health items anyway. So there'd only really be the naive hope of somehow making it into a 0.4.2 hotfix.

Thoughts, @Br00ty?

@Br00ty
Copy link
Contributor

Br00ty commented Sep 4, 2023

it really shouldn't affect anything if the items in the 'Progressive Items' locations get moved around ID wise. probably wouldn't be a bad idea to have them in their own category separate from the rest of the consumables and such. it wont effect any of the 'important locations' ids at least. looking through the code, it should be easy enough to move the estus/bone shards to the end of the locations, and tie all the health stuff together by themselves, without affecting really anything

@Zunawe
Copy link
Collaborator Author

Zunawe commented Sep 9, 2023

Moved them to their own table then. Again, generated with all 4 combinations to make sure the expected locations were included in the spoiler logs.

@Zunawe
Copy link
Collaborator Author

Zunawe commented Sep 15, 2023

Rolled and played some seeds to verify it definitely works in a game:
Both Progressive and Health
Only Health
Only Progressive
Neither

Everything worked as expected. Locations were randomized when they were supposed to be and vanilla when they weren't, pickups were sent to the server correctly, and items were received from the client correctly. No mismatches or anything.

@black-sliver
Copy link
Member

Oh i forgot. If it changes IDs, you should also change the data_version in init.py

@Zunawe
Copy link
Collaborator Author

Zunawe commented Sep 16, 2023

Right. Thanks for catching that.

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine. Thanks!

@black-sliver black-sliver merged commit cff6c7c into ArchipelagoMW:main Sep 16, 2023
12 checks passed
@Zunawe Zunawe deleted the ds3-fix-health branch September 16, 2023 21:45
@ThePhar ThePhar added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Oct 16, 2023
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* DS3: Fix health locations setting not enabling

* DS3: Move health locations to their own table

* DS3: Bump data version
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* DS3: Fix health locations setting not enabling

* DS3: Move health locations to their own table

* DS3: Bump data version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants