Skip to content
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

fuse -> ROS 2 fuse_constraints: Port fuse_constraints #290

Merged

Conversation

methylDragon
Copy link
Collaborator

@methylDragon methylDragon commented Nov 18, 2022

See: #276

Description

This PR ports the entirety of fuse_constraints, including:

  • CMakeLists and packages
  • Source code
  • Tests

Other stuff like docs are in other PRs.
Caught a bug from the time port in the tests due to signature changes between ros::Time and rclcpp::Time, fixed (rclcpp time needed a conversion to nanoseconds from seconds)

Notes

Pinging @svwilliams for visibility.

Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Collaborator Author

methylDragon commented Dec 9, 2022

@ros-pull-request-builder retest this please!
Build Status

fuse_constraints/CMakeLists.txt Outdated Show resolved Hide resolved
fuse_constraints/CMakeLists.txt Outdated Show resolved Hide resolved
fuse_constraints/CMakeLists.txt Outdated Show resolved Hide resolved
find_package(benchmark QUIET)
if(benchmark_FOUND)
# Normal Delta Pose 2D benchmark
add_executable(benchmark_normal_delta_pose_2d benchmark_normal_delta_pose_2d.cpp)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a utility ament_add_google_benchmark that could be used here. It's a little different from this pattern. Instead of finding Google benchmark quietly, it requires it but disables the test unless -DAMENT_RUN_PERFORMANCE_TESTS=ON is set.

https://github.com/ros2/rclcpp/blob/91bc312190e61c8915b0152e5c927c987fc22d6c/rclcpp_components/CMakeLists.txt#L137-L140

@sloretz
Copy link
Collaborator

sloretz commented Dec 9, 2022

Ah it looked good to me, but CI is failing with

16:16:16 CMake Error at CMakeLists.txt:34 (add_library):
16:16:16   Target "fuse_constraints" links to target "SuiteSparse::CCOLAMD" but the
16:16:16   target was not found.  Perhaps a find_package() call is missing for an
16:16:16   IMPORTED target, or an ALIAS target is missing?

${geometry_msgs_TARGETS}
pluginlib::pluginlib
rclcpp::rclcpp
SuiteSparse::CCOLAMD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments. First:

This target doesn't exist, but we can create it in FindSUITESPARCE.cmake.

At the bottom of that find module, create an imported target

add_library(SuiteSparce::CC0LAMD IMPORTED)
target_link_libraries(SuiteSparce::CC0LAMD IMPORTED ${SUITESPARCE_LIBRARIES}`)
target_include_directories(SuiteSparce::CC0LAMD IMPORTED ${SUITESPARCE_INCLUDE_DIRS}`)

The advantage of doing it that way is the target can still be transitively depended upon downstream.

Second:

I didn't see any use of SuiteSparce in the header files. I think that means we can make this a PRIVATE dependency here:

target_link_libraries(${PROJECT_NAME} PRIVATE
    SuiteSparce::CC0LAMD)

@methylDragon
Copy link
Collaborator Author

methylDragon commented Dec 9, 2022

@ros-pull-request-builder retest this please!
Build Status

@methylDragon
Copy link
Collaborator Author

methylDragon commented Dec 9, 2022

@ros-pull-request-builder retest this please!
Build Status

@methylDragon
Copy link
Collaborator Author

methylDragon commented Dec 9, 2022

@ros-pull-request-builder retest this please!
Build Status

Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Collaborator Author

@ros-pull-request-builder retest this please!
Build Status

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@methylDragon
Copy link
Collaborator Author

@ros-pull-request-builder retest this please!
Build Status

@methylDragon methylDragon merged commit ad645ee into locusrobotics:rolling Dec 9, 2022
@methylDragon methylDragon deleted the rolling-constraints branch December 9, 2022 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants