Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wxmerkt opened this issue Jul 11, 2015 · 9 comments
Closed

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

wxmerkt opened this issue Jul 11, 2015 · 9 comments
Labels

Comments

@wxmerkt
Copy link
Member

wxmerkt commented Jul 11, 2015

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
Copy link
Member

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 OctoMap/octomap#1

@wxmerkt
Copy link
Member Author

wxmerkt commented Jul 12, 2015

Thank you very much for the clarification and update :)

@wxmerkt wxmerkt closed this as completed Jul 12, 2015
@wxmerkt
Copy link
Member Author

wxmerkt commented Jul 15, 2015

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"?

@wxmerkt wxmerkt reopened this Jul 15, 2015
@ahornung
Copy link
Member

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
Copy link

lsouchet commented Aug 7, 2015

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 OctoMap/octomap#1, is anybody working on it?

@ahornung
Copy link
Member

ahornung commented Aug 9, 2015

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
Copy link
Contributor

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

@ahornung
Copy link
Member

ahornung commented Jun 9, 2016

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

@ahornung
Copy link
Member

New release on the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants