Skip to content

Fix specialized shape to shape min distance update and enable_signed_distance flag - #885

Merged
jorisv merged 6 commits into
coal-library:develfrom
mjd3:fix/shape-shape-distance-min-update
Nov 4, 2026
Merged

jorisv merged 6 commits into
coal-library:develfrom
mjd3:fix/shape-shape-distance-min-update

Conversation

@mjd3

@mjd3 mjd3 commented Aug 24, 2026 •

Copy link
Copy Markdown
Contributor

Fixes two issues:

  • The SHAPE_SHAPE_DISTANCE_SPECIALIZATION / SHAPE_SELF_DISTANCE_SPECIALIZATION wrappers assigned result.min_distance (and o1/o2/nearest_points/normal) unconditionally, while the generic ShapeShapeDistancer path checks request.isSatisfied and merges through DistanceResult::update. When a DistanceResult is shared across several pairs (e.g., broadphase checks) a specialized pair evaluated after a closer pair overwrote the minimum, so the query returned the last evaluated pair instead of the closest one. Fix is to merge with DistanceResult::update, like the generic path does; added regression test for this behavior as well.
  • Analytic distance functions in src/distance ignore the compute_signed_distance argument, so with DistanceRequest::enable_signed_distance = false a specialized pair could still report a negative min_distance (different from documentation/GJK path). Fix is to clamp the result at zero; the witness points and normal are unchanged.

@mjd3

mjd3 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@jorisv or @nim65s let me know if there's anything else you need for this PR; would love to get this PR in eventually since it fixes a pretty major issue with broadphase distance checks in my current project.

@pgraverdy
pgraverdy requested a review from lmontaut September 14, 2026 09:38

@jorisv jorisv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. You can add yourself in the credit section and use rebase instead of merge commit to synchronize your development.

Comment thread test/broadphase_dynamic_AABB_tree.cpp Resolved
Comment thread test/broadphase_dynamic_AABB_tree.cpp Resolved
Comment thread test/broadphase_dynamic_AABB_tree.cpp Resolved
@mjd3
mjd3 force-pushed the fix/shape-shape-distance-min-update branch from 2016855 to 3984d77 Compare October 28, 2026 16:36
@mjd3
mjd3 requested a review from jorisv October 28, 2026 16:39
@mjd3

mjd3 commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

@jorisv addressed your feedback here; let me know if you have any other comments.

Comment thread test/distance.cpp Resolved
@mjd3
mjd3 requested a review from jorisv November 3, 2026 18:54
@mjd3
mjd3 force-pushed the fix/shape-shape-distance-min-update branch from f972cd1 to 08a1087 Compare November 3, 2026 19:10
jorisv
jorisv previously approved these changes Nov 4, 2026
@jorisv

jorisv commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

@mjd3 Thanks, the PR is approved, it will be merged soon.

@jorisv

jorisv commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Apparently your patch fix an unit test that was failing for a long time.

@jorisv
jorisv force-pushed the fix/shape-shape-distance-min-update branch from 09d2205 to 8d2b94a Compare November 4, 2026 12:37
@jorisv
jorisv merged commit 3e18bd5 into coal-library:devel Nov 4, 2026
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants