Move post-processor params into AIKIDO.

[sniyaz]

A while ago we added post-processor params in libherb with this PR:

https://github.com/personalrobotics/libherb/pull/114

It's time to start moving these into AIKIDO, and this PR starts with the post-processor parms structs. This makes a bunch of new things possible and lets users specify exactly how they'd like to post-process a path. This is relevant, for example, to #558.

This PR does three key things:

1. Move these param structs into AIKIDO.
2. Add a getTrajectoryPostProcessor method to ConcreteRobot that takes param struct in and returns the corresponding post-processor.
3. Add a postProcessPath method to ConcreteRobot that takes param struct in and post-processes according to those params.

TODO

Once this PR merges, update libherb and herb3_demos to use this code instead of what lives there now.

---

Before creating a pull request

• Document new methods and classes
• Format code with make format

Before merging a pull request

• Set version target by selecting a milestone on the right side
• Summarize this change in CHANGELOG.md
• Add unit test(s) for this change

[brianhou]

I'd still prefer defining the parameters closer to the actual planner / postprocessor. I think your previous argument was that it was convenient to have all the parameters for one robot in one place that could be easily tweaked, but that isn't quite as convincing to me now that we're defining these default parameters in AIKIDO. What do you think?

[sniyaz]

@brianhou That sounds fair. Would moving the new structs to a location like /include/aikido/planner/PostProcessorParams.hpp sound better to you?

[sniyaz]

Whoops, bumbling finger accidentally closed this, please ignore that 😅

[brianhou]

I was thinking even closer, like defining the Hauser parameter struct in the same file as the actual postprocessors.

Then, I think we'll need to figure out how to register/construct these parameter structs. For starters, that could be just moving the current PostProcessorParams class to aikido/planner/PostProcessorParams.hpp as you suggested.

[sniyaz]

@brianhou Sweet, that sounds like a good plan! I'll go with that then.

[sniyaz]

@brianhou Can you take a look and see if this new structure is OK with you? I'm working on the libherb PR right now 😄

[egordon]

I would recommending implementing this as a template class PostProcessorParams<T>.

Where T is of type aikido::planner::TrajectoryPostProcessor (you can enforce this with static_assert, see here)

In the PostProcessor Classes:
Move the structs HauserParams and KunzParams inside their respective classes, and just call them struct Params, giving them the same default values as the constructor.
Additionally, add the constructor (Eigen::VectorXd _velocityLimits, Eigen::VectorXd _accelerationLimits, Params params) to those postprocessors that you can call from getTrajectoryPostProcessor

In PostProcessorParams:
Have the single constructor be PostProcessorParams(T::Params params = T::Params()) : mParams(params) {}, ensuring that this is only called with PostProcessors that have the Params struct will be enforced at compile-time.

Then you can get rid of PostProcessorType and run-time checks, as getTrajectoryPostProcessor can just be a template function, enforced at compile-time.

Have the public getter function T::Params getParams() access the private variable T::Params mParams.

You can make T a default class (see here) for convenience.

[egordon]

Switching to Request changes since I think we should get this templated before merging.

[sniyaz]

@egordon @brianhou Just as a heads up, I just tested this using soda_handoff and things seem to build and work fine 👍

[egordon]

Looks good in general, thanks! Just a few nits (won't approve since it will just get overridden when you address them.

[sniyaz]

@egordon I think this is ready to merge, can you take a look when you have the chance?

[brianhou]

Thanks for doing this! I left a few small comments.

Looking forward, I know we have a bunch of cruft from not having this dispatch function previously. Aside from the libherb and libada compatibility issues, I think we can now:

• Delete smoothPath, retimePath, and retimePathWithKunz, in favor of the postProcessPath function you've added here.
• Delete the previous version of getTrajectoryPostProcessor that's hardcoded to return a ParabolicSmoother.

What do you think?

[sniyaz]

@brianhou I think the nits should be responded to, and PostProcessorParams has been killed. LMK if things look good to merge!

Also, my plan to hold off on killing the old post-processing methods (like you suggested) in a separate set of PRs up and through herb3_demos (I'll work on those ASAP once these get merged). My thinking is that we're probably going to break lots of things, and I want to keep these PR sizes more manageable. LMK if that sounds good to you!

[brianhou]

It seems that libherb could use getTrajectoryPostProcessor(metaSkeleton, params) if it existed. Can we add it just for convenience? (And again, it could just dispatch to the more explicit getTrajectoryPostProcessor.)

Aside from that and one other nit, looks pretty good to me. @egordon can you take one final look?

[sniyaz]

@brianhou @egordon Alright, think we're done done now 😅 LMK if anything else looks wrong, and thanks so much again for reviewing!

Resolved.