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

Fix: Minimap dungeon entrance placement #2958

Merged

Conversation

Archez
Copy link
Contributor

@Archez Archez commented Jun 6, 2023

This PR accomplishes a few goals around the minimap dungeon entrance icon:

  • Fix the missing dungeon entrance icons due to authentic game data values and our custom Fast3D opcodes
  • Fix dungeon entrance icon not positioning correctly with various hud placement settings
  • Simply the dungeon entrance icon logic

The first issue I noticed when working on the Mirror World PR was that all the dungeon entrance icons (except for ice cavern) never appeared. Digging into it I found the issue is that the games data uses large negative values for the Y pos values here:

static s16 sOwEntranceIconPosY[24] = {
0, -833, 0, 0, -850, -889, -829, 0, -844, 0, 0, -836, 0, 0, 0, -852, -873, 0, 0, -848, -825, -829, 0, -833,
};

In hardware this is fine as the game uses gSPTextureRectangle which truncates the number and only keeps 12 bits (U10.2). The issue is after we introduced and switched to gSPWideTextureRectangle the value was now only truncated at 24 bits, meaning the icon would now be placed off screen. Early cosmetic editor days shows that we were applying an arbitrary + 1024 which pushed the authentic value back to a positive number in the range that it should have been, but after a later cosmetic editor change, this "fix" was running after all the checks and was effectively dead code.

To address this I am reapplying this fix at the beginning of all the logic to bitwise AND with 0x3FF to keep only the first 10 bits (and later will be shifted by 2 for the U10.2 value). I opted for the bitwise over the 1024 cause it felt more clear why it was happening.

The second issue I noticed is that all the cosmetic margin/anchor logic was essentially broken for the icon, so I've refactored it following the pattern used for the minimap itself.

Other refactorings I did was merge duplicate code paths under a single branch and apply the gAlwaysShowDungeonMinimapIcon in the right spot (where the entrance flag is checked).

The other fix I added was to display the correct minimap in randomizer:

  • The Lake Hylia with filled lake minimap is now displayed when the water temple is cleared for rando
  • The Gerudo Fortress map showing horse back archery is shown when the membership card is found for rando

Let me know if we want to reduce the scope of this PR

Build Artifacts

@Archez Archez force-pushed the fix-minimap-dungeon-entrance branch from 502fa34 to 0f6748b Compare June 6, 2023 06:20
@jbodner09
Copy link
Contributor

Closes #2301 and #949

Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

overall this looks great! thanks for fixing/cleaning this up!

left a few comments about places where adding a comment might help make the logic clearer, and one little question

soh/src/code/z_map_exp.c Outdated Show resolved Hide resolved
soh/src/code/z_map_exp.c Outdated Show resolved Hide resolved
soh/src/code/z_map_exp.c Outdated Show resolved Hide resolved
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

one last little question, this looks pretty much good to go!

soh/src/code/z_map_exp.c Outdated Show resolved Hide resolved
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

beautiful

@leggettc18 leggettc18 merged commit ca9b161 into HarbourMasters:develop Jun 12, 2023
@garrettjoecox garrettjoecox added this to the Sulu (7.1.x) milestone Jun 13, 2023
@Archez Archez deleted the fix-minimap-dungeon-entrance branch June 17, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants