-
Notifications
You must be signed in to change notification settings - Fork 81
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
Cmake files added #15
Cmake files added #15
Conversation
It may be a bit confusing that there is a commit which talks about the locale settings still. This is because I originally had it in there. But have removed it in this branch which I used for the PR. So this PR only adds the CMake files and doesn't change anything in the source. |
Ah, and another note: The cmake files still use catkin (though no other ROS packages). At some stage the catkin dependencies should be made optional, so that the CMakeLists can be used without catkin as well. Put this on the TODO list. |
Overall, looks pretty good. I will download and try it out this weekend. Some of my thoughts:
|
…les CGDB and PLY stuff
Sounds reasonable. I've updated the branch for my PR.
Note about the new CMakeLists.txt: It can be used with or without catkin. It detects whether catkin is used to compile, and if it is, it uses catkin commands. Otherwise, it doesn't. I have successfully compiled and run the simulator using cmake only as well. |
Side note: If including the graspit library compiled with cmake only from the install directories (e.g. from an external package), a few changes in the original source are required:
The src/ and include/ directories should be added by the makefile. Such changes are only in the master branch of my fork so far. This commit still doesn't touch the sources. |
So I am able to get this to build just fine when using cmake. When I use catkin_make, I get: CMake Error at /opt/ros/hydro/share/catkin/cmake/catkin_package.cmake:291 (message): should this line in CMakeMacros/FindQhull.cmake: If I change the above, then it builds fine with cmake and catkin_make. I am on 12.04 not 14.04 on the machine I am currently testing this perhaps that accounts for the difference? If I make this change, and then I build with catkin_make, It compiles just fine when it is alone. If I have a graspit plugin in my workspace, then catkin_make fails. I have not been able to figure out exactly why yet. As a minimal example, I make a workspace with
for the moveit plugin, I have to comment out everythin inside grasp_msg_publisher.cpp sendPickupRequest function this has to do with another PR. With this workspace, caktin_make fails with:
But if I run it with the graspit-ros package rather than the graspit package, everything builds just fine. The graspit_moveit_plugin does not generate any messages or have the targets that the error message is saying it does. I know the error doesn't mention anything to do with the graspit package, but it is causing the problem. I am not sure what exactly is causing the problem. |
Thanks for all the testing! 1) Qhull error Yes the qhull error is most likely due to our different system configurations. The FindQHull.cmake is a copy of the PCL FindQhull.cmake (if I remember correctly).. On my system it won't compile without the To fix this, I removed the addition of qhull directory, so it'll compile on yours, and added a separate search for qhull_a.h so it'll compile on mine as well. I've updated the PR. Can you please try if this works fine on yours now? 2) Message generation error I can't compile graspit_moveit_plugin with my fork of graspit:
Because of the graspit_source reference I assume either the CURG graspit-ros or the graspit-simulator/graspit-ros are required to compile with cmake. So I've tried that, instead of using my fork. Unfortunately, neither of the graspit-ros packages work for me out-of-the box. With CURG/graspit-ros:
There are more errors if I fix the individual issues, so I tried graspit-simulator/graspit-ros:
I could fix the issues, but thought there's little point if I don't know which of the two packages you have been using, or if you have the graspit_source elsewhere in your system. Please send me the details of your set-up so I can try to reproduce the error. Anyway, two first thoughts:
As a first try I would edit line 155 in graspit_moveit_plugin/CMakeLists.txt and change it from
to
I'm not sure this will make a difference at all but it's worth a shot. I've had message generation errors in the past which were often related to the exported targets (or lack of them). |
P.S.: You could also try to comment out line 677 in my CMakeLists.txt:
This line is not really needed to compile all my packages, I just added it for completeness so the cmake file can be used with or without catkin. Not sure if it does anything at all, because I'm not using add_dependencies() anywhere. But maybe the *_EXPORTED_TARGETS variables are used by the catkin build system internally... |
@@ -0,0 +1,28 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we fix the package xml to look more like:
https://github.com/graspit-simulator/graspit-ros/blob/master/graspit/package.xml
Ok I tried it again as now I'm testing on Indigo anyway. Here I could reproduce your error. It makes total sense because graspit_msgs was not processed before graspit_moveit_plugin in the cmake pipeline. I also realized I had gotten something wrong in your last post, sorry about that: about you maybe having the graspit_source somewhere else. That's not needed of course if you have a working graspit-ros in your catkin_ws, and you compiled my graspit fork only with cmake or with no graspit_moveit_plugin in the catkin_ws... ouups. It must have been late ;) Anyway, now it's starting to compile but I obviously get the same error like before from graspit_moveit_plugin because of the missing graspit_source. Only first line:
That's no surprise. I guess the #include directives would have to be changed in graspit_moveit_plugin, which would make sense anyway (i.e. take out all "graspit_source/" and "include/" from the includes and instead add required paths in the CMakeLists.txt) |
SET( QT_USE_QT3SUPPORT TRUE ) | ||
find_package(Qt4 COMPONENTS QtCore REQUIRED) | ||
|
||
# include QT_USE_FILE is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comment
can we add to the commit to remove all the .pro files. |
Hi Jennifer, What did you add to the CMakeLists.txt for this? Thanks, |
Hi Jake, I edited CMakeLists.txt as you suggested, all makes sense. Can you check again if I got it right please? To your question: I tried to compile graspit_moveit_plugin, but only got so far. Did the following changes
But then I get stuck on indigo, I'll look into this, unless you already have some ideas?
There seems to be an issue with the MoveIt! headers on indigo (included by planning_scene_msg_builder.h:7), not sure if I'm overlooking something here. I've just started to look into it now but have something else I need to do in the next hour or two... |
Graspit has tiny xml included in it, which I would assume is an older version than the one used by Moveit. |
…o compile and have flag saying whether or not to build them at the top
@JenniferBuehler |
One thing I forgot to say about the (*) side note posted above. |
I think a good plugin to look at is the openclose plugin inside graspit as it includes a good number of GraspIt! headers. It currently does not work with the cmake graspit version. I made a PR to this branch with plugin fixes for the demo plugins that come with graspit. The only change that I don't really like is how I fixed the .pro file to allow the plugin to find the ui_* headers, but I am not sure of a better way to do this. I think it will be helpful to write a findGraspit.cmake sometime soon. If you want to run the plugin |
Thanks for checking this. I will test it as soon as I can. |
I have never done anything with the PQP_COLLISION flag. My understanding of it is that Graspit can use either PQP for collision detection, or graspit's own collision detection. Except for when Bullet is in use, and dynamics are running, then it uses bullet's collision detection. |
I never tried the plugins prior to the windows changes. |
Ok, thanks. I hope to get the time to look at it one of the next days. So to confirm with @mateiciocarlie, we could use the following DYNAMICS_ENGINE flags and put it all under one hood:
Or am I missing anything? Is it possible to use no dynamics, but still use bullet, but with PQP collision detection instead of bullet collision detection? |
I think you got it. I cannot currently use bullet without dynamics. What is it printing? |
I haven't tried it yet, just working on something else. What I was asking is what you mean with plugins not currently working with the cmake version. What is going wrong: Segfault? Error? |
The choice of dynamics and the choice of collision engine should be independent, even though some dynamics engines will actually provide both. Also, one can choose to use a different collision engine and just never use the dynamics engine at all - a lot of GraspIt's usefulness actually comes from the static simulations. So perhaps we can have two independent flags, e.g. DYNAMICS_ENGINE and COLLISION_ENGINE? |
Sounds reasonable, so confirming: DYNAMICS_ENGINE:
COLLISION_ENGINE
Choosing GRASPIT_DYNAMICS and BULLET_COLLISION, and probably other combinations, would be invalid though? So checking for inconsistencies may get a little convoluted, but that's not avoidable if making 2 variables of it... |
It didn't compile unless I made the changes from the PR I sent you. With the PR, it runs fine. |
Agreed! COLLISION_ENGINE should also get the PQP_COLLISION option though. |
Oh, of course, forgot PQP_COLLISION. So no error checking in cmakelists.txt? @jvarley: Ok so the plugin does work with the new windows changes, after your PR? |
yes |
YAY!!! |
lol, I know right. Thanks for checking out windows, that would have taken me forever. |
It did take me forever too. Compiling on Windows is a nightmare. Always you think you're almost there, and then... you aren't. I hope to not see Windows for a long, long while after this is done ;) |
@jvarley You can use this Findgraspit.cmake to start with, it can probably be improved. |
I will checkout the Findgraspit.cmake. I just made a pr for graspit-ros: This switches it over to work with graspit built from cmake. It still has graspit as a submodule, I don't know if we want to keep it that way, or switch over to using a Findgraspit.cmake similar to gazebo, pcl etc. I am still having a small problem, where it places graspit in build rather than devel, I am trying to fix it at the moment, let me know if you guys know how. Once this is fixed, I think we are ready to pull in everything. |
@mateiciocarlie @JenniferBuehler I think we are ready to pull in both this PR, and Best of my knowledge, we currently work
Any concerns? -Jake |
@jvarley if you build graspit with cmake (not with catkin), it will always put it in the build folder by default. It is catkin which changes the install destination to the devel directory. You could maybe try setting CMAKE_INSTALL_PREFIX manually? Are you trying to compile graspit from within the catkin workspace (eg. pulling it in as external cmake source)? |
Cmake plugins
I fixed it, I just need to move the catkin_package linie above the add_subdirectory line in the graspit-ros cmakelist.txt |
Currently graspit is a submodule to graspit-ros. I think at some point I would prefer to make it an external cmake source, but I need to talk to Matei about that, and I think it can be left for another day. At the moment, I think we are good to go. |
Hooray! Merged. Thanks for all the contributions here - it was s big one! |
This pull request only adds the cmake files. No changes to original source included yet.