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

Remove Iterative Spline and Iterative Parabola time-param algorithms #1191

Closed
wants to merge 3 commits into from

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Apr 21, 2022

We've been using TOTG by default for awhile now. The only drawback of TOTG I'm aware of is that it can't handle nonzero initial or final velocities/accelerations. So I was thinking that would be a reason to keep the older algorithms (ISTP and IPTP). But, I added a little test and it looks like neither of those algorithms handles it either. Even worse -- they just fail without throwing any error. So I think we should delete them.

3 commits:

  • The first commit adds a simple test for IPTP. The test passes but the output is wrong. (It didn't do anything.)
20: [ RUN      ] TestTimeParameterization.NonzeroInitialVelAccel
20: Trajectory has 2 points over 0.000 seconds
20:   waypoint   0 time 0.000 pos  0.000 vel  0.000 acc  0.000
20:   waypoint   1 time 0.000 pos  0.000 vel  0.000 acc  0.000
20: [       OK ] TestTimeParameterization.NonzeroInitialVelAccel (0 ms)
  • The second commit adds a simple test for ISTP. Again, the test passes but the output is wrong. The state at the final waypoint should be (position, vel, accel) == (0.01, 0.1, 0.2).
20: [ RUN      ] TestTimeParameterization.NonzeroInitialVelAccel
20: Trajectory has 4 points over 0.343 seconds
20:   waypoint   0 time 0.000 pos  0.000 vel  0.000 acc  0.742
20:   waypoint   1 time 0.118 pos  0.003 vel  0.021 acc -0.394
20:   waypoint   2 time 0.253 pos  0.005 vel  0.029 acc  0.526
20:   waypoint   3 time 0.343 pos  0.010 vel  0.100 acc  1.036
20: [       OK ] TestTimeParameterization.NonzeroInitialVelAccel (0 ms)
  • The third commit deletes both of the plugins.

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #1191 (d46389a) into main (6b985cc) will decrease coverage by 0.65%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1191      +/-   ##
==========================================
- Coverage   61.32%   60.68%   -0.64%     
==========================================
  Files         274      270       -4     
  Lines       24766    24318     -448     
==========================================
- Hits        15186    14754     -432     
+ Misses       9580     9564      -16     
Impacted Files Coverage Δ
...eit_core/robot_trajectory/src/robot_trajectory.cpp 52.54% <0.00%> (-11.39%) ⬇️
...include/moveit/robot_trajectory/robot_trajectory.h 98.65% <0.00%> (-0.03%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 46.50% <0.00%> (+0.45%) ⬆️

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 6b985cc...d46389a. Read the comment docs.

@v4hn
Copy link
Contributor

v4hn commented Apr 21, 2022 via email

@AndyZe
Copy link
Member Author

AndyZe commented Apr 21, 2022

What do we gain

@v4hn
Copy link
Contributor

v4hn commented Apr 28, 2022

I just noticed my feedback by mail never made it here! 😠 Here's a copy:

Thank you for answering the question. 👍 I know I'm being annoying by
opposing various remove-that-code initiatives lately.

  • Nothing unexpected happens if somebody tries to execute one of these
    plugins on a real robot

If someone explicitly uses a non-default they should be looking for different
behavior explicitly.
Please note that TOTG is also (by far) not the perfect dynamics
parameterization you make it sound like.
We had some long discussions on that here and here.

The API changes we've been talking about don't need to be added to these
older plugins --> less maintenance
#1180 (comment) #1186

True, though I believe it would be perfectly justified to add new arguments to
the callback and ignore them or warn if they are set.
And that takes 5 minutes to implement and test. :)

  • Slightly faster CI run times and build times for MoveIt users

I'm not sure this is relevant. From
moveit/moveit#2081 it looks like the
longest-compiling method in this code part took 300ms to compile. So you might
gain a second.

@AndyZe
Copy link
Member Author

AndyZe commented Apr 28, 2022

I'm going to go ahead and close this, then. Let me know if you have second thoughts. (I'll likely re-open a PR to make TOTG the default everywhere)

@AndyZe AndyZe closed this Apr 28, 2022
@v4hn
Copy link
Contributor

v4hn commented Oct 11, 2022 via email

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