search

OctoMap / octomap_msgs

ROS package to provide messages and serializations / conversion for the OctoMap library
http://www.ros.org/wiki/octomap_msgs
32 stars 53 forks source link

Clarification: why force binaryMsg to only accept OcTrees? #4

Closed wxmerkt closed 8 years ago

wxmerkt commented 9 years ago

Looking at https://github.com/OctoMap/octomap_msgs/blob/indigo-devel/include/octomap_msgs/conversions.h#L74, why does this have to be an OcTree? Can't we also have it be an AbstractOcTree similar to https://github.com/OctoMap/octomap_msgs/blob/indigo-devel/include/octomap_msgs/conversions.h#L55?

If there is no particular reason why this has to be, I am very happy to make a PR changing this to the more general AbstractOcTree (to then support e.g. ColorOcTrees).

ahornung commented 9 years ago

The binary format (.bt files in OctoMap) can only store binary information for a leaf: free or occupied. Therefor, you cannot store color or other information, which is why only the OcTree class makes sense. The other format (comparable to .ot files) is more like general container and can contain any information.

It may be possible to have different occupancy octrees in the binary format and somehow store or "invent" the missing information. But for the format to be used in a general "factory pattern" way there's still functionality missing from the OctoMap API itself, as described in https://github.com/OctoMap/octomap/issues/1

wxmerkt commented 9 years ago

Thank you very much for the clarification and update :)

wxmerkt commented 9 years ago

One more follow up question regarding this - when taking any OcTree and creating a binary message, the id/type is still specified as what the original OcTree was - e.g. it will say "ColorOcTree" even if the binary message only contains non-color information, cf. https://github.com/OctoMap/octomap_msgs/blob/master/include/octomap_msgs/conversions.h#L151. Should the id here perhaps be hard-coded to be "OcTree"?

ahornung commented 9 years ago

I'm not really sure what the best or correct behavior would be here...

In theory there could be other occupancy octree classes that validly encode back and forth to a binary msg so I thought it might be a good idea to preserve the original class just in case. I think the final implementation depends on how it'll be implemented in OctoMap/octomap#1.

lsouchet commented 9 years ago

Hello, I encounter the same issue with OctreeStamped. From what I understand, the check for "Octree" id here: https://github.com/OctoMap/octomap_msgs/blob/indigo-devel/include/octomap_msgs/conversions.h#L74 is irrelevant. It prevents using derived octree types and in case of a malformed octree binary buffer, I believe the check could be done in a lower level. Regarding the final implementation in https://github.com/OctoMap/octomap/issues/1, is anybody working on it?

ahornung commented 9 years ago

Alright, that check is now gone in indigo-devel.

There is currently no one working on OctoMap/octomap#1, so if you have an idea feel free to contribute!

felixendres commented 8 years ago

FYI: Fix is not included in the current indigo .deb, i.e. 0.3.2-0trusty-20160419-181158-0700

ahornung commented 8 years ago

Good catch! That needs a new release, pending #6

ahornung commented 8 years ago

New release on the way.