Skip to content

use $(dirname) in MoveIt Setup Assistant#2748

Merged
v4hn merged 1 commit intomoveit:masterfrom
v4hn:pr-master-msa-use-dirname
Jul 1, 2021
Merged

use $(dirname) in MoveIt Setup Assistant#2748
v4hn merged 1 commit intomoveit:masterfrom
v4hn:pr-master-msa-use-dirname

Conversation

@v4hn
Copy link
Copy Markdown
Contributor

@v4hn v4hn commented Jun 30, 2021

Available since lunar. This simplifies the descriptions in moveit_resources
quite a bit because their package name does not match the include path
and we have to adapt the names everywhere after edits.

Note that two candidates for this replacement still remain -
they define the file path to the rviz config in an arg value.
Due to a bug these two can not be replaced at this moment.

@gavanderhoorn Could you please review this patch?

Available since lunar. This simplifies the descriptions in moveit_resources
quite a bit because their package name does not match the include path
and we have to adapt the names everywhere after edits.

Note that two candidates for this replacement still remain -
they define the file path to the rviz config in an arg value.
Due to [a bug](ros/ros_comm#1487)
these two can not be replaced at this moment.
@v4hn v4hn requested a review from rhaschke as a code owner June 30, 2021 19:04
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 30, 2021

Codecov Report

Merging #2748 (89d0af2) into master (b440c72) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2748      +/-   ##
==========================================
- Coverage   60.51%   60.49%   -0.02%     
==========================================
  Files         402      402              
  Lines       29659    29659              
==========================================
- Hits        17944    17938       -6     
- Misses      11715    11721       +6     
Impacted Files Coverage Δ
...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%) ⬇️
...on/pick_place/src/approach_and_translate_stage.cpp 73.24% <0.00%> (-0.70%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 81.00% <0.00%> (-0.41%) ⬇️
...meterization/work_space/pose_model_state_space.cpp 83.98% <0.00%> (+0.65%) ⬆️
.../ompl_interface/src/detail/constrained_sampler.cpp 59.46% <0.00%> (+16.22%) ⬆️

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 b440c72...89d0af2. Read the comment docs.

Copy link
Copy Markdown
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

I assume the main motivation is:

This simplifies the descriptions in moveit_resources quite a bit because their package name does not match the include path and we have to adapt the names everywhere after edits.

as there isn't anything too 'complex' about the expressions here in the MSA templates.

@v4hn
Copy link
Copy Markdown
Contributor Author

v4hn commented Jul 1, 2021

I assume the main motivation is [...]

That's why I came up with the patch, yes. Also I believe it's the better approach for including launch files in the same folder anyway, but that alone would not justify the template changes.

Thanks for the review.

@v4hn v4hn merged commit 097583d into moveit:master Jul 1, 2021
rhaschke added a commit to ubi-agni/moveit that referenced this pull request Jul 15, 2021
felixvd pushed a commit that referenced this pull request Jul 16, 2021
* create static_transform_publisher for each virtual joint type

* another $(dirname)

augmenting #2748

* formatting
@v4hn v4hn mentioned this pull request Jul 16, 2021
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.

2 participants