-
Notifications
You must be signed in to change notification settings - Fork 69
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
DiscreteTrajectorySegment, part 4: downsampling #3150
Conversation
template<typename Frame> | ||
void DiscreteTrajectorySegment<Frame>::SetDownsampling( | ||
DownsamplingParameters const& downsampling_parameters) { | ||
// TODO(phl): Do we need this precondition? |
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.
If we have ClearDownsampling
we might as well be able to change it directly (but the semantics of doing either are unclear if there is any actual downsampling going on, see my comment in the .h).
|
||
// Clear the downsampling parameters. From now on, all points appended to the | ||
// segment are going to be retained. | ||
void ClearDownsampling(); |
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 semantics of setting and clearing the downsampling on a segment with more than one point are unclear, and the comments fail to clarify this.
Clearing the downsampling causes points to be retained, but what happens when you reset the downsampling (or when you set it on a trajectory which never had downsampling) ? Are those points stuck in their dense state forever, or does it commute with appending ? If they are stuck in their dense state forever, do we have a way to identify those points ?
I would recommend forbidding mutations of the downsampling parameters on a segment whose size is greater than 1, otherwise the semantics are going to be unwieldy to define and think about.
#3136.