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

Cross out pre-completed dungeons in dungeon info menu #2244

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

GSKirox
Copy link
Collaborator

@GSKirox GSKirox commented Jun 10, 2024

Currently the only in-game way to tell if a dungeon is pre-completed is by comparing the dungeon rewards with the map/compass informations.
This PR adds the ability to tell pre-completed dungeons apart directly in the rando-custom dungeon info menu (A in pause screen).
They will be grayed and crossed out :
image

This should be fully compatible with side dungeons and Ganon Castle, if they get implemented later as pre-completed valid choices.

@fenhl
Copy link
Collaborator

fenhl commented Jun 10, 2024

This should also be displayed on the dungeon info menu (D-left). And I'm not sure it makes sense to gray out the key counts, that kinda implies you're not going to need the keys, which isn't necessarily going to be true in mixed pools bosses or blue warp ER.

@fenhl fenhl added Type: Enhancement New feature or request Component: ASM/C Changes some internals of the ASM/C libraries Status: Needs Review Someone should be looking at it Status: Needs Testing Probably should be tested Status: Under Consideration Developers are considering whether to accept or decline the feature described Component: Patching Affects the patching of the ROM labels Jun 10, 2024
Copy link

@flagrama flagrama left a comment

Choose a reason for hiding this comment

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

The Patches.py comment is just a suggestion, the other two are changes that should be made though.

ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
@@ -2771,6 +2771,7 @@ def configure_dungeon_info(rom: Rom, world: World) -> None:
dungeon_rewards[codes.index(area.dungeon_name)] = boss_reward_index(location.item)

dungeon_is_mq = [1 if world.dungeon_mq.get(c) else 0 for c in codes]
dungeon_precompleted = [1 if world.empty_dungeons[c].empty else 0 for c in codes]

Choose a reason for hiding this comment

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

The existing names chosen for pre-completed dungeons seem to be misleading. They aren't empty dungeons, and empty as a property on the dungeon almost looks like a language feature to check if the index in an array is empty.

For clarity maybe these should be renamed to precompleted_dungeons and precompleted?

Copy link
Collaborator

@fenhl fenhl Jun 10, 2024

Choose a reason for hiding this comment

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

I'm not sure the .empty attribute access is needed at all. I think World.empty_dungeons could just be a dict[str, bool] (or maybe even set[str]). I guess this PR is as good a time to change it as any, but it shouldn't be a blocker since it could also be handled in a separate PR. Regarding the name change, the current name matches the internal/plando name for the setting, so it should stay for now imo. Hopefully we can eventually accept #1839 so we can rename it.

@GSKirox
Copy link
Collaborator Author

GSKirox commented Jul 24, 2024

Rewrote the change with the newly added text printing functions, and added the crossed dungeons to the dpad left menu.

@fenhl fenhl removed Status: Under Consideration Developers are considering whether to accept or decline the feature described Status: Needs Testing Probably should be tested labels Aug 20, 2024
@fenhl fenhl requested a review from flagrama August 21, 2024 18:10
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/src/config.asm Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
ASM/c/dungeon_info.c Outdated Show resolved Hide resolved
@fenhl fenhl added the Status: Waiting for Author Changes or response requested label Aug 22, 2024
Copy link

@flagrama flagrama left a comment

Choose a reason for hiding this comment

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

Just one last question.

ASM/c/dungeon_info.c Show resolved Hide resolved
@fenhl fenhl requested a review from flagrama September 3, 2024 06:54
@fenhl fenhl removed the Status: Waiting for Author Changes or response requested label Sep 3, 2024
@fenhl fenhl added this to the next milestone Sep 3, 2024
@fenhl fenhl removed the Status: Needs Review Someone should be looking at it label Sep 4, 2024
@fenhl fenhl merged commit a93636d into OoTRandomizer:Dev Sep 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ASM/C Changes some internals of the ASM/C libraries Component: Patching Affects the patching of the ROM Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants