search

cabaletta / baritone

google maps for block game
GNU Lesser General Public License v3.0
7.15k stars 1.43k forks source link

Make every Vec3i use BetterBlockPos hash #4501

Open babbaj opened 1 week ago

babbaj commented 1 week ago

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.

leijurv commented 1 week ago

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

ZacSharp commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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 commented 6 days ago

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 commented 6 days ago

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

wagyourtail commented 6 days ago

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