Skip to content

Physics System populates link bounding boxes #2821

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
Draft
wants to merge 13 commits into
base: gz-sim9
Choose a base branch
from
Draft

Physics System populates link bounding boxes #2821

wants to merge 13 commits into from

Conversation

gabrielfpacheco
Copy link
Contributor

@gabrielfpacheco gabrielfpacheco commented Mar 12, 2025 •
edited
Loading

🎉 New feature

Follow-up of #2787

Summary

Computes the link bounding boxes for all link entities that have components::AxisAlignedBox present. The computation follows the same logic as the models bbox, but falls back to the SDF calculation introduced in #2787 in case the feature is not present for the selected physics plugin.

Test it

Run the following integration test:

./build/gz-sim9/bin/INTEGRATION_physics_system --gtest_filter=*LinkBoundingBox*

It will load bounding_boxes.sdf world (see below) and perform link AABB checks while the models free-fall until reaching the ground.

image

Note: after the previous PR is merged, this PR will rebase to gz-sim9 and be marked as ready to review

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Mar 12, 2025
@gabrielfpacheco gabrielfpacheco mentioned this pull request Mar 12, 2025
Open
9 tasks
iche033
iche033 reviewed Mar 14, 2025
View reviewed changes
Copy link
Contributor

@iche033 iche033 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

did a first pass review, overall approach looks good

@iche033 iche033 added the needs upstream release Blocked by a release of an upstream library label Mar 14, 2025
@gabrielfpacheco gabrielfpacheco force-pushed the gp-link_aabb_physics_system branch from 1fed07c to 0595ea9 Compare March 14, 2025 20:52
@gabrielfpacheco
Copy link
Contributor Author

@iche033, I believe I addressed all your comments. If you agree with the proposed changes, could you please approve #2787 before? I would like to rebase this PR after the previous one is merged so that it focuses only on the physics system changes.

@iche033
Copy link
Contributor

iche033 commented Mar 26, 2025

@iche033, I believe I addressed all your comments. If you agree with the proposed changes, could you please approve #2787 before? I would like to rebase this PR after the previous one is merged so that it focuses only on the physics system changes.

Currently there's a build error as it needs the new API from sdformat. So I'm waiting for gazebosim/sdformat#1547 to be merged first. We also need a new sdformat release afterwards in order to run CI in #2787 and make sure everything is green. We can only approve and merge a PR when the required CI checks are green.

@gabrielfpacheco
Copy link
Contributor Author

So I'm waiting for gazebosim/sdformat#1547 to be merged first. We also need a new sdformat release afterwards in order to run CI in #2787 and make sure everything is green.

Yes, sure! I initially thought there might be a way to trigger the CI with dependent packages at specific branches, but then realized that the merge/release process is required, as you said.

Besides the dependency that prevents the proper CI checks, though, there are still some unresolved threads here from your first pass review, are those solved?

Regarding the sdformat PR, the CI is already green, and I believe all comments have been addressed. I've pinged the reviewers in gazebosim/sdformat#1547 (comment) recently, so I think we should be close to a merge.

iche033
iche033 reviewed Mar 31, 2025
View reviewed changes
@iche033
Copy link
Contributor

iche033 commented Mar 31, 2025

Besides the dependency that prevents the proper CI checks, though, there are still some unresolved threads here from your first pass review, are those solved?

yep looks good to me. Feel free to mark them as resolved once you address the comments.

@gabrielfpacheco gabrielfpacheco force-pushed the gp-link_aabb_physics_system branch from 270e71e to b5d6982 Compare April 1, 2025 20:51
@gabrielfpacheco gabrielfpacheco mentioned this pull request May 8, 2025
Merged
9 tasks
@gabrielfpacheco
Add support to get link AABB from its collisions
6f8b1d8 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Add python bindings to new methods and corresponding tests
8d42ecf 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Fix and improve new methods API documentation
e808e18 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Include missing used headers
07e7264 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Update axis-aligned box computation to better fit overall architecture
3fd29ce 
* Following gazebosim#2353 (comment)
* This allows to use the ECM as cache if bounding box doesn't have to be
recomputed and is more suitable so that other systems can determine the
AABB if necessary (e.g. physics system if checks are enabled)

Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Make link AABB computation from collision shapes public
bed6040 
This could be helpful if users want to explicetly compute the AABB from
the link SDF collision shapes. For instance, the Physics system can
fallback and use this method in case the selected engine does not support
AABB calculation.

Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Add python bindings to new public method
6ec09e0 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Fix mesh AABB calculation to take scale factor into account
8a29e21 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Remove unnecessary gz::sim and sim:: namespaces
e3fdde0 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Return std::optional to indicate mesh AABB cannot be computed
5b88d09 
* Also fix `_sdfMesh` to be `const &`

Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco
Fix spelling error flagged by codespell
91547ab 
Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco gabrielfpacheco force-pushed the gp-link_aabb_physics_system branch from b5d6982 to fa9726a Compare May 30, 2025 03:27
gabrielfpacheco and others added 2 commits May 30, 2025 08:36
@gabrielfpacheco gabrielfpacheco force-pushed the gp-link_aabb_physics_system branch from fa9726a to adfee80 Compare May 30, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iche033 iche033 iche033 left review comments

@azeey azeey Awaiting requested review from azeey azeey will be requested when the pull request is marked ready for review azeey is a code owner

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll will be requested when the pull request is marked ready for review mjcarroll is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
🏛️ ionic Gazebo Ionic needs upstream release Blocked by a release of an upstream library
Projects
Core development
Status: Inbox
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gabrielfpacheco @iche033