-
Notifications
You must be signed in to change notification settings - Fork 514
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
Port planning_scene to ROS2 #43
Conversation
Authored by @anasarrak
@@ -1,7 +1,8 @@ | |||
set(MOVEIT_LIB_NAME moveit_planning_scene) | |||
|
|||
add_library(${MOVEIT_LIB_NAME} src/planning_scene.cpp) | |||
set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES VERSION "${${PROJECT_NAME}_VERSION}") | |||
#TODO: Fix the versioning | |||
# set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES VERSION "${${PROJECT_NAME}_VERSION}") |
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.
This was added in moveit/moveit#273. Maybe @v4hn can confirm this but perhaps we can take the same approach as the urdf_parser
target_link_libraries(test_planning_scene ${MOVEIT_LIB_NAME}) | ||
endif() | ||
#TODO: Fix this so it can be built with colcon | ||
# if(BUILD_TESTING) |
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.
re-enable tests please
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.
Tests failing. Let's try to solve AcutronicRobotics#13 first please. We should be able to re-enable all of them afterwards.
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.
You all fixed this somewhere in AcutronicRobotics#71, is that correct?
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.
Yes however they should be submitted here (and we should rebase this code as well). @ahcorde started doing it iteratively while we review the changes.
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.
+1 to restoring tests before merge
moveit_core/planning_scene/include/moveit/planning_scene/planning_scene.h
Outdated
Show resolved
Hide resolved
I applied the clang formatting to this PR. Once we have the tests enabled, and we resolved why world is being made non-const +1 |
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.
Some small changes, enable and pass tests then +1.
target_link_libraries(test_planning_scene ${MOVEIT_LIB_NAME}) | ||
endif() | ||
#TODO: Fix this so it can be built with colcon | ||
# if(BUILD_TESTING) |
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.
+1 to restoring tests before merge
@mlautman updated |
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.
+1 from me after this point is addressed
@@ -1,30 +1,56 @@ | |||
set(MOVEIT_LIB_NAME moveit_planning_scene) | |||
|
|||
add_library(${MOVEIT_LIB_NAME} src/planning_scene.cpp) | |||
if(WIN32) | |||
# set(append_library_dirs "$<TARGET_FILE_DIR:${PROJECT_NAME}>;$<TARGET_FILE_DIR:${PROJECT_NAME}_TestPlugins1>") |
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.
This should be fixed, commented or removed.
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.
Done ;)
include_directories(${moveit_resources_INCLUDE_DIRS}) | ||
|
||
if(UNIX OR APPLE) | ||
set(append_library_dirs "${CMAKE_CURRENT_BINARY_DIR}:${CMAKE_CURRENT_BINARY_DIR}/../utils:${CMAKE_CURRENT_BINARY_DIR}/../collision_detection_fcl:${CMAKE_CURRENT_BINARY_DIR}/../collision_detection") |
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.
Hardcoding relative paths is a bad idea.
I would recommend that for every library in this package that is used else ware you set a <lib_name>_lib_dir
environment variable that can then be used by the tests
A good example is the rcl_lib_dir
I have taken the time to show how it is used below:
/home/mike/ws_ros2/src/ros2/rcl/rcl/CMakeLists.txt:
78
79: # rcl_lib_dir is passed as APPEND_LIBRARY_DIRS for each ament_add_gtest call so
80 # the librcl that they link against is on the library path.
81 # This is especially important on Windows.
82 # This is overwritten each loop, but which one it points to doesn't really matter.
83: set(rcl_lib_dir "$<TARGET_FILE_DIR:${PROJECT_NAME}>")
84
/home/mike/ws_ros2/src/ros2/rcl/rcl/test/CMakeLists.txt:
16
17: set(extra_lib_dirs "${rcl_lib_dir}")
18
/home/mike/ws_ros2/src/ros2/rcl/rcl/test/CMakeLists.txt:
34 rcl_add_custom_gtest(test_client${target_suffix}
35 SRCS rcl/test_client.cpp
36: INCLUDE_DIRS ${osrf_testing_tools_cpp_INCLUDE_DIRS}
37 ENV ${rmw_implementation_env_var}
38 APPEND_LIBRARY_DIRS ${extra_lib_dirs}
The example uses rcl_add_custom_gtest but it should work just as well with ament_add_gtest
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.
If you don't mind I prefer to merge this. And then I will fix them all together. Because we have other merged that use this style. I can open an issue to avoid forgetting it.
Ready to merge @henningkayser |
@henningkayser or @davetcoleman can we merge this PR? |
Update CI to moveit_ci
* clang formatting * adding clang-format notes to README * fixing build warnings
* updated cmake to colcon/ament * file cleanup and commented out unconfigured build_depend in package.xml * more cleanup * removed subdirectory moveit_cpp * Switch to travis-ci.com in readme links * fix to travis status in readme * Update conf.py to generate CNAME * updated to meta.keys() (moveit#45) * fix travis for custom domain (moveit#47) * updated cmake with moveit_package, removed empty cmake, added vcs file * package.xml cleanup * updated readme with source build instructions * updated travis to foxy * updated repos to include moveit2.repos * fixed typo * ament format * more ament formatting * removed code coverage ci test added moveit.repos * updated cmake to colcon/ament * file cleanup and commented out unconfigured build_depend in package.xml * more cleanup * removed subdirectory moveit_cpp * updated cmake with moveit_package, removed empty cmake, added vcs file * package.xml cleanup * updated readme with source build instructions * updated travis to foxy * updated repos to include moveit2.repos * fixed typo * ament format * more ament formatting * removed code coverage ci test added moveit.repos * fixed travis and repose files, rebased on main * re-added fix travis for custom domain Co-authored-by: Alex Goldman <[email protected]>
Authored by @anasarrak