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

[peci1]

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 bc1cf5e - 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]

@peci1 As this just came up again in PR2/robot_self_filter#23 (comment) ,
feel free to provide a melodic backport.

Originally posted by @v4hn in #138 (comment)

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

[v4hn]

... That PR ended up in the backlog (as many others...)

I believe we bump the library SONAME in this repository, so ABI breakage should not be a concern and I would be fine with simplifying your logic again to a virtual interface.

That aside I approve.
@rhaschke, you were the second review for the original PR, so maybe you can spare a moment?

[peci1]

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 68e6740 which is not in this PR. I think it should be included.

I've also inspected your additional commit bc1cf5e 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]

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 #225 should avoid the vtable lookup.

[rhaschke]

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

[peci1]

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]

Thanks!