Skip to content

Fix segfault in FrameCollisionResidual, add corresponding test in test_frames.py #294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Apr 26, 2025
Merged

Fix segfault in FrameCollisionResidual, add corresponding test in test_frames.py #294

merged 6 commits into from
Apr 26, 2025

Conversation

oomcth
Copy link
Contributor

@oomcth oomcth commented Apr 26, 2025 •
edited by ManifoldFR
Loading

#289 (comment)

Resolves #289.

Problem description

When FrameCollisionResidual is initialized with a geometry that has no collision pairs, we get a segfault.

@ManifoldFR ManifoldFR changed the base branch from main to devel April 26, 2025 11:58
@ManifoldFR ManifoldFR requested a review from edantec April 26, 2025 15:13
@ManifoldFR ManifoldFR self-assigned this Apr 26, 2025
@ManifoldFR ManifoldFR changed the title Added a test in 'test_frames.py' to debug a segfault occurring when 'FrameCollisionResidual' is initialized with a geometry that has no collision pairs. Fix segfault in FrameCollisionResidual, add corresponding test in test_frames.py Apr 26, 2025
@ManifoldFR
[tests/python] change instances of "hppfcl" to "coal"
4151dde 
+ use pin.buildSampleGeometryModelHumanoid()
+ rename test_FrameCollisionResidual_no_collision_pairs() to test_frame_collision_no_collision_pairs()
+ remove unused local vars
@ManifoldFR
frame-collision.hpp : change typedefs and some member data names in F…
538b4ac 
…rameCollisionData
@ManifoldFR ManifoldFR force-pushed the main branch 2 times, most recently from c1bb908 to 603cee7 Compare April 26, 2025 16:11
@ManifoldFR
frame-collision.hpp : throw out of range error
821524b 
tests/python/test_frames.py : ask for the corresponding error type
@ManifoldFR
Copy link
Member

Okay, so this is handled by throwing an std::out_of_range exception now. Underlying issue is that the class' constructor queries the element in slot frame_pair_id of the GeometryModel::collisionPairs... which was empty here, but in general, it might just be an invalid index.

@edantec maybe down the line we could change the ctor to instead take the CollisionPair and just perform a geom.existCollisionPair() check (without adding, since GeometryModel is passed as const*) ? It might be better semantically.

*We could modify the internal copy of GeometryModel to add the desired pair. But then again, maybe it shouldn't be a copy... but a reference or pointer...

@ManifoldFR ManifoldFR enabled auto-merge April 26, 2025 16:20
ManifoldFR
ManifoldFR approved these changes Apr 26, 2025
View reviewed changes
Copy link
Member

@ManifoldFR ManifoldFR left a comment

Choose a reason for hiding this comment

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

lgtm (i wrote it)

@ManifoldFR ManifoldFR merged commit 00a7e8d into Simple-Robotics:devel Apr 26, 2025
17 checks passed
@ManifoldFR ManifoldFR mentioned this pull request Apr 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ManifoldFR ManifoldFR ManifoldFR approved these changes

@edantec edantec Awaiting requested review from edantec

Assignees

@ManifoldFR ManifoldFR

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oomcth @ManifoldFR