API changes to support Octomap 1.8

[jvgomez]

At the end it was not that much :)

Looking forward to your comments.

@gauthamm FYI.

[jvgomez]

I see tests are failing because the CI is not using the latest octomap. I am not a CI expert. I guess I have to change the .travis YAML files. Is there any way I can easily test without doing tons of commits to this PR? (100% tests succeed in my computer).

[jmirabel]

For linux, you can just change line

git checkout tags/v1.6.8

of ci/install_linux.sh

[jslee02]

The overall looks good to me. I have some minor suggestions.

1. inline wouldn't be necessary for the member functions defined in class definition because the inline is implicitly specified.

2. For const correctness, it would make sense to drop const specifier of the first added function as:

   inline OcTreeNode* getNodeChild(OcTreeNode* node, unsigned int childIdx)

3. The tap used for indentation in nodeHasChildren should be replaced by spaces.

4. It would be nicer if fcl can be compiled with both of 1.6.8 and 1.8.0 for backward compatibility using some preprocessors with octomap versions something like:

   #if OCTOMAP_VERSION_LESS_THAN(1,8,0)
       if(!root1->hasChildren())
   #else
       if(!tree1->nodeHasChildren(root1))
   #endif
         {
           if(tree1->isNodeOccupied(root1))
           {

[jvgomez]

Ok, we will address the comments in the following days :)

[jvgomez]

Most of the comments have been addressed (I have removed some other inline in the header file). However, regarding point 4 I am having some difficulties:

• A macro like the one proposed cannot be used, as Octomap does not define these values in code. I could do it with some definitions in the CMakeLists.txt. Or just a very simple definition and use it direclt. Or I could directly template the headers and configure them with CMake.
• As an alternative, I am trying to come up with a method to use SFINAE to figure out which function should be called.

[jslee02]

As octomap provides octomap-config-version.cmake, we could get the version string if octomap is found using find_package instead of pkg-config. We might need some additional work to extract the version number components.

Here is a possible way to do it:

# Find Octomap (optional)
find_package(Octomap)
if (OCTOMAP_FOUND OR (OCTOMAP_INCLUDE_DIRS AND OCTOMAP_LIBRARY_DIRS))
  string(REPLACE "." ";" VERSION_LIST ${OCTOMAP_VERSION})
  list(GET VERSION_LIST 0 OCTOMAP_MAJOR_VERSION)
  list(GET VERSION_LIST 1 OCTOMAP_MINOR_VERSION)
  list(GET VERSION_LIST 2 OCTOMAP_PATCH_VERSION)
  include_directories(${OCTOMAP_INCLUDE_DIRS})
  link_directories(${OCTOMAP_LIBRARY_DIRS})
  set(FCL_HAVE_OCTOMAP 1)
  message(STATUS "FCL uses Octomap")
else()
  message(STATUS "FCL does not use Octomap")
endif()

In order to define the version check macro, we might also need to put following code into config.h.in of fcl.

#if FCL_HAVE_OCTOMAP
  #define OCTOMAP_MAJOR_VERSION @OCTOMAP_MAJOR_VERSION@
  #define OCTOMAP_MINOR_VERSION @OCTOMAP_MINOR_VERSION@
  #define OCTOMAP_PATCH_VERSION @OCTOMAP_PATCH_VERSION@

  #define OCTOMAP_VERSION_AT_LEAST(x,y,z) \
    (OCTOMAP_MAJOR_VERSION > x || (OCTOMAP_MAJOR_VERSION >= x && \
    (OCTOMAP_MINOR_VERSION > y || (OCTOMAP_MINOR_VERSION >= y && \
    OCTOMAP_PATCH_VERSION >= z))))

  #define OCTOMAP_VERSION_AT_MOST(x,y,z) \
    (OCTOMAP_MAJOR_VERSION < x || (OCTOMAP_MAJOR_VERSION <= x && \
    (OCTOMAP_MINOR_VERSION < y || (OCTOMAP_MINOR_VERSION <= y && \
    OCTOMAP_PATCH_VERSION <= z))))
#endif // FCL_HAVE_OCTOMAP

Not sure if this is overkill, but it would work.

[jvgomez]

Yep, something like that is what I had in mind if I cannot get the C++ template-based approach (altough I didn't realized about the find_package stuff), but I wanted to ask before because I also think it is a bit of an overkill. But if you are OK with this, I will do it in some hours.

[jvgomez]

Hey @jslee02, if you could take a look at the latest commit it would be nice. I did what you mention (although using pkgconfig as it is still OK). This is the version of the code that was supposed to work but it does not! For some reason I keep getting the error:

In file included from /home/jvgomez/Desktop/fcl/include/fcl/traversal/traversal_node_octree.h:45:0,
                 from /home/jvgomez/Desktop/fcl/include/fcl/traversal/traversal_node_setup.h:48,
                 from /home/jvgomez/Desktop/fcl/src/ccd/conservative_advancement.cpp:42:
/home/jvgomez/Desktop/fcl/include/fcl/octree.h:190:29: error: missing binary operator before token "("
 #if OCTOMAP_VERSION_AT_LEAST(1,8,0)

Many times, and therefore this leads to:

/home/jvgomez/Desktop/fcl/include/fcl/octree.h: In member function ‘fcl::OcTree::OcTreeNode* fcl::OcTree::getNodeChild(fcl::OcTree::OcTreeNode*, unsigned int)’:
/home/jvgomez/Desktop/fcl/include/fcl/octree.h:193:18: error: ‘fcl::OcTree::OcTreeNode’ has no member named ‘getChild’
     return node->getChild(childIdx);
                  ^

I have been checking, together with a colleague, the macros and we cannot find the problem. Eclipse macro expansion is able to correctly tell the values. To make it even worse, for example adding this:

#if OCTOMAP_MAJOR_VERSION
#error "test"
#endif

does not work either. But if we use #FCL_HAS_OCTOMAP it works. For some reasons, the new defines are not included. It must be one of those type of errors...

[jslee02]

Nice! Have you included fcl/config.h to octree.h?

[jvgomez]

That is one of the million things I tested, and didn't solve it. These definitions should be available even without including it, as FCL_HAS_OCTOMAP for example is defined and the octree.h is only included if FCL_HAS_OCTOMAP is already defined. Perhaps I am missing something anyway.

[jslee02]

I quickly tested your branch with and without octomap, and they worked. My best guess is your compiler keeps including the old config.h rather than the newly generated one. I usually do clean build in this case.. but not sure if this the case.

One possible test would be creating a new branch that installs octomap 1.7.0 (or any other version less than 1.8.0) and seeing what travis-ci would say.

[jvgomez]

Wow, this was not expected. I did clean builds most of the times. In any case, I am glad it works. Is there any way I can trigger the CI without creating a PR that is never going to be merged?

[jslee02]

I'm also glad it works! AFAIK, there is no way triggering the CI from the forked repo. It would be fine to create a dummy PR for testing (at least to me).

This PR seems to be ready to merge to me. Let's wait more days for other's feedback.

[jvgomez]

As suggested, did another PR (#130) using octomap 1.7 and seems to work nicely :)