-
Notifications
You must be signed in to change notification settings - Fork 1.2k
The Witness: Use entity IDs as location IDs directly #5526
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
base: main
Are you sure you want to change the base?
The Witness: Use entity IDs as location IDs directly #5526
Conversation
2a7b324
to
5864abc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through all of the files except player_logic.py
. None of the code stood out as an issue to me. Data files seem to be accurately converted to exclude the old AP numbers.
Did not check other files for potentially missing references nor any now unused values or functions. One small comment on a value within the files, otherwise everything I looked over seems fine.
Did not test generation
from .static_locations import ID_START | ||
|
||
ID_START = 158000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to keep ID_START
if cross-game ID conflicts are no longer an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since items are not changing, I see no reason to disturb their IDs atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like yes I could also change the ID of the Dots item from 158010 to 10 or whatever, but I guess like... I don't really see a need, since the item IDs are already completely arbitrary. It just means I have to make another breaking / backwards compatible client change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair
Nowadays that we have per-game IDs, making the distinction between entity IDs and location IDs no longer makes sense.
This greatly reduces the size of a Witness slot's slot_data by removing the conversion dict from one to the other.
Changes:
New client: https://github.com/NewSoupVi/The-Witness-Randomizer-for-Archipelago/releases/tag/9.0.0p1
Probably needs more testing (like seed comparisons and mypy checking)