Refactor moveit_core package to export a single library #1801
Conversation
Codecov ReportBase: 50.24% // Head: 49.59% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1801 +/- ##
==========================================
- Coverage 50.24% 49.59% -0.64%
==========================================
Files 374 381 +7
Lines 31277 31667 +390
==========================================
- Hits 15712 15703 -9
- Misses 15565 15964 +399
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
This pull request is in conflict. Could you fix it @ChrisThrasher? |
| @@ -0,0 +1,22 @@ | |||
| add_subdirectory(collision_detection_bullet) | |||
There was a problem hiding this comment.
Is this file really necessary? Having the content in the main file means less searching around and I don't see any additional complexity that is worth hiding.
There was a problem hiding this comment.
No it's not strictly necessary. My thought was that it was nice to bury some of this code to avoid having to relocate and change the path to many dozens of source files. I could be convinced to move all those target_sources call up the main CMakeLists.txt thus eliding them.
My bigger holdup was whether it was worth reorganizing all the tests. Currently I'm keeping them in place but I'm on the fence about whether they should get moved out into their own tests directory alongside include and src. If we do this, then we can delete all these extra CMakeLists.txt in src/. That would be more consistent with how else I've reorganized things but I'd like someone else's input.
There was a problem hiding this comment.
I was more thinking about using add_subdirectory(src/...). The separate target sources make sense to me, and would even make more sense if we could group all of these targets into fewer but more meaningful namespaces/contexts. Until then, I think it's ok to leave tests where they are for now.
There was a problem hiding this comment.
I was more thinking about using add_subdirectory(src/...).
That sounds slightly better to me as well.
whether they should get moved out into their own tests directory alongside include and src
I do think it makes much more sense to keep the tests in the individual source folders.
moveit_core might become one library now, conceptually, but it's still pretty big and the separate folders help to show what component has how many tests.
| @@ -40,7 +40,7 @@ | |||
| #include <rclcpp/logging.hpp> | |||
|
|
|||
| // Logger | |||
| static const rclcpp::Logger LOGGER = rclcpp::get_logger("moveit_collision_detection.collision_world_allvalid"); | |||
| static const rclcpp::Logger LOGGER = rclcpp::get_logger("moveit_core.collision_world_allvalid"); | |||
There was a problem hiding this comment.
I agree that the logger names should be revised, but setting them all to moveit_core doesn't really make them better. Let's omit these changes for now and first discuss how we want to log MoveIt in general.
My thought is that some context is still needed (here maybe collision_detection) and that this context should be reflected by file structure and namespace. Ideally we should define a pattern that we could use everywhere, something like moveit.<package>.<context>.<optional detail> -> moveit.core.collision_detection. (copied to #1614 (comment))
Curiously, this file is *_env_allvalid and the logger says _world_allvalid...
There was a problem hiding this comment.
I would suggest we change the logger names to just:
<package_name>.<source_file_name>
There are two uses of these identifiers I know of:
- Enabling DEBUG level on a small amount of code to get the debug output.
- Searching through the code for an ERROR log to figure out where it came from.
I believe the format above would be effective for both of these use cases.
In the code this comment is on it would be changed to moveit_core.collision_world_allvalid
There was a problem hiding this comment.
third use case is setting up logger filters for debugging of packages/contexts/detail, and with detail I mean plugin or class instance. Actually, adding file would be redundant because that's already reported by the logger. (see https://docs.ros2.org/latest/api/rcutils/logging_8h.html#a27340ac73188b1cf8d9cb96d86c76694)
There was a problem hiding this comment.
No problem, I can revert these logger name changes. I was even 100% they were worthwhile changes to begin with but I was just doing pattern matching.
| endif() | ||
|
|
||
| ament_add_gtest(test_fcl_collision_env test/test_fcl_env.cpp) | ||
| target_link_libraries(test_fcl_collision_env moveit::core) |
There was a problem hiding this comment.
Doesn't this change the behavior of the test? The old cmake didn't link against the plugin code so it implicitly tested for valid plugin registration and loading. This version might omit this step, no?
There was a problem hiding this comment.
That's a good point. I think it's reasonable to keep the plugins as separate targets. I'll have to think about that more.
There was a problem hiding this comment.
We talked about this in the MoveIt standup this morning. It came up that there is a technical reason for this. Something to do with how classloader works. You can't load a plugin from the same library that the base class of the plugin is in because of how ClassLoader uses RTTI. The plugins will need to be separate targets.
There was a problem hiding this comment.
It is best practice to keep plugin lib instantiations in a separate standalone library and I would strongly propose you keep it that way. We had a similar case in MTC last year (that I brought up because of the infamous memory leaks) and ended up splitting the plugins off again.
Personally my main argument is that unless strictly necessary, libraries should not execute code on so-load, unless strictly needed for every use of the library. But that's exactly what the PLUGINLIB/CLASSLOADER macros do and not every use of moveit::core requires plugins. You have no idea how often boost libraries broke my neck as a linux distribution maintainer because of some random unnecessary initialization it does on load.
There was a problem hiding this comment.
Viable options I see to move the plugins out:
- new moveit_plugins subfolders (actually new packages) for all
- a separate
moveit::core_pluginslibrary for the three of them - add
moveit::collision_pluginsand split the butterworth filter out into moveit_plugins. fcl/bullet are definitely close to moveit::core, but the signal smoother is only used in moveit_servo. On the other hand moveit_core does not use the plugins but directly relies on the classes in most cases.
I slightly prefer the last option.
There was a problem hiding this comment.
Great simplification, I really appreciate this effort! Some notes from my end:
- Should we maybe keep separate plugin libraries? I think it would make sense to eventually separate out plugin implementations along with their dependencies (e.g. Bullet/FCL). That would cleanly delineate what core really does (feature integration+abstraction), and the feature implementation itself (collision checking, smoothing, etc). This is already the case for IK and planners.
- Let's split out the logger name changes (see comment)
- Could you add an entry to the migration notes for those depending on modules?
- There is still a
moveit_core/dummy.cpp
@lilustga would you have some time to review and test the export header for Windows?
|
@ChrisThrasher Can you rebase this PR so we can consider merging it? |
I wish I could. At a minimum, we need to find a home for plugins in If anyone still has lingering issues with this PR, please share them now before I put any more work into reimplementing this. |
ChrisThrasher
changed the title
| @@ -0,0 +1,22 @@ | |||
| add_subdirectory(collision_detection_bullet) | |||
There was a problem hiding this comment.
I was more thinking about using add_subdirectory(src/...).
That sounds slightly better to me as well.
whether they should get moved out into their own tests directory alongside include and src
I do think it makes much more sense to keep the tests in the individual source folders.
moveit_core might become one library now, conceptually, but it's still pretty big and the separate folders help to show what component has how many tests.
| add_library(moveit::core ALIAS moveit_core) | ||
| generate_export_header(moveit_core EXPORT_FILE_NAME include/moveit/export.h) | ||
| target_include_directories(moveit_core PUBLIC | ||
| $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include> |
There was a problem hiding this comment.
I'm not convinced we gain anything from splitting sources and headers for the different components. I'll not block the decision though if you think it more convenient.
The decision to split the headers of is fully independent of whether or not there should be one libmoveit_core.so and it's definitely not considered separately in this discussion so far.
Yes, it consolidates paths a bit. But it also adds three subdirectory changes to go from a component's sources to the respective headers...
| endif() | ||
|
|
||
| ament_add_gtest(test_fcl_collision_env test/test_fcl_env.cpp) | ||
| target_link_libraries(test_fcl_collision_env moveit::core) |
There was a problem hiding this comment.
Viable options I see to move the plugins out:
- new moveit_plugins subfolders (actually new packages) for all
- a separate
moveit::core_pluginslibrary for the three of them - add
moveit::collision_pluginsand split the butterworth filter out into moveit_plugins. fcl/bullet are definitely close to moveit::core, but the signal smoother is only used in moveit_servo. On the other hand moveit_core does not use the plugins but directly relies on the classes in most cases.
I slightly prefer the last option.
|
I spent yesterday trying to split out the three plugins in moveit_core and it was awful.
I didn't even get to the last one. I started with the two collision_detector plugins which I tried to move to a new package within the moveit_plugins meta package. The major issue is that there are parts of moveit_core than depend heavily on collision_detector_fcl so extracting that whole directory is very messy. I tried to move all the dependent code out of that collision_detector_fcl into the collision_detection package but I think I ended up created a cyclic dependency in the Does anyone have other ideas for extracting collision_detector_fcl when parts of moveit_core are already depending on it? I might change focus to instead extract the other two plugins which will hopefully put up less of a fight then we can get a clearer picture of how to handle collision_detector_fcl. |
|
This pull request is in conflict. Could you fix it @ChrisThrasher? |
No I cannot. |
|
This pull request is in conflict. Could you fix it @ChrisThrasher? |
Description
moveit_core is currently comprised of a few dozen, small libraries. Some of these libraries are small as a single .cpp/.h pair. Some libraries have circular dependencies with others which mean they're difficult if not impossible to use in isolation.
This PR consolidates those libraries into a single
moveit::coretarget which better expresses the fact that moveit_core is intended to be used as one library, not a collection of them. Along the way I was able to make some satisfying simplifications like how many dozens oftarget_link_libraries,target_include_directories, andinstallcommands were collapsed into just a small few. There's a certain amount of CMake overhead related to each sub-library which is now gone. This will make it easier for others to understand how moveit_core is built and understand how to modify it as needed as requirements change in the future.This change also required no modification to downstream packages which continue to use moveit_core exactly as they have before.
I'm submitting this PR with the full, verbose commit history so reviewers can see the incremental steps I took, but I plan on squashing this into a single commit before merging since these individual commits are not particularly valuable.