-
Notifications
You must be signed in to change notification settings - Fork 84
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
Interpolating spline #141
Interpolating spline #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I do have a question on that shift from @
to *
in the animation base utils. Besides that, few comments on edge case handling and reusing visualization code, and obligatory request for docstrings everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being nitty since it's a public repo (right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff, only thing that might possibly be an actual issue is type hinting with set
.
spatialmath/spline.py
Outdated
interpolation_indices.remove(0) | ||
interpolation_indices.remove(len(self.pose_data) - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use range(1, len(self.pose_data) - 1)
? Or `range(len(self.pose_data))[1:-1]? The current way seems a bit unconventional but I guess it doesn't matter.
spatialmath/spline.py
Outdated
interpolation_indices.remove(0) | ||
interpolation_indices.remove(len(self.pose_data) - 1) | ||
|
||
for _ in range(len(self.time_data) - 2): # you must have at least 2 indices |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well use len(interpolation_indices)
and drop the - 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the length of interpolation indices is changing as the loop executes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your suggestion might work anyway, but that's why i didn't code it like that originally
Adds functionality for generating a dense SE3 trajectory interpolated from sparse waypoints. Includes a SplineFit class to downsample the waypoints.
Ideally this SplineFit class would also work with the BSplineSE3 class, but I'm going to leave that for another day.