-
Notifications
You must be signed in to change notification settings - Fork 723
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 Messenger: more optimizations #2451
The Messenger: more optimizations #2451
Conversation
# Conflicts: # worlds/messenger/regions.py # worlds/messenger/rules.py
"music_box": self.options.music_box.value, | ||
"required_seals": self.required_seals, | ||
"mega_shards": self.options.shuffle_shards.value, | ||
"logic": self.options.logic_level.current_key, |
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.
Is it intentional that the key "logic" is being removed and replaced by "logic_level"?
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.
yes. this is just for the tracker, and the newer way both reduces the slot data by a tad bit and looks cleaner imo.
self.add_locations(shop_locations, MessengerShopLocation) | ||
elif self.name == "The Craftsman's Corner": | ||
self.add_locations({figurine: world.location_name_to_id[figurine] for figurine in FIGURINES}, | ||
MessengerLocation) |
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.
Don't the figurine locations need some access rules that take their price into account?
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.
No, because 90% of the time the maximum price to get to the craftsman's corner will be world.total_shards
, so adding more rules that need to be checked after that won't do anything but slow it down, and I don't think they're worth adding for the incredibly slim chance that the total shards is actually greater than those shop slots prices, since shards are easy enough to grind for in game. The main reason they're in logic is just because money grinding isn't fun.
…ired, and no seals will exist when the option is disabled
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.
(Didn't play any; just looked at code.)
More speed optimizations for The Messenger. Moves Figurines into their own region, so their complicated access rule only needs to be calculated once when doing a sweep. Removes a redundant loop for shop locations by just directly assigning the access rule in the class instead of retroactively. Reduces slot_data to only information that can't be derived, and removes some additional extraneous data. Removes some unused sets and lists. Removes a redundant event location, and increments the required_client_version to prevent clients that don't expect the new slot_data. Drops data version since it's going away soon anyways, to remove conflicts.
What is this fixing or adding?
More speed optimizations for The Messenger. Moves Figurines into their own region, so their complicated access rule only needs to be calculated once when doing a sweep. Removes a redundant loop for shop locations by just directly assigning the access rule in the class instead of retroactively. Reduces slot_data to only information that can't be derived, and removes some additional extraneous data. Removes some unused sets and lists. Removes a redundant event location, and increments the required_client_version to prevent clients that don't expect the new slot_data. Drops data version since it's going away soon anyways, to remove conflicts.
How was this tested?
Unit tests, a few thousand generations, and updating and testing client.
If this makes graphical changes, please attach screenshots.