search

moveit / geometric_shapes

Representation of geometric shapes
57 stars 92 forks source link

[noetic] Added bodies::Body::computeBoundingBox (oriented box version) #210

Closed peci1 closed 2 years ago

peci1 commented 2 years ago

This is a noetic version of #105. I basically just cherry-picked the commits from #105 and resolved merge conflicts.

I also figured out this PR breaks ABI by adding a virtual function, so I again made it fake virtual. It's a pity it will be such in both noetic and melodic, but there's not much more that can be done.

I also made use of the fact that FCL 0.6 is available in noetic and relayed AABB computation in bodies::AABB::extendWithTransformedBox() directly to FCL. This change is a bit off-topic, but does not hurt and offers to get potential future improvements from FCL. I made this change in a separate commit, so if it is not desired, I can get rid of it.

peci1 commented 2 years ago

The FCL search logic in CMake might look complicated, but that's what I had to do in https://github.com/peci1/robot_body_filter/commit/db08750759d5dd93822451a7c2981368c6bd187b#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a in order to get it correctly working on Melodic and Noetic on both Ubuntu and Debian.

peci1 commented 2 years ago

I would even drop ABI compatibility

Should I remove the ABI compatibility commit, then?

rhaschke commented 2 years ago

Should I remove the ABI compatibility commit, then?

Yes, please.

rhaschke commented 2 years ago

@ros-planning/moveit-maintainers, this is awaiting the 2nd approval for more than a month...

simonschmeisser commented 2 years ago

Do we actually intend to backport this to melodic-devel? If not it could be simplified a lot by dropping support for fcl 0.5 ... or maybe not as the libfcl package in Ubuntu Focal is still 0.5.0 while the ros-noetic-fcl is at 0.6.1

peci1 commented 2 years ago

I saw that versions of FCL can range from 0.5 to 0.6 even on newer systems depending on many things, so I'd rather keep it compatible with both. And yes, I'd like to backport this to melodic.

simonschmeisser commented 2 years ago

Looks good now!

Do you want to squash the second commit into the first before merging?

peci1 commented 2 years ago

Do you want to squash the second commit into the first before merging?

Do you mean to not have a commit where build is broken?

simonschmeisser commented 2 years ago

Do you want to squash the second commit into the first before merging?

Do you mean to not have a commit where build is broken?

Yes and because the commit is somewhat "boring" :wink: and looks like a fixup

peci1 commented 2 years ago

Okay, squashed ;)

peci1 commented 2 years ago

Great, thanks! Now there's #105 which could be used for the melodic backport. Should I help with it somehow?