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

Port rdf_loader to ROS2 #104

Merged
merged 5 commits into from
Dec 6, 2019
Merged

Conversation

JafarAbdi
Copy link
Contributor

Description

This is a continuation of PR#76

The addressed review was added to the master branch see but because I'm not able to cherry-pick the changes for the specific files of rdf_loader, I just applied the changes to the rdf_loader and committed them

moveit_ros/planning/rdf_loader/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

This looks pretty good so far. +1 from me after you fixed up the remaining changes

moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/rdf_loader/src/rdf_loader.cpp Outdated Show resolved Hide resolved
MoveIt2 Beta - Phase 1 automation moved this from Review in progress to Reviewer approved Oct 31, 2019
This was referenced Nov 21, 2019
@JafarAbdi JafarAbdi force-pushed the pr-rdf_loader branch 3 times, most recently from 0739631 to 83c9ba4 Compare November 30, 2019 11:59

if (!nh.searchParam(robot_description, robot_description_) || !nh.getParam(robot_description_, content))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'm happy with the new implementation, has_parameter() + get_parameter seems like an overhead to me. Also, we need to discuss how searchParam() should be implemented. @mkhansen-intel do you have any suggestions for this part?

@henningkayser henningkayser merged commit 7c77c08 into moveit:master Dec 6, 2019
MoveIt2 Beta - Phase 1 automation moved this from Reviewer approved to Done Dec 6, 2019
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants