[UUM-148935] Make VectorHash quantization runtime-independent - #700
Open
lopezt-unity wants to merge 1 commit into
Open
[UUM-148935] Make VectorHash quantization runtime-independent#700lopezt-unity wants to merge 1 commit into
lopezt-unity wants to merge 1 commit into
Conversation
VectorHash.HashFloat hashed a float by casting it to ulong. ECMA-335 leaves conv.u8 on an out-of-range or NaN float unspecified, so each JIT emits whatever the underlying hardware instruction yields: Mono on x64 wraps around, CoreCLR (and Mono on Arm64) saturate. Saturation clamps every negative product to 0, causing hash collisions and equality/hashcode mismatches that are especially visible on CoreCLR and on macOS13 Arm64. HashFloat is replaced by VectorHash.RoundToInt, which maps NaN to 0, clamps explicitly instead of relying on the cast, and rounds half to even. IntVec2/IntVec3/IntVec4.round now call the same function instead of duplicating the quantization via Convert.ToInt32, which rounded differently than HashFloat truncated and caused GetHashCode to disagree with Equals for some coincident positions. Ported from unity/unity PR 123982, which fixed this in the ShadowPackages vendored snapshot of this package; this is the upstream fix that needs to land here so the next ShadowPackages refresh doesn't silently revert it.
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Harness Review
The change makes vector quantization explicit and shares it with integer-vector equality; I examined the affected equality/hash consumers, welding and export dictionary paths, picker usage, and the updated tests.
Reviewed commit 9b9d77f
🤖 Helpful? 👍/👎
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## master #700 +/- ##
==========================================
+ Coverage 38.43% 38.57% +0.13%
==========================================
Files 279 279
Lines 39164 39201 +37
==========================================
+ Hits 15052 15121 +69
+ Misses 24112 24080 -32
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of this PR
Ports the fix from unity/unity PR 123982, which fixed UUM-148935 and UUM-133532 (formerly UUM-111993) in the vendored
ShadowPackages/com.unity.probuildersnapshot in the engine repo. That PR's own description notes the fix must land here too, or it will be silently reverted on the next ShadowPackages refresh.VectorHash.HashFloathashed a float via(ulong)(f * FltCompareResolution). Casting an out-of-range or NaN float toulongis unspecified by ECMA-335: Mono on x64 wraps around, CoreCLR (and Mono on Arm64) saturate — collapsing every negative product to 0. That single cause explains all threeIntVectorTestsfailures.HashFloatis replaced byVectorHash.RoundToInt, which maps NaN to 0, clamps explicitly instead of relying on the cast, and rounds half to even — deterministic on every runtime and architecture.Second defect, present on every runtime today
IntVec2/IntVec3/IntVec4.roundquantized viaConvert.ToInt32(v * FltCompareResolution)whileHashFloattruncated — aGetHashCode/Equalscontract violation.SharedVertex.GetSharedVerticesWithPositionsandObjExporteruseIntVec3as aDictionarykey, so coincident vertices could land in different buckets and silently fail to weld. Routing both throughRoundToIntfixes this too, and drops anOverflowExceptionthatround()threw on NaN/infinite positions.Test changes
IntVectorTestsno longer need a CoreCLR-disable guard, since the underlying hashing bug — not CoreCLR itself — was the cause.TestVectorHashOverflow's expectations are updated to the new well-defined values (and aNegativeInfinitycase is added).TestEqualIntVec3SharesHashCode, which asserts theEquals/GetHashCodeagreement directly.Release Notes
See CHANGELOG.md.
Reviewed by
Reviewed against the merged engine-repo PR (unity/unity#123982) and the linked Jira ticket (UUM-148935); logic and test-expectation changes were independently re-derived and checked for consistency rather than copied blind.