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

The Witness: Item loading refactor. #1953

Merged
merged 16 commits into from
Jul 19, 2023

Conversation

blastron
Copy link
Collaborator

@blastron blastron commented Jul 6, 2023

What is this fixing or adding?

This is an in-place refactor to the code that loads Witness-specific items from its configuration files. No actual AP- or player-facing behavior is different.

How was this tested?

I generated a bunch of runs with fixed seed values and compared spoiler logs before/after to verify that they were identical.

If this makes graphical changes, please attach screenshots.

N/A

NewSoupVi and others added 15 commits June 26, 2023 00:48
This change shuffles around a lot of the item loading and creation logic for the purpose of increasing readability and setting me up to work on the speed boost rework. These changes are fairly extensive, but they've been tested rigorously to ensure that the actual output does not differ at all between the existing code and this refactor.

This does not touch any of the logic/location code except where necessary to interface with the item refactor.

Summary of changes:
- Condensed all fields that stored redundant data into a smaller number of unique lists, replacing all uses of those fields with list/dictionary comprehensions where relevant.
- Moved most item generation logic out of WitnessWebWorld and into WitnessPlayerItems. WitnessWebWorld.create_items() now only builds the actual list of items and places specific items on specific locations, with all of the actual item selection behavior happening in WitnessPlayerItems.
- Replaced various anonymous data structures with ones with concrete fields, such as ItemData.
- Moved filler/trap ratios into WitnessItems.txt and reworked the generation logic to calculate total quantities before creating any items.
- Added "joke" item category. Joke items are automatically added to the junk pool.
Refactor + Code quality improvements (Motivation: Make it easier to add new junk items)
@ThePhar ThePhar added the is: refactor/cleanup Improvements to code/output readability or organizization. label Jul 6, 2023
@ThePhar
Copy link
Member

ThePhar commented Jul 6, 2023

@NewSoupVi

@NewSoupVi
Copy link
Member

NewSoupVi commented Jul 6, 2023

@NewSoupVi

Blastron is a new dev for this world (and long-time dev of the client). I approved his changes and asked him to take the lead on the PR process as well :) So please treat him as the main person responsible~

@blastron
Copy link
Collaborator Author

blastron commented Jul 6, 2023

thanks for adding the tag, i'll be sure to check those in the future

# Generate the actual items.
for item_name, quantity in item_pool.items():
self.multiworld.itempool += [self.create_item(item_name) for _ in range(0, quantity)]
if self.items.item_data[item_name].local_only:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: this is not currently being used by anything live. I've tested it locally with my other changes.

@ThePhar
Copy link
Member

ThePhar commented Jul 6, 2023

@NewSoupVi

Blastron is a new dev for this world (and long-time dev of the client). I approved his changes and asked him to take the lead on the PR process as well :) So please treat him as the main person responsible~

Ah shoot, that's right I forgot. My bad. Should have checked my own CODEOWNERS sheet lol.

thanks for adding the tag, i'll be sure to check those in the future

All good, it's 80% of what I do on GitHub anyway. :P

@black-sliver
Copy link
Member

Sorry for merging the other PRs first, creating a merge conflict, but they were labelled as fixes.

@NewSoupVi can you still review and test, if not done already, and approve the changes (using the github approve feature)?

Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Already reviewed, I can approve yes

Altho yeah the merge conflicts will have to be solved ofc

@ThePhar ThePhar merged commit 1f6db12 into ArchipelagoMW:main Jul 19, 2023
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
Co-authored-by: el-u <[email protected]>
Co-authored-by: NewSoupVi <[email protected]>
Co-authored-by: black-sliver <[email protected]>
kl3cks7r pushed a commit to kl3cks7r/Archipelago that referenced this pull request Dec 15, 2023
Co-authored-by: el-u <[email protected]>
Co-authored-by: NewSoupVi <[email protected]>
Co-authored-by: black-sliver <[email protected]>
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
Co-authored-by: el-u <[email protected]>
Co-authored-by: NewSoupVi <[email protected]>
Co-authored-by: black-sliver <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants