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

LADX: Converted to new options API (+other small refactors) #3542

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

hatkirby
Copy link
Collaborator

What is this fixing or adding?

This PR contains a handful of refactors for LADX:

  • It now uses the new options API, and most options access goes through named fields on the objects object rather than through getattr.
  • Moved away from using self.multiworld.random or per_slot_randoms in favor of the world random field.
  • Moved away from accessing other various fields through self.multiworld, such as the player's name.
  • The world object is now passed to generateRom so that the various parameters can be accessed through it instead of having to be passed manually, since they are almost all members of the world class anyway.
  • Fixed a couple of typos.

How was this tested?

Ran 100 generations with random options. pytest continues to pass.

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jun 15, 2024
@hatkirby hatkirby added the is: refactor/cleanup Improvements to code/output readability or organizization. label Jun 15, 2024
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.

Looks good, two small things

worlds/ladx/__init__.py Outdated Show resolved Hide resolved
worlds/ladx/__init__.py Show resolved Hide resolved
@hatkirby hatkirby requested a review from NewSoupVi June 16, 2024 14:16
patches.aesthetics.removeLowHPBeep(rom)
if 0 <= int(settings.linkspalette):
patches.aesthetics.forceLinksPalette(rom, int(settings.linkspalette))
if 0 <= int(world.ladxr_settings.linkspalette):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is another unneeded cast

Suggested change
if 0 <= int(world.ladxr_settings.linkspalette):
if 0 <= world.ladxr_settings.linkspalette:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is a string, so the cast actually is needed.

Copy link
Collaborator

@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.

Aside from two really minor things, these changes all LGTM. Asked a few questions about some of them and got clarifications, merged into main, did 500 random generations and saw that no warnings were printed for deprecated options, per_slot_randoms or anything else. Couldn't find any other obscure/hidden uses of deprecated option getters either. The playthroughs for the generated worlds looked fine as well

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
@NewSoupVi NewSoupVi merged commit af213c9 into ArchipelagoMW:main Jun 18, 2024
17 checks passed
@hatkirby hatkirby deleted the ladx-generate branch June 18, 2024 02:49
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
…agoMW#3542)

* Refactored various things

* Renamed hidden variable in dungeon item shuffle block

* Fixed LADXRSettings initialization

* Rename ladxr_options -> ladxr_settings

* Remove unnecessary int cast

* Update worlds/ladx/LADXR/generator.py

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

---------

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
…agoMW#3542)

* Refactored various things

* Renamed hidden variable in dungeon item shuffle block

* Fixed LADXRSettings initialization

* Rename ladxr_options -> ladxr_settings

* Remove unnecessary int cast

* Update worlds/ladx/LADXR/generator.py

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

---------

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
GameWyrm pushed a commit to GameWyrm/Archipelago-GW that referenced this pull request Jul 4, 2024
…agoMW#3542)

* Refactored various things

* Renamed hidden variable in dungeon item shuffle block

* Fixed LADXRSettings initialization

* Rename ladxr_options -> ladxr_settings

* Remove unnecessary int cast

* Update worlds/ladx/LADXR/generator.py

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

---------

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
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: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants