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

[1.20.6] Use stack hash strategy for mutable map in creative tab event #1043

Merged
merged 5 commits into from
Jun 5, 2024

Conversation

dhyces
Copy link
Contributor

@dhyces dhyces commented May 31, 2024

Vanilla disallows duplicate elements in creative mode tabs, throwing a pretty unhelpful error:

Accidentally adding the same item stack twice [Wooden Sword] to a Creative Mode Tab: Damaged Wooden Swords

The current BuildCreativeModeTabContentsEvent mutable entries map does allow duplicate elements due to using default object comparison and hashing. This PR changes the hash strategy used to ItemStackLinkedSet#TYPE_AND_TAG

Fixes #1040.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@TelepathicGrunt
Copy link
Contributor

TelepathicGrunt commented May 31, 2024

Uhhh. Are you positive you are not going to reintroduce this bug where the creative menu enchanted books gets broken or order incorrectly? #920

I feel like there was some issue I had before with the hashing and enchanted books in the menu. And had to remove the hashing in order to allow the enchanted books to show up properly

@dhyces
Copy link
Contributor Author

dhyces commented May 31, 2024

I was not aware of that issue. This does reintroduce that bug. If this does not have a proper solution, then a better error should at least be implemented along with an explanation on the event hook with why the custom hash strategy cannot be used.

@TelepathicGrunt
Copy link
Contributor

If testing and the enchanted book bug doesn’t happen, then this pr should be alright. Though maybe we should have a junit test added to verify that two of exact same item errors but enchanted books passes. See my Neo startup config PR for an example of a junit test that is also a test mod

@TelepathicGrunt
Copy link
Contributor

TelepathicGrunt commented May 31, 2024

ah I see how vanilla bypasses their enchanted books. They deliberately skip checking search only entries lmao.
image

Tested PR and it does break the books tho. While the enchanted books do show up in the search, the normal books are not showing under the Ingredients tabs.
image

I think this PR's fix needs to change to allow duplicate itemstacks if the duplicates are search only mode. Also note, vanilla's check is actually order dependent too! So if you add search only itemstack first, and then the normal tab itemstack, you hit the duplicate exception error. But not the other way around. So that check will also need to be added to neo

@TelepathicGrunt TelepathicGrunt added bug A bug or error 1.20.6 Targeted at Minecraft 1.20.6 labels May 31, 2024
@TelepathicGrunt TelepathicGrunt self-requested a review May 31, 2024 13:42
Copy link
Contributor

@TelepathicGrunt TelepathicGrunt left a comment

Choose a reason for hiding this comment

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

Ok lets see. I need to leave a proper review asking for changes. PR just needs to solve this bug:
image

dhyces added 3 commits June 2, 2024 22:20
Not a big fan of the impl. Honestly would rather just remove the map entirely and use something else
@dhyces dhyces requested a review from TelepathicGrunt June 3, 2024 17:55
@sciwhiz12 sciwhiz12 merged commit caf97c9 into neoforged:1.20.x Jun 5, 2024
4 checks passed
@dhyces dhyces deleted the fix/stack-hash-tabs branch June 7, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20.6 Targeted at Minecraft 1.20.6 bug A bug or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BuildCreativeModeTabContentsEvent uses wrong hashing strategy for entries
3 participants