moveit2 icon indicating copy to clipboard operation
moveit2 copied to clipboard

Construct JointModelGroup with ranges-v3

Open tylerjw opened this issue 3 years ago • 9 comments

Description

I know this probably won't pass for the same reason the other PRs aren't passing CI. I don't understand the issue with the Ruckig test or what is causing it to fail. While investigating that I discovered this constructor as I was trying to understand how some of these values are initialized. I believe this is much-much easier to read than the nested for loops. I should be able to move all of these into the initializer list and out of the body of the constructor too, I'm just done hacking on this tonight.

tylerjw avatar Jan 25 '22 04:01 tylerjw

I broke this into separate small commits to hopefully make it easier to review.

tylerjw avatar Jan 26 '22 04:01 tylerjw

One thing I did that I'm not sure I like is manually formatted much of this code and disabled clang-format for the block. I did this because I think it makes it much more readable than the way our clang-format is configured for these huge blocks of initialization via ranges.

tylerjw avatar Jan 26 '22 04:01 tylerjw

Before someone asks I did try putting this all in the initializer list and determined that it was basically unreadable. The whole point of this change is to make it easier to reason about what is in these containers. This is the same reason I disabled clang-format and manually formatted this block.

tylerjw avatar Jan 26 '22 04:01 tylerjw

Codecov Report

Merging #1029 (7bfcd07) into main (77e43f3) will increase coverage by 0.12%. The diff coverage is 85.59%.

:exclamation: Current head 7bfcd07 differs from pull request most recent head a197bbd. Consider uploading reports for the commit a197bbd to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
+ Coverage   61.03%   61.14%   +0.12%     
==========================================
  Files         275      275              
  Lines       23739    23786      +47     
==========================================
+ Hits        14486    14541      +55     
+ Misses       9253     9245       -8     
Impacted Files Coverage Δ
...include/moveit/robot_trajectory/robot_trajectory.h 98.71% <ø> (ø)
...cessing/src/time_optimal_trajectory_generation.cpp 76.12% <0.00%> (ø)
moveit_core/robot_state/src/robot_state.cpp 48.05% <69.24%> (+0.04%) :arrow_up:
moveit_core/robot_model/src/joint_model_group.cpp 66.00% <86.60%> (+5.14%) :arrow_up:
...del/include/moveit/robot_model/joint_model_group.h 87.18% <100.00%> (ø)
...eit_core/robot_trajectory/src/robot_trajectory.cpp 64.46% <100.00%> (ø)
...ocessing/src/iterative_spline_parameterization.cpp 85.16% <100.00%> (ø)
...processing/src/iterative_time_parameterization.cpp 93.24% <100.00%> (ø)
...rajectory_processing/src/ruckig_traj_smoothing.cpp 80.62% <100.00%> (ø)
... and 1 more

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 dd240ef...a197bbd. Read the comment docs.

codecov[bot] avatar Jan 26 '22 22:01 codecov[bot]

One thing nice is that almost all of the code in this file is covered by existing tests. While those tests might not be perfect they should give us more confidence that these changes do not change the behavior of this code.

tylerjw avatar Jan 26 '22 23:01 tylerjw

Before someone asks I did try putting this all in the initializer list and determined that it was basically unreadable. The whole point of this change is to make it easier to reason about what is in these containers. This is the same reason I disabled clang-format and manually formatted this block.

I would have asked about the initializer list but I really think that would require breaking this up into smaller objects which I realize is not the point of this PR.

griswaldbrooks avatar Jan 27 '22 01:01 griswaldbrooks

I still don't really know what's going on with joint_variables_index_map_ and some of the others in that part of the code (feels like some intermediate named variables or lambas could help).

That is a good point. I updated by adding some intermediate named variables and adding comments. Let me know if that makes it clearer.

tylerjw avatar Jan 27 '22 03:01 tylerjw

This pull request is in conflict. Could you fix it @tylerjw?

mergify[bot] avatar Apr 12 '22 00:04 mergify[bot]

This pull request is in conflict. Could you fix it @tylerjw?

mergify[bot] avatar Sep 27 '22 22:09 mergify[bot]

Closing as this is abandoned work.

tylerjw avatar Nov 17 '22 21:11 tylerjw