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

HK: Options API updates, et al. #3428

Merged
merged 4 commits into from
Jul 28, 2024
Merged

Conversation

qwint
Copy link
Contributor

@qwint qwint commented Jun 1, 2024

What is this fixing or adding?

updates HK to:

  • consistently use world.random
  • use world.options
  • don't use world = self.multiworld
  • removes some things from the logicMixin
  • use new options dataclass

How was this tested?

couple generations on base settings
generated template yaml and compared to previous one

If this makes graphical changes, please attach screenshots.

qwint added 2 commits June 1, 2024 11:29
… use world = self.multiworld, and remove some things from the logicMixin
@qwint qwint requested a review from ThePhar as a code owner June 1, 2024 17:24
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jun 1, 2024
@Exempt-Medic Exempt-Medic added the is: refactor/cleanup Improvements to code/output readability or organizization. label Jun 1, 2024
Copy link
Collaborator

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

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

Seems largely good as best as I can tell

Not that I care either way but I am curious to what degree pycharm completion and analysis tolerates/cooperates with the generated data class

worlds/hk/__init__.py Outdated Show resolved Hide resolved
@@ -603,15 +640,15 @@ def get_filler_item_name(self) -> str:
'RandomizeGeoRocks', 'RandomizeSoulTotems', 'RandomizeLoreTablets', 'RandomizeJunkPitChests',
'RandomizeRancidEggs'
):
if getattr(self.multiworld, group):
if getattr(self.options, group):
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is the truthiness of the options classes handled? Might need a .value on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which ones are you worried about? because options are pretty good at truthy handling,
i can always cast to a bool if you want it to be explicit too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI confirmed that a yaml with all the relevant filler shuffle options turned off and with 9 shop slots for each option
had (except the one Collector's map and Salubra's blessing) only "one_geo" and "soul_refill" filler items

strange unrelated note: i had to filter down to items with .code not None because for some reason lifeblood cocoons were being created as events and i'm not sure why

@@ -684,42 +721,7 @@ def _hk_notches(self, player: int, *notches: int) -> int:
return sum(self.multiworld.worlds[player].charm_costs[notch] for notch in notches)

def _hk_option(self, player: int, option_name: str) -> int:
return getattr(self.multiworld, option_name)[player].value
return getattr(self.multiworld.worlds[player].options, option_name).value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious what the blocker is for the remaining ones to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ones i moved are only used in setting completion condition, I just didn't want to go through the effort to confirm the other ones can be properly moved out of logicmixin (yet)

@qwint
Copy link
Contributor Author

qwint commented Jun 2, 2024

did run a small multi with two HK worlds (and two others) on 24242 and saw no issues

Copy link
Collaborator

@agilbert1412 agilbert1412 left a comment

Choose a reason for hiding this comment

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

Looked over all the code, couldn't find any errors. 95% of the changes are the same thing over and over

Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Small things, everything else looks good. A couple things like self.player_name could fit in alongside the deprecation fixes, but it's out of scope for this PR. The game generates just fine and the option getter warnings are removed, though it would be nice if _hk_notches could be updated as well

worlds/hk/Rules.py Outdated Show resolved Hide resolved
worlds/hk/__init__.py Outdated Show resolved Hide resolved
worlds/hk/__init__.py Outdated Show resolved Hide resolved
worlds/hk/__init__.py Outdated Show resolved Hide resolved
worlds/hk/__init__.py Outdated Show resolved Hide resolved
@Exempt-Medic Exempt-Medic added waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 7, 2024
@BadMagic100 BadMagic100 added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Jun 8, 2024
@NewSoupVi NewSoupVi merged commit e764da3 into ArchipelagoMW:main Jul 28, 2024
17 checks passed
@qwint qwint deleted the HK_fixes branch July 28, 2024 22:02
AustinSumigray pushed a commit to AustinSumigray/Archipelago that referenced this pull request Jan 4, 2025
* updates HK to consistently use world.random, use world.options, don't use world = self.multiworld, and remove some things from the logicMixin

* Update HK to new options dataclass

* Move completion condition helpers to Rules.py

* updates from review
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. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants