Move Aabb and Obb out of internal namespace #23120
Conversation
There was a problem hiding this comment.
+@SeanCurtis-TRI what do you think? I'll add python bindings if you think it's not a bad idea.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @xuchenhan-tri)
There was a problem hiding this comment.
I'm in favor of this. It also unlocks the ability to report the bounding boxes of various objects. I'd say forge ahead.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @xuchenhan-tri)
geometry/proximity/aabb.h line 159 at r1 (raw file):
/** %AabbMaker implements the logic to fit an Aabb to a collection of points. The points are the position of a subset of verties in a mesh. The Aabb will
nit: while we're here?
Suggestion:
vertices
There was a problem hiding this comment.
Python bindings add, PTAL.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)
geometry/proximity/aabb.h line 159 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: while we're here?
Done
There was a problem hiding this comment.
Two questions about bindings; see below.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)
bindings/pydrake/geometry/geometry_py_common.cc line 661 at r2 (raw file):
} void DefineBoundingBoxes(py::module m) {
BTW Hypothetical; given that these bounding boxes are so inherently tied in with meshes, should this definition possibly be in geometry_py_meshes.cc?
If the idea seems a bridge too far, perhaps geometry_py_bounding_box.cc (a new file). That could also eventually be a home to BVH.
But at the end of the day, common doesn't strike me as quite the right place.
bindings/pydrake/geometry/test/common_test.py line 752 at r2 (raw file):
# Test volume calculation. expected_volume = 8 * half_width[0] * half_width[1] * half_width[2] self.assertAlmostEqual(aabb.CalcVolume(), expected_volume, 1e-13)
BTW I don't think we have a clear tradition in writing pybind tests. And my particular attitude may differ from day to day. But, given the nature of the bindings, these tests could be simpler.
Arguably, for a trivial binding, all one really needs to do is confirm that it is bound with the right signature and an indication that right kind of thing is returned. So, for example,
self.assertIsInstance(aabb.CalcVolume(), float)`would be sufficient. How far the idea gets taken and the impact on the test, really comes down to judgment.
I'll defer to you if it's worth changing the tests to make it simpler.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)
bindings/pydrake/geometry/geometry_py_common.cc line 661 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Hypothetical; given that these bounding boxes are so inherently tied in with meshes, should this definition possibly be in
geometry_py_meshes.cc?If the idea seems a bridge too far, perhaps
geometry_py_bounding_box.cc(a new file). That could also eventually be a home to BVH.But at the end of the day,
commondoesn't strike me as quite the right place.
Done.
bindings/pydrake/geometry/test/common_test.py line 752 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I don't think we have a clear tradition in writing pybind tests. And my particular attitude may differ from day to day. But, given the nature of the bindings, these tests could be simpler.
Arguably, for a trivial binding, all one really needs to do is confirm that it is bound with the right signature and an indication that right kind of thing is returned. So, for example,
self.assertIsInstance(aabb.CalcVolume(), float)`would be sufficient. How far the idea gets taken and the impact on the test, really comes down to judgment.
I'll defer to you if it's worth changing the tests to make it simpler.
I see your point, but I think the tests here are fine -- the extra computation is trivial and it adds slightly more confidence in the binding (i.e. it's not bound to some other method with the same return type).
There was a problem hiding this comment.
+@sammy-tri for Monday review, please.
Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)
bindings/pydrake/geometry/test/common_test.py line 752 at r2 (raw file):
Previously, xuchenhan-tri wrote…
I see your point, but I think the tests here are fine -- the extra computation is trivial and it adds slightly more confidence in the binding (i.e. it's not bound to some other method with the same return type).
Accepted.
bindings/pydrake/geometry/geometry_py.h line 44 at r3 (raw file):
void DefineGeometryRefine(py::module m); /** Define the bounding box functions. See geometry_py_bounding_box.cc. */
nit: Alphabetical order in declaration here.
bindings/pydrake/geometry/geometry_py.cc line 34 at r3 (raw file):
DefineGeometryOptimization(m.def_submodule("optimization")); DefineGeometryVisualizers(m); DefineGeometryBoundingBox(m);
nit: You're going to want to move this just below meshes. Order here matters.
Towards #15121
This change is