moveit2
moveit2 copied to clipboard
Construct JointModelGroup with ranges-v3
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.
I broke this into separate small commits to hopefully make it easier to review.
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.
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.
Codecov Report
Merging #1029 (7bfcd07) into main (77e43f3) will increase coverage by
0.12%. The diff coverage is85.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
@@ 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 dataPowered by Codecov. Last update dd240ef...a197bbd. Read the comment docs.
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.
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.
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.
This pull request is in conflict. Could you fix it @tylerjw?
This pull request is in conflict. Could you fix it @tylerjw?
Closing as this is abandoned work.