Fix GeometryModel::collisionPairMapping not updated on removeGeometryObject - #2937
Open
benjiiDELPECH wants to merge 3 commits into
Open
Fix GeometryModel::collisionPairMapping not updated on removeGeometryObject#2937benjiiDELPECH wants to merge 3 commits into
benjiiDELPECH wants to merge 3 commits into
Conversation
…Object removeGeometryObject() correctly removed and reindexed collisionPairs when a GeometryObject was deleted, but left collisionPairMapping untouched: wrong size (still ngeoms+1) and stale pair indices that could point past the end of the shrunk collisionPairs vector. Rebuild collisionPairMapping in the same pass that already walks collisionPairs to erase/reindex the surviving pairs, so it stays in sync with both the new object count and the renumbered pair list. Add regression coverage in test_simple_boxes checking that collisionPairMapping.rows()/cols() match ngeoms after removal. Fixes stack-of-tasks#2846
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
👋 Hi,
This is a reminder message to assign an extra build label to this Pull Request if needed.
By default, this PR will be build with minimal build options (URDF support and Python bindings)
The possible extra labels are:
- build_collision (build Pinocchio with coal support)
- build_casadi (build Pinocchio with CasADi support)
- build_autodiff (build Pinocchio with CppAD support)
- build_codegen (build Pinocchio with CppADCodeGen support)
- build_extra (build Pinocchio with extra algorithms)
- build_mpfr (build Pinocchio with Boost.Multiprecision support)
- build_sdf (build Pinocchio with SDF parser)
- build_accelerate (build Pinocchio with APPLE Accelerate framework support)
- build_all (build Pinocchio with ALL the options stated above)
Thanks.
The Pinocchio development team.
4 tasks
jcarpent
requested changes
Aug 22, 2026
jcarpent
left a comment
jcarpent
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me overall
Thanks @benjiiDELPECH for this nice PR.
Could you add a test to check the content of collisionPairMapping?
Best,
Justin
Author
|
Done, thanks for the review! |
Requested by @jcarpent: the existing regression check only compared collisionPairMapping.rows()/cols() to ngeoms, which passes even if the matrix content is wrong. Assert both symmetric entries of the surviving pair and the still-empty diagonal after removal.
benjiiDELPECH
force-pushed
the
fix/2846-collision-pair-mapping
branch
from
August 23, 2026 00:01
7e9b470 to
583f4c4
Compare
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.
Summary
GeometryModel::removeGeometryObjectcorrectly removed and reindexedcollisionPairswhen aGeometryObjectwas deleted, but leftcollisionPairMappinguntouched — wrong size (stillngeoms+1) and stale pair indices that could point past the end of the shrunkcollisionPairsvector.This rebuilds
collisionPairMappingin the same pass that already walkscollisionPairsto erase/reindex the surviving pairs, so it stays in sync with both the new object count and the renumbered pair list — no extra pass needed.Closes #2846.
Test plan
BOOST_CHECK_EQUALoncollisionPairMapping.rows()/cols()vsngeomsintest_simple_boxes, after the existingremoveGeometryObjectcall.3 != 2) and pass with it — confirmed locally viactest -R pinocchio-test-cpp-geometry-algorithms.