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

Make every Vec3i use BetterBlockPos hash #4501

Open
wants to merge 1 commit into
base: 1.19.4
Choose a base branch
from

Conversation

babbaj
Copy link
Collaborator

@babbaj babbaj commented Sep 16, 2024

BetterBlockPos returning a different hashCode than the superclasses is a massively stupid correctness problem and is currently causing my game to crash.
In theory this might also improve performance a bit if it allows the jvm to devirtualize calls to hashCode.

@0-x-2-2 0-x-2-2 requested review from 0-x-2-2 and leijurv September 16, 2024 06:49
@0-x-2-2 0-x-2-2 added bug Something isn't working acepted This issue has been judged to be reasonable. labels Sep 16, 2024
@leijurv
Copy link
Member

leijurv commented Sep 17, 2024

This is a crazy big change, it could have tons of unintended side effects 😭

@ZacSharp
Copy link
Collaborator

ZacSharp commented Sep 18, 2024

Better stop passing BetterBlockPos as if it were a BlockPos, ideally removing the subclassing (but that causes hundreds of compile errors).

Alternatively if no BetterBlockPos is ever equal to any non-BetterBlockPos (as is normal for most classes) this won't be a problem at all. Just very annoying and error prone to work with. (And it would require a mixin into BlockPos.equals)

@babbaj
Copy link
Collaborator Author

babbaj commented Sep 19, 2024

I think the only other solution is to just not override the hashcode at all and figure out a different way to use the better hash function when using hashmaps but I don't think this could cause any problems anyways

@ZacSharp
Copy link
Collaborator

ZacSharp commented Sep 21, 2024

That different way of using the better hash would be a class equivalent to BetterBlockPos but without extending from BlockPos. That could be either BetterBlockPos itself (lots of breakage) or a reimplementation (unknown effort, probably easier).
In either case we'd probably want to migrate Baritone to use either our own position class or BlockPos exclusively (I vote for using our own) and convert explicitly were needed.

@babbaj
Copy link
Collaborator Author

babbaj commented Sep 21, 2024

The different way could just be fastutil's OpenCustomHashMaps or a simple class that just extends BetterBlockPos and is explicitly only to be used as keys to a hashmap. Creating a completely separate class would be unnecessarily annoying and would inevitably create a ton of unnecessary conversions that would make performance worse.

@ZacSharp
Copy link
Collaborator

Please not a class only meant to be used for hash maps. That's doomed to end where we are already.

@wagyourtail
Copy link
Collaborator

wagyourtail commented Sep 22, 2024

I don't think overriding the hashCode should cause any issues in practice, nothing should actually rely on the hashCode being the same between game launches...

anyway, the other way would be to extend HashMap/HashSet to overwrite what it querys for hashCode like babbaj said

@leijurv
Copy link
Member

leijurv commented Sep 30, 2024

@ZacSharp
Copy link
Collaborator

ZacSharp commented Oct 3, 2024

Is there any other reason to overwrite the hash code? (Other than general Minecraft performance, which I'd leave to optimization mods and mojang)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acepted This issue has been judged to be reasonable. bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants