Skip to content

collision_plugin_loader port to ROS 2#69

Closed
vmayoral wants to merge 2 commits intomoveit:masterfrom
AcutronicRobotics:collision-plugin-loader
Closed

collision_plugin_loader port to ROS 2#69
vmayoral wants to merge 2 commits intomoveit:masterfrom
AcutronicRobotics:collision-plugin-loader

Conversation

@vmayoral
Copy link
Copy Markdown
Contributor

@vmayoral vmayoral commented Apr 1, 2019

Authored originally by @anasarrak.

@vmayoral vmayoral changed the title Collision plugin loader collision_plugin_loader port to ROS 2 Apr 1, 2019
@mlautman
Copy link
Copy Markdown
Contributor

mlautman commented Apr 1, 2019

Let's rebase this onto master so that we can start using travis

@vmayoral
Copy link
Copy Markdown
Contributor Author

vmayoral commented Apr 2, 2019

@mlautman rebased this and #71 to check CI. I see #71 failed.

Edit: Looking into why it failed.

@vmayoral
Copy link
Copy Markdown
Contributor Author

vmayoral commented Apr 2, 2019

On a second look, it looks like the CI is not building anything just yet right @mlautman? https://github.com/ros-planning/moveit2/blob/master/.docker/ci/Dockerfile

Update: It seems part of it is being tested indeed https://travis-ci.org/ros-planning/moveit2/builds/514743505#L1322, only not what's of interest for this PR. What's the flow you'd expect for new PRs? Shall we take care of updating the CI or will you guys take care of it?

@mlautman
Copy link
Copy Markdown
Contributor

mlautman commented Apr 2, 2019

I think that building the sub-packages and passing travis is an essential part of the PR process. Ideally each PR would include the changes necessary for travis to build the changes.

In this case, there are dependencies that will need to be ported before this can be built. (moveit_core and moveit_ros_perception) If there are open PR's for the dependencies that are blocking this port, then I would recommend that you include links in the PR description. That will help direct our focus towards getting the underlying dependencies done first.

@vmayoral
Copy link
Copy Markdown
Contributor Author

vmayoral commented Apr 2, 2019

I think that building the sub-packages and passing travis is an essential part of the PR process. Ideally each PR would include the changes necessary for travis to build the changes.

In this case, there are dependencies that will need to be ported before this can be built. (moveit_core and moveit_ros_perception) If there are open PR's for the dependencies that are blocking this port, then I would recommend that you include links in the PR description. That will help direct our focus towards getting the underlying dependencies done first.

Great, thanks. Can you also indicate which changes would be required for travis? Maybe provide an example for a simpler module (e.g. #67)?

@mlautman
Copy link
Copy Markdown
Contributor

mlautman commented Apr 3, 2019

@vmayoral Here are the steps:

Let me know if this works for you

@vmayoral vmayoral force-pushed the collision-plugin-loader branch from 97725b7 to 043a2f9 Compare April 5, 2019 13:35
@vmayoral
Copy link
Copy Markdown
Contributor Author

vmayoral commented Apr 8, 2019

@mlautman, I went ahead and performed the steps described over the weekend. Thanks for the guidelines. Unfortunately, the result wasn't satisfactory and @LanderU and I ended up having to modify the Docker image and moveit_ci quite heavily to make it work.

See AcutronicRobotics#34 for the complete discussion or moveit/moveit_ci#56 for the proposed changes. Returning to this PR, I'd advice we go ahead and merge these changes since they already pass CI (see #62 (comment)).

@mlautman
Copy link
Copy Markdown
Contributor

mlautman commented Apr 9, 2019

Unfortunately, the result wasn't satisfactory

This is a good thing. The fact that CI failed shows us were there are still issues. Let's fix them in moveit/moveit_ci#56 and get these packages building :)

@henningkayser
Copy link
Copy Markdown
Member

@mlautman CI seems to pass now, is there still something missing here? Also, I was wondering if we should delay migrating the collision plugins until moveit/moveit#1584 is synced. The plugin loader doesn't seem to be affected though.

@henningkayser
Copy link
Copy Markdown
Member

@JafarAbdi could you reopen this one as well?

@JafarAbdi
Copy link
Copy Markdown
Member

@henningkayser Absolutely - I'll finish addressing the review for the other PR and open a new one for this package

@henningkayser
Copy link
Copy Markdown
Member

Closing in favor of #137

MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
* Remove move group prefixes
* Fix quickstart rviz tutorial
* Fix move_group_interface and move_it_cpp tutorials
* Update doc/moveit_cpp/src/moveit_cpp_tutorial.cpp
* Add ros shutdown to end of tutorial
* Update move_group_interface_tutorial.rst
Co-authored-by: Jafar Abdi <cafer.abdi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants