search

moveit / geometric_shapes

Representation of geometric shapes
57 stars 92 forks source link

[melodic backport] Added body operations constructShapeFromBody() and constructMarkerFromBody(). #209

Closed peci1 closed 2 years ago

peci1 commented 2 years ago

This is a backport of #138 to melodic. All tests pass on my side in both debug and release modes. I used the fake non-virtual polymorphy for Body::getScaledDimensions() to retain feature parity with noetic and keep melodic ABI unchanged.

I added a special test case for calling constructShapeFromBody() on a bodies::Body pointer rather than on one of the downstream types. I can port this test to the noetic branch, too, to test that the polymorphic behavior is doing what it is expected to do. I've actually noticed that neither dynamic, nor static casts are needed in the places edited by commit bc1cf5ea43f6b8d09cfd5d53f68f418cdae9defd - as getScaledDimensions() is polymorphic (noetic) or fake polymorphic (melodic), calling it directly on a Body instance should be good enough. I also did this simplification in this PR, and can transfer it to noetic-devel, too (but it's just a cosmetic issue).

Accepting this backport would allow me to drop a #define private public hack from https://github.com/peci1/robot_body_filter/blob/master/src/utils/bodies.cpp (and would also allow me to drop constructShapeFromBody() and constructMarkerFromBody() implementations in robot_body_filter and use the geometric_shapes versions).

peci1 commented 2 years ago

@peci1 As this just came up again in https://github.com/PR2/robot_self_filter/issues/23#issuecomment-674032618 , feel free to provide a melodic backport.

_Originally posted by @v4hn in https://github.com/ros-planning/geometric_shapes/issues/138#issuecomment-674088013_

@v4hn could you please have a look at this until Melodic times out? :)

peci1 commented 2 years ago

Thanks for the review! I would definitely love to drop the ABI workaround.

I was just checking #138 and found out that Robert has added commit https://github.com/ros-planning/geometric_shapes/pull/138/commits/68e67408fc827ec04f7fefb49c84fee246b801c1 which is not in this PR. I think it should be included.

I've also inspected your additional commit https://github.com/ros-planning/geometric_shapes/pull/138/commits/bc1cf5ea43f6b8d09cfd5d53f68f418cdae9defd which has turned dynamic_casts into static_casts in constructShapeFromBody(). However, I'm now not sure why a cast is needed at all in this case. Function getScaledDimensions() is virtual in #138, so it should not be needed to downcast to particular body types to call it. But I don't remember well if there wasn't some other reason. I guess it would not even save a vtable lookup as even the downcasted functions are virtual.

rhaschke commented 2 years ago

Function getScaledDimensions() is virtual in #138, so it should not be needed to downcast to particular body types to call it.

I agree.

I guess it would not even save a vtable lookup as even the downcasted functions are virtual.

Not yet. But https://github.com/ros-planning/geometric_shapes/pull/225 should avoid the vtable lookup.

rhaschke commented 2 years ago

I agree with Michael, that we could drop the ABI compatibility.

peci1 commented 2 years ago

I've removed the ABI compatibility commit and added the two additional commits from #138. I'd wait for #225 to be merged, cherry-pick the commit here, and then this PR could be ready. What do you think?

peci1 commented 2 years ago

Thanks!