Skip to content

Conversation

@rjoomen
Copy link
Contributor

@rjoomen rjoomen commented Mar 20, 2024

Profile Points Positions IK Remarks
SimplePlannerFixedSizeAssignMoveProfile Fixed size Assign Yes New
SimplePlannerFixedSizeAssignNoIKMoveProfile Fixed size Assign No Renamed
SimplePlannerFixedSizeMoveProfile Fixed size Interpolate Yes
N/A Fixed size Interpolate No
SimplePlannerLVSAssignMoveProfile LVS Assign Yes New
SimplePlannerLVSAssignNoIKMoveProfile LVS Assign No New
SimplePlannerLVSMoveProfile LVS Interpolate Yes
SimplePlannerLVSNoIKMoveProfile LVS Interpolate No

@rjoomen rjoomen requested a review from Levi-Armstrong March 20, 2024 06:12
@rjoomen
Copy link
Contributor Author

rjoomen commented Mar 20, 2024

Still have to add unit tests.

@codecov
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 93.97539% with 142 lines in your changes missing coverage. Please review.

Project coverage is 79.96%. Comparing base (105a8a2) to head (bcd51d0).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...eract_motion_planners/simple/src/interpolation.cpp 41.17% 20 Missing ⚠️
...est/simple_planner_fixed_size_assign_move_unit.cpp 93.99% 20 Missing ⚠️
...mple_planner_fixed_size_assign_no_ik_move_unit.cpp 92.59% 18 Missing ⚠️
...imple/test/simple_planner_lvs_assign_move_unit.cpp 95.89% 18 Missing ⚠️
...simple/test/simple_planner_lvs_no_ik_move_unit.cpp 96.05% 16 Missing ⚠️
...test/simple_planner_lvs_assign_no_ik_move_unit.cpp 96.43% 12 Missing ⚠️
...profile/simple_planner_lvs_assign_move_profile.cpp 86.11% 10 Missing ⚠️
...e_planner_fixed_size_assign_no_ik_move_profile.cpp 85.48% 9 Missing ⚠️
...e/simple_planner_lvs_assign_no_ik_move_profile.cpp 85.93% 9 Missing ⚠️
...imple/test/simple_planner_fixed_size_move_unit.cpp 96.47% 8 Missing ⚠️
... and 1 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
+ Coverage   77.36%   79.96%   +2.60%     
==========================================
  Files         237      248      +11     
  Lines       14338    16349    +2011     
==========================================
+ Hits        11092    13074    +1982     
- Misses       3246     3275      +29     
Files with missing lines Coverage Δ
...le/simple_planner_fixed_size_assign_move_profile.h 100.00% <ø> (ø)
...ple_planner_fixed_size_assign_no_ik_move_profile.h 100.00% <100.00%> (ø)
...e/profile/simple_planner_fixed_size_move_profile.h 100.00% <ø> (ø)
...e/profile/simple_planner_lvs_assign_move_profile.h 100.00% <100.00%> (ø)
...ile/simple_planner_lvs_assign_no_ik_move_profile.h 100.00% <100.00%> (ø)
...profile/simple_planner_fixed_size_move_profile.cpp 73.68% <100.00%> (ø)
...le/src/profile/simple_planner_lvs_move_profile.cpp 81.81% <100.00%> (ø)
.../profile/simple_planner_lvs_no_ik_move_profile.cpp 81.81% <100.00%> (ø)
...nners/simple/test/simple_planner_lvs_move_unit.cpp 95.75% <100.00%> (ø)
...planners/simple/test/simple_planner_test_utils.hpp 100.00% <100.00%> (ø)
... and 11 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rjoomen rjoomen changed the title Add SimplePlannerLVSAssignPlanProfile Add SimplePlannerLVSAssignPlanProfile, SimplePlannerLVSAssignNoIKPlanProfile, SimplePlannerFixedSizeAssignPlanProfile (with IK), rename SimplePlannerFixedSizeAssignNoIKPlanProfile May 29, 2024
@rjoomen rjoomen requested review from Levi-Armstrong and removed request for Levi-Armstrong May 29, 2024 15:06
@rjoomen rjoomen force-pushed the splapp branch 4 times, most recently from 1e58b21 to 6989d88 Compare June 3, 2024 19:15
@rjoomen rjoomen force-pushed the splapp branch 2 times, most recently from fa81c1b to 2c792af Compare June 14, 2024 04:48
@rjoomen rjoomen force-pushed the splapp branch 2 times, most recently from 12bed9c to 8d4374b Compare December 4, 2024 10:33
@rjoomen rjoomen force-pushed the splapp branch 2 times, most recently from 30730aa to 38a5580 Compare January 14, 2025 18:58
@rjoomen rjoomen force-pushed the splapp branch 2 times, most recently from 9fe5e81 to dc42fff Compare January 22, 2025 07:54
@rjoomen rjoomen force-pushed the splapp branch 2 times, most recently from f0c1488 to 75433c3 Compare February 25, 2025 07:38
@Levi-Armstrong
Copy link
Contributor

@rjoomen What are the next steps needed to get this merged?

@rjoomen
Copy link
Contributor Author

rjoomen commented Mar 20, 2025

I need to add unit tests for the new profiles. I hope to finish this soon.

@Levi-Armstrong
Copy link
Contributor

@rjoomen Are you still working on unit tests before this is ready for review? I just want to make sure you are not waiting on me.

@rjoomen
Copy link
Contributor Author

rjoomen commented Jul 16, 2025

I just finished this today, and it's finally ready for review!

@Levi-Armstrong
Copy link
Contributor

@rjoomen The only thing I see which is the existing SimplePlannerFixedSizeAssignMoveProfile was renamed SimplePlannerFixedSizeAssignNoIKMoveProfile but there still exist SimplePlannerFixedSizeAssignMoveProfile which runs IK so those that were using the existing SimplePlannerFixedSizeAssignMoveProfile will not know they are getting different behavior. Do you agree?

@rjoomen
Copy link
Contributor Author

rjoomen commented Jul 16, 2025

Yes, that is a good point. The new situation is named consistently, but might cause running IK when not expected if you use the old profile name assuming it does not do IK.
Is this a problem though? I suppose normally one would only use the NoIK variant if you have proper seed values, so using the IK variant instead would not run IK anyway (right? I'm just assuming, I didn't check). But if this is an undesirable risk, we should rename SimplePlannerFixedSizeAssignMoveProfile, probably to SimplePlannerFixedSizeAssignIKMoveProfile or something, but for consistency we then should do this with all IK profiles. This would make the long and clunky names even longer, and the difference smaller as they have both IK in them. Although on the other hand it is explicit then that !NoIK = IK.
What do you think?

@Levi-Armstrong
Copy link
Contributor

I do not think anything needs to change. Its just good to make note here so it is documented.

@Levi-Armstrong Levi-Armstrong merged commit 09c3f78 into tesseract-robotics:master Jul 16, 2025
13 of 17 checks passed
@rjoomen rjoomen deleted the splapp branch July 21, 2025 06:03
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