refactor: increment multiple keypoints if needed#667
Open
trisceptre wants to merge 5 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates SimpleTimeInterpolator to better handle scenarios where the interpolator needs to advance across multiple keypoints within a single cycle, and relaxes the chase-point “freeze” threshold to reduce freezing behavior.
Changes:
- Increase default
maximumDistanceFromChasePointInMetersfrom0.3to3. - Make
InterpolationResultastaticnested class and simplify empty-list checks withisEmpty(). - Replace single-step keypoint advancement with a loop that can advance across multiple keypoints in one call.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aschokking
reviewed
Mar 28, 2026
trisceptre
changed the title
aschokking
approved these changes
May 29, 2026
| // If the fraction is above 1, it's time to set a new baseline point and start LERPing on the next point | ||
| if (lerpFraction >= 1 && index < keyPoints.size()-1) { | ||
| // What was our target now becomes our baseline. | ||
| while (index < keyPoints.size() - 1) { |
Contributor
There was a problem hiding this comment.
⭐ are there any unit tests that could be added to help validate this logic?
| log.debug("Baseline is now " + baseline.getTranslation2d() | ||
| + " and target is now " + targetKeyPoint.getTranslation2d()); | ||
|
|
||
| log.info("Advancing to next keypoint, new baseline is {} and new target is {}", baseline.getTranslation2d(), targetKeyPoint.getTranslation2d()); |
Contributor
There was a problem hiding this comment.
❔ would we ever have so many keypoints that this logline would become spammy?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are we doing this?
Not too relevant for this year, but incrementing by one keypoint at a time can cause breakage when there is an abundance of keypoints over a short period of time. Additionally, the maximum distance from chase points was changed to 3 so that we won't have to deal with any freezing logic in the interpolator for now.
Whats changing?
Questions/notes for reviewers
How this was tested