box/aabb2d: fix wrong face selected below AABB min bound in sphere/circle test - #496
Merged
Merged
Conversation
…rcle test glm_aabb_sphere() and glm_aabb2d_circle() pick which box face to measure axis distance to via `!(a - 1)`, where `a = (s[i] < box[0][i]) + (s[i] > box[1][i])` is 1 for either out-of-range direction. `!(a - 1)` always resolves to index 1 (the max face) whenever a == 1, so when the query point is below the box's min bound on an axis, distance is measured to the wrong (max) face instead of the min face, inflating dmin and producing false-negative intersection results. The above-max direction happens to pick the correct face already, which is why this went unnoticed since the index-selection logic was introduced in recp#180 (fix for recp#179). Fix: index directly with the "above max" boolean (s[i] > box[1][i]) instead of the derived !(a - 1) expression -- it's 0 for below-min (selects box[0], correct) and 1 for above-max (selects box[1], correct), and is already computed as part of `a`/`b`/`c` so no extra branching is introduced. Same fix applied to both the 3D (box.h) and 2D (aabb2d.h) variants since they share the identical pattern. Verified against a hand-written Ericson/Graphics-Gems reference point-AABB distance test: 200k randomized box/sphere fuzz cases show 3418/200000 false negatives before this fix and 0/200000 after, with no change to the already-correct above-max and fully-enclosed cases.
Owner
|
@94xhn the PR is merged, thanks for your contributions 🚀 |
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
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.
Problem
glm_aabb_sphere()(include/cglm/box.h) andglm_aabb2d_circle()(include/cglm/aabb2d.h) compute the closest point on an AABB to a sphere/circle center per axis using:ais1for either out-of-range direction (belowbox[0]or abovebox[1]), and!(a - 1)collapses to index1(the max face) whenevera == 1, regardless of which side the point is actually on. So when the query point is below the box's min bound on an axis, the code clamps to the max face instead of the min face, producing an incorrect (too-large) distance-squared contribution and false-negative intersection results.Fix
Use the already-computed "above max" boolean directly as the face index instead of the
!(a - 1)derivation:This selects
box[0](min face) when the point is below the min bound andbox[1](max face) when above the max bound. The in-range case is unaffected — it's already zeroed out by the existing(a != 0)multiplier, so the index value there doesn't matter.Same one-line-per-axis change applied identically to both
glm_aabb_sphere(3 axes) andglm_aabb2d_circle(2 axes), since they share the exact same root cause.Verification
box[0]on one axis, below on two axes simultaneously (3D), etc. — all mismatched (false negative) before the fix, correct after.cglm.hstill compiles clean with-Wall -Wextra.Diff is minimal: 2 files, 5 insertions / 5 deletions, no behavior change to the in-range or above-max cases.