Make TimeParamerization configurable

[rhaschke]

Replaces #333, implementing #333 (review). Fixes #330.
The TimeParameterization method becomes a property in corresponding stages and PlannerInterfaces.
This allows users to configure them easily.

[codecov]

Codecov Report

Merging #339 (3a3da5b) into master (ca38d11) will decrease coverage by 0.03%.
The diff coverage is 66.67%.

@@            Coverage Diff             @@
##           master     #339      +/-   ##
==========================================
- Coverage   54.59%   54.56%   -0.02%     
==========================================
  Files         102      102              
  Lines        7832     7847      +15     
==========================================
+ Hits         4275     4281       +6     
- Misses       3557     3566       +9     

Impacted Files	Coverage Δ
core/src/container.cpp	73.87% <0.00%> (-0.37%)	⬇️
core/src/merge.cpp	66.25% <100.00%> (-0.41%)	⬇️
core/src/solvers/cartesian_path.cpp	83.79% <100.00%> (ø)
core/src/solvers/joint_interpolation.cpp	87.76% <100.00%> (ø)
core/src/solvers/planner_interface.cpp	100.00% <100.00%> (ø)
core/src/stages/connect.cpp	89.52% <100.00%> (+0.26%)	⬆️
core/src/stages/fixed_state.cpp	95.46% <0.00%> (-4.54%)	⬇️
core/src/storage.cpp	82.79% <0.00%> (-2.25%)	⬇️

---

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 ca38d11...3a3da5b. Read the comment docs.

[rhaschke]

This commit finally breaks compatibility to older MoveIt releases, namely Melodic.
Actually, the polymorphic base class TimeParameterization is available in MoveIt 1.0.9 as well!

[v4hn]

Mostly looks good. I'll leave it to you whether you want to deal with the missing pointers in upstream before merging.

I would be happier with this if ...

• the time parameterization were an actual plugin in MoveIt
• if the triple <plugin, vel_scaling, acc_scaling> would be somehow one property instead of three needed in multiple places
• if we could add additional string overloads to load these properties instead of having to pass shared_ptr.

[v4hn]

This can (and could before) break a lower specified dynamics limit in one of the merged trajectories. The merged trajectory might suddenly be "inexplicably" faster.

But the real solution to this is not adding yet another set of parameters, but implement a "proper" merge, that does not need to compute a whole new timing for the trajectory, but resamples the trajectories as needed.
So I guess it's best to leave this as is until then.

[rhaschke]

Please file an issue as a reminder...

[v4hn]

I just saw you backported the polymorphic base class change to melodic. So this should be unnecessary.
But Jafar and you both did not notice the missing pointers in the original patch, so I just filed this PR. It might be worth creating a patch release for melodic with this addition as well. If you should do that, these lines can go away

Suggested change

	#if !MOVEIT_VERSION_GE(1, 1, 9)
	// the pointers are not yet available
	namespace trajectory_processing {
	MOVEIT_CLASS_FORWARD(TimeParameterization);
	}
	#endif

[rhaschke]

Until we have a new release of MoveIt, this addition is required. Do you propose a patch release of MoveIt before?
I could do that over the weekend indeed...

[v4hn]

I guess you can remove this now. :)

[rhaschke]

I would be happier with this if ...

• the time parameterization were an actual plugin in MoveIt

It was consensus to not use plugins for these (although I proposed plugins originally)
See #333

• if the triple <plugin, vel_scaling, acc_scaling> would be somehow one property instead of three needed in multiple places

I can understand you somehow. But, that would make it impossible for the user to change a single of these properties.

• if we could add additional string overloads to load these properties instead of having to pass shared_ptr.

String names are more brittle and cannot be checked at compile time.