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

Migrate PRBT ikfast plugin from moveit_resources #2697

Closed
wants to merge 4 commits into from

Conversation

v4hn
Copy link
Contributor

@v4hn v4hn commented May 28, 2021

As announced in #2653 (comment) .
Together with moveit/moveit_resources#79 this is supposed to provide a straight-forward solution for #2653 .

Moving the plugin here circumnavigates problems with running tests in the moveit repository that are based on it. Before, this plugin was part of moveit_resources, but introduced a compile time dependency on moveit_core.
Because a repository's tests should run without having to compile downstream code (a sane requirement on buildfarm/prerelease tests), we move the plugin here instead.

This circumnavigates problems with running tests in the moveit repository
that are based on this plugin. Before, this plugin was part of
moveit_resources, but introduced a compile time dependency on moveit_core.

Because a repository's tests should run without having to compile
downstream code (a sane requirement on buildfarm/prerelease tests),
we move the plugin here instead.
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

As said before, I do not support moving a specific IKfast planner into the moveit_kinematics package, which should provide generic interfaces only.
Additionally, as pointed out in moveit/moveit_resources#79 (comment), moving the IK plugin into moveit_kinematics, will not resolve the issue that moveit_resources depends on moveit_kinematics.

If that is the only alternative solution we can come up with, I would prefer the current solution, pulling both moveit and moveit_resources into the target workspace of GHA (and accept the broken prerelease + build farm test).

@v4hn
Copy link
Contributor Author

v4hn commented May 31, 2021

As said before, I do not support moving a specific IKfast planner into the moveit_kinematics package, which should provide generic interfaces only.

We could put it in a separate ROS package in the repository if that's your main concern.
The only important thing to get this to work is that the code is somewhere in this repository and it somewhat made sense to me to associate it with moveit_kinematics.

@codecov
Copy link

codecov bot commented May 31, 2021

Codecov Report

Merging #2697 (04c71b4) into master (b75d37e) will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2697      +/-   ##
==========================================
- Coverage   60.62%   60.51%   -0.11%     
==========================================
  Files         402      402              
  Lines       29615    29615              
==========================================
- Hits        17952    17918      -34     
- Misses      11663    11697      +34     
Impacted Files Coverage Δ
.../ompl_interface/src/detail/constrained_sampler.cpp 43.25% <0.00%> (-16.21%) ⬇️
...ove_group_interface/src/wrap_python_move_group.cpp 34.83% <0.00%> (-2.98%) ⬇️
...e/src/parameterization/model_based_state_space.cpp 68.31% <0.00%> (-2.81%) ⬇️
moveit_core/robot_model/src/joint_model_group.cpp 63.06% <0.00%> (-1.94%) ⬇️
...meterization/work_space/pose_model_state_space.cpp 82.70% <0.00%> (-0.64%) ⬇️
.../move_group_interface/src/move_group_interface.cpp 47.12% <0.00%> (-0.19%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 68.55% <0.00%> (-0.13%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 50.64% <0.00%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b75d37e...04c71b4. Read the comment docs.

@v4hn
Copy link
Contributor Author

v4hn commented Jun 10, 2021

Closing as we decided to take a different approach.
Still, someone has to actually do all the work for this...

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.

None yet

2 participants