Skip to content

Add AxisAlignedBox getters for all relevant geometries #1547

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.

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

Merged
merged 5 commits into from
May 2, 2025

Conversation

gabrielfpacheco
Copy link
Contributor

@gabrielfpacheco gabrielfpacheco commented Feb 19, 2025

🎉 New feature

Part of gazebosim/gz-sim#2787

Summary

This PR adds AxisAlignedBox support for SDF geometries. Now each volumetric geometry type such as Box, Capsule, Cone, Cylinder, Ellipsoid, Sphere, and Mesh has a new AxisAlignedBox() function that computes its bounding box from the geometry dimensions. The Geometry class uses these functions to determine the correct AABB, allowing a custom calculator for meshes , which is a special case since no mesh loading is performed in the sdf::Mesh class.

Test it

  • Checkout the appropriate sdformat branch
  • Build this package
$ colcon build --packages-select sdformat15 --merge-install
  • Run the following sdformat15 tests:
$ ./build/sdformat15/bin/UNIT_Box_TEST && \
./build/sdformat15/bin/UNIT_Capsule_TEST && \
./build/sdformat15/bin/UNIT_Cone_TEST && \
./build/sdformat15/bin/UNIT_Cylinder_TEST && \
./build/sdformat15/bin/UNIT_Ellipsoid_TEST && \
./build/sdformat15/bin/UNIT_Sphere_TEST && \
./build/sdformat15/bin/UNIT_Mesh_TEST && \
./build/sdformat15/bin/UNIT_Geometry_TEST
  • Run all package tests to veify everything is still passing:
$ colcon test --packages-select sdformat15 --merge-install

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.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

These new methods should also be included in the python API (tests too), if you don't the time to do it, please open an issue about adding them

@gabrielfpacheco
Copy link
Contributor Author

These new methods should also be included in the python API (tests too), if you don't the time to do it, please open an issue about adding them

I'm on it already

@gabrielfpacheco gabrielfpacheco force-pushed the gp-geometry_aligned_box branch 2 times, most recently from caa3799 to 2ef02d4 Compare February 20, 2025 15:47
@scpeters
Copy link
Member

scpeters commented Mar 4, 2025

it's reasonable to add gz::math::AxisAlignedBox accessors to the sdf Shape APIs. I'm going to open a feature request to move the conversion logic to gz-math, since I think it would be more broadly useful there. Something like gz/math/AxisAlignedBoxHelpers.hh with static functions like:

static gz::math::AxisAlignedBox ConvertToAxisAlignedBox(const gz::math::Box &);
static gz::math::AxisAlignedBox ConvertToAxisAlignedBox(const gz::math::Sphere &);
static gz::math::AxisAlignedBox ConvertToAxisAlignedBox(const gz::math::Capsule &);
etc.

The implementation here could then be refactored to use those helper functions, but we don't need to block this PR for that

@scpeters
Copy link
Member

scpeters commented Mar 4, 2025

it's reasonable to add gz::math::AxisAlignedBox accessors to the sdf Shape APIs. I'm going to open a feature request to move the conversion logic to gz-math, since I think it would be more broadly useful there.

The implementation here could then be refactored to use those helper functions, but we don't need to block this PR for that

gazebosim/gz-math#666

@gabrielfpacheco gabrielfpacheco force-pushed the gp-geometry_aligned_box branch from 9ac3e02 to 529d2e6 Compare March 10, 2025 13:37
@gabrielfpacheco
Copy link
Contributor Author

The implementation here could then be refactored to use those helper functions, but we don't need to block this PR for that

I saw that gazebosim/gz-math#667 has been created. However, I’d prefer not to introduce a dependency here if possible. That said, I’d be happy to create a new PR using the new APIs once both PRs are merged. Does that sound good to you, @scpeters?

@gabrielfpacheco gabrielfpacheco force-pushed the gp-geometry_aligned_box branch 2 times, most recently from 575234c to 4850432 Compare March 14, 2025 20:39
@gabrielfpacheco
Copy link
Contributor Author

@scpeters @ahcorde, is there anything still missing here?

@gabrielfpacheco
Copy link
Contributor Author

@scpeters, is this ready to merge?

@gabrielfpacheco
Copy link
Contributor Author

Hi! Sorry if I'm being a bit pushy here, I just wanted to check if there's anything preventing this PR from being merged. I'd be more than happy to address any required changes. This is currently blocking a couple of other gz-sim PRs that would be really helpful on our end. Thanks a lot!

@gabrielfpacheco gabrielfpacheco force-pushed the gp-geometry_aligned_box branch from fc7bab1 to 5ad4f74 Compare April 11, 2025 15:16
@gabrielfpacheco gabrielfpacheco force-pushed the gp-geometry_aligned_box branch from 5ad4f74 to 13c13c6 Compare April 29, 2025 17:19
@gabrielfpacheco
Copy link
Contributor Author

ping @azeey, @scpeters

* Mesh is a special case since it forwards the calculations for
the caller. If callback is not set, the return is nullptr
* This has been done since this library does not depend on gz-common
* Geometry types that do not have a volume, return nullptr.

Signed-off-by: Gabriel Pacheco <[email protected]>
@gabrielfpacheco gabrielfpacheco force-pushed the gp-geometry_aligned_box branch from 13c13c6 to 6f3f738 Compare May 2, 2025 17:42
@scpeters scpeters merged commit e073ffa into gazebosim:sdf15 May 2, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Core development May 2, 2025
@scpeters
Copy link
Member

scpeters commented May 2, 2025

thanks for your contribution and patience with our review!

@gabrielfpacheco
Copy link
Contributor Author

My pleasure, I'm happy to contribute! Totally understand that reviews can take time since we all have busy schedules, no need to worry :)

By the way, would it be possible to backport this to Harmonic?

@gabrielfpacheco gabrielfpacheco deleted the gp-geometry_aligned_box branch May 2, 2025 19:54
@gabrielfpacheco
Copy link
Contributor Author

gabrielfpacheco commented May 8, 2025

By the way, would it be possible to backport this to Harmonic?

Also, are there any plans for the next release? This will be necessary to continue the related work in gazebosim/gz-sim#2787 and gazebosim/gz-sim#2821

@scpeters
Copy link
Member

By the way, would it be possible to backport this to Harmonic?

Also, are there any plans for the next release? This will be necessary to continue the related work in gazebosim/gz-sim#2787 and gazebosim/gz-sim#2821

sorry for the delay; this was included in the 15.3.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants