-
Notifications
You must be signed in to change notification settings - Fork 417
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
API changes to support Octomap 1.8 #129
API changes to support Octomap 1.8 #129
Conversation
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). |
For linux, you can just change line
of |
The overall looks good to me. I have some minor suggestions.
|
Ok, we will address the comments in the following days :) |
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:
|
As octomap provides octomap-config-version.cmake, we could get the version string if octomap is found using 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 #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. |
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. |
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:
Many times, and therefore this leads to:
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:
does not work either. But if we use |
Nice! Have you included |
That is one of the million things I tested, and didn't solve it. These definitions should be available even without including it, as |
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. |
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? |
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. |
As suggested, did another PR (#130) using octomap 1.7 and seems to work nicely :) |
At the end it was not that much :)
Looking forward to your comments.
@gauthamm FYI.