Closed kmecpp closed 2 years ago
Noticed after I submitted the PR that it was only being used for randomness. Updated to make it more random. Still ~5x faster than the original code with the cryptographic hash
The SHA1 function definitely needs replaced (unrelated to this is that the sorting algorithm that it's used by should probably be run on the manager thread rather than in the post-completion function). A couple of issues with your current solution:
Because you are hashing each coordinate individually and then summing them together, the x/y/z values are commutative. This causes some undesirable rotational symmetry when the blast is used on a diagonal axis https://imgur.com/a/j6U81sp. This isn't a huge deal currently, but at some point I plan to implement pre-baking of the blast crater offsets, centered at 0/0/0 so that it can be offset and applied to the world when the blast is triggered without having to be calculated on-the-fly. Under that system, these artifacts would be present every time. This is the original reason that I chose to use concatenation for the input rather than addition; using a cryptographic function rather than something custom was just me being lazy.
This post by Thomas Mueller (who I believe is the original source of the 0x119de1f3 constant that you're using, based on the source code that they provide for deriving it) https://stackoverflow.com/questions/664014/what-integer-hash-function-are-good-that-accepts-an-integer-hash-key suggests using the logical shift right (>>>) operator rather than the arithmetic shift right (>>) when using Java. No idea if it matters for this application.
If you can fix the rotational symmetry issue then I'd be happy to merge this 👍
Closing - stale.
This is the standard way to produce a hashCode with a good distribution. See java.util.Arrays.hashCode()