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

Create a WaypointTrajector class #312

Closed
dqyi11 opened this issue Jan 24, 2018 · 4 comments
Closed

Create a WaypointTrajector class #312

dqyi11 opened this issue Jan 24, 2018 · 4 comments
Labels

Comments

@dqyi11
Copy link
Contributor

dqyi11 commented Jan 24, 2018

Currently both the Spline class and the Interpolated class have a few methods for "waypoints", which are getNumWaypoints(), getWaypoint(i) and getWaypointTime(i), which are not inherited from the base class Trajectory.
Those methods are mostly called in converting to some implicit path format, for example DynamicPath in computeParabolicTiming().

@brianhou suggested creating an abstract class WaypointTrajectory (in between base class Trajectory and Interpolated \ Spline) and defining all the "waypoints" operations in this class.
The change will merge conversions based on "waypoints" from both Interpolated and Spline into a single method, which will remove duplicate code.

A very minor concern is that whether it is fair to consider Spline as a WaypointTrajectory.
Because Spline is a sequence of line segments.
The "waypoints" of a Spline is "indirectly" obtained from segments.
https://github.com/personalrobotics/aikido/blob/master/src/trajectory/Spline.cpp#L206
https://github.com/personalrobotics/aikido/blob/master/src/trajectory/Spline.cpp#L226

We would like to hear some comments before making the change.

@psigen
Copy link
Member

psigen commented Jan 25, 2018

Quick question: Can Spline be implemented as a type of interpolator for Interpolated?

If not, is there a way to generalize it such that it can? Because it seems like the Spline definition is really implementing spline interpolation, since it's not like it's defining control points or some sort of inherent spline parameterization -- it's just finding a spline that passes through the set of waypoints, right?

@brianhou
Copy link
Contributor

I was wondering the same thing! After some investigation, I think the (current) answer is no because each segment of the spline has different polynomial coefficients and Interpolator::interpolate(State* from, State* to, double alpha, State* state) doesn't provide a way to pass those in.

Maybe it would be possible to model a Spline trajectory as a sequence of Interpolated segments, where each segment has its own PolynomialInterpolator? I'm not sure if that would be useful either.

@psigen
Copy link
Member

psigen commented Jan 25, 2018

Yeah, I suppose it gets a bit weird once you have each segment need different interpolation parameters. One thing that's kind of interesting though is that if you have fixed order of the segments of the spline, and you have specified the start and end waypoints to the order/2-th derivative, then this is a fully constrained solution and you no longer need the polynomial coefficients.

Our existing Spline class is somewhat more general here, but it's interesting that for a certain subset of waypoints and fixed-order splines, this should be do-able.

@mkoval
Copy link
Member

mkoval commented Feb 2, 2018

Maybe it would be possible to model a Spline trajectory as a sequence of Interpolated segments, where each segment has its own PolynomialInterpolator? I'm not sure if that would be useful either.

This is the way I would approach it, if we decide to go this way. The spline parameters are an implementation detail of the interpolator - not a formal argument required to evaluate the trajectory.

The bigger issue is that Interpolated and Interpolator currently only allows zero velocity waypoints. We often create Splines with non-zero velocity at waypoints, e.g. inside the Hauser parabolic smoother. If we wanted to do this, we would have to relax this limitation. E.g. change the signature to:

I don't see a huge amount of value in this change, so I suggest leaving things as they are for now.

@brianhou brianhou closed this as completed Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants