Skip to content

Fix OnItemReceive hook for items in the extendedVanillaGetItem table #3063

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

Merged

Conversation

garrettjoecox
Copy link
Contributor

@garrettjoecox garrettjoecox commented Jul 3, 2023

Currently both the OnItemReceive and OnSaleEnd hooks are receiving inaccurate information about the item that was received if the item belongs to the extendedVanillaGetItem table.

For example if you receive the kokiri emerald, as far as the hooks are concerned you recieved a bottom of the well small key, because the table lookup is done with the ItemID instead of the GetItemID

Getting the Kokiri Emerald:

  • Before changes
    • getItemId: 0x6C (108) # Incorrect
    • itemId: 0x6C (108) # Correct
    • modIndex: 1 # Incorrect
  • After changes
    • getItemId: 0x7A (122) # Correct
    • itemId: 0x6C (108) # Correct
    • modIndex: 0 # Correct

Build Artifacts

Copy link
Contributor

@Malkierian Malkierian 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 minor thing, then I think it's ready.

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.

Super happy to see things move to enums from ints, left a little comment about the _MAX usage

if (ItemIDtoRandomizerGetMap.contains(itemID)) {
return ItemIDtoRandomizerGetMap.at(itemID);
}
return RG_MAX;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reasoning behind using RG_MAX here? it seems like it's being used to mean "invalid", so maybe it makes sense to add RG_INVALID to the enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my limited experience in Cpp land, I've seen ENUM_MAX be used to communicate an invalid value, as well as doing arbitraryValue < ENUM_MAX to validate it's a valid option before continuing (eg: we're doing this a few times with this enum in z_actor.c)

@Malkierian
Copy link
Contributor

Would this also depend on whether we wanted to maintain separate tables? Though, if it doesn't change anything else, I wouldn't mind adding it or the other one in the meantime to make maintaining Anchor easier while we work on the refactor.

@garrettjoecox
Copy link
Contributor Author

Would this also depend on whether we wanted to maintain separate tables? Though, if it doesn't change anything else, I wouldn't mind adding it or the other one in the meantime to make maintaining Anchor easier while we work on the refactor.

If you're meaning whether or not we want to merge the extendedVanillaGetItem into the randomizer table, then yes. But I don't think that will happen before v3

@garrettjoecox garrettjoecox force-pushed the fixOnItemReceiveHook branch 2 times, most recently from 5f0082b to 03d7d15 Compare November 15, 2023 04:29
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.

:shipit:

@garrettjoecox garrettjoecox merged commit 1fea642 into HarbourMasters:develop Dec 4, 2023
@garrettjoecox garrettjoecox deleted the fixOnItemReceiveHook branch March 4, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants