-
Notifications
You must be signed in to change notification settings - Fork 425
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
Improve incremental B-spline builder via stochastic optimisation #6066
Conversation
Okay this looks pretty amazing. |
I've attempted to resolve merge conflicts. @OliBomby please check if I did it correctly whenever you have the time. Sometimes if I try really hard I seem to be able to break this: The approximation straight up stops following the path for a bit there. I'm not exactly sure why this is though, it's exceedingly difficult to reproduce, and only happens if I try really dumb stuff, so not sure how much of a blocker this is. Will do a 15-minute pass over the source now, but I don't expect to understand much of this, especially so that this is to be expedited. |
// Make sure the last segment has the correct end-points | ||
if (lastSegment.Count >= 1) | ||
lastSegment[0] = cps[0]; | ||
else | ||
lastSegment.Add(cps[0]); | ||
if (lastSegment.Count >= 2) | ||
lastSegment[^1] = cps[^1]; | ||
else | ||
lastSegment.Add(cps[^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.
What's this doing? I'd hope initializeSegment()
would already enforce this?
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.
initializeSegment()
does not already enforce this because it does not update the lastSegment
, it only returns what would be the initial control points for this segment. lastSegment
is not equal to the initial control points because it already has been optimized for several iterations in the previous frames.
This part of the code is responsible for updating the end-points of lastSegment
to be equal to cps
or adding the end-points if it does not already have them.
// Make a mask to prevent modifying the control points which have already been optimized enough. | ||
// Also the end-points can not move. |
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.
Where is the "have already been optimised enough" part done? I only see the end 2 * degree
control points being fixed in place.
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.
Perhaps this comment is misleading. I deemed control points which are 2 * degree
away from the end as "optimized enough" because they would have barely any impact on the end-point which is the part that has to be updated. There is no actual check to see if they are "optimized enough" as in close to optimal. I chose the 2 * degree
because that seemed like the point where control points stopped moving around a whole lot. I'll change the comment to make this more clear.
btw, the end 2 * degree
control points are the only ones not fixed in place. 0 in this mask means that they are fixed.
public void Finish() | ||
{ | ||
outputCache.Invalidate(); | ||
controlPointsPartiallyInvalid = true; |
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.
What does controlPointsPartiallyInvalid
mean? And why is it true
when done drawing?
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.
controlPointsPartiallyInvalid
means that controlPoints
is valid for the current input path and settings minus some part of the path which was added onto the end. It will then re-use the previous controlPoints
to prevent doing double work on the segments it has already computed control points for, basically only updating the last segment of the controlPoints
.
I set it to true when done drawing because we want to optimize the control points a bit further once we know this is the entire input path. This leads to better results.
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.
I've renamed this flag and a few other things in e7c7397 to hopefully aid in understanding. Please check that you're ok with this.
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.
I think these new names kinda misrepresent the meaning.
The method regenerateLastApproximatedSegment
doesn't regenerate the last segment, because it re-uses the existing control points and updates their position or adds/removes some. Also the effect is not limited to just the last segment. If enough path has been added it could be adding several segments to the path. Saying it 'updates' the control points would be more accurate there. Same thing for shouldOptimiseLastSegment
.
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.
Well what you had was pretty terrible readability-wise (regenerateApproximatedPathControlPoints
vs updateApproximatedPathControlPoints
- what is the difference there? what's the significance of "regenerate" vs "update"? the entirety of the context that the second method only touches the last part is lost).
I dunno. Maybe the new names aren't strictly correct but at least they attempt to explain themselves in some respect. If you can figure out an alternative that is both correct and descriptive then push at your leisure.
I think I found the cause for this issue. In extremely rare cases The fix is pretty simple so i'll fix that right away. |
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.
can't seem to break this any further so /shrug
Summary
This PR proposes an improvement to
IncrementalBSplineBuilder
to make it optimize control point placement to more closely match the input path. It utilizesPiecewiseLinearToBSpline
to iteratively optimize the control point placement while drawing the path, and when a segment is finished it will optimize the segment as a whole a bit further.Furthermore
IncrementalBSplineBuilder.ControlPoints
is now aIReadOnlyList<List<Vector2>>
so each segment is explicitly provided.IncrementalBSplineBuilder.ControlPoints
is now aIReadOnlyList<List<Vector2>>
.IncrementalBSplineBuilder.Finish()
which should be called when you finish drawing.TestSceneInteractivePathDrawing
.learnableMask
toPiecewiseLinearToBSpline
to control which control points to optimize.Requirements
Considerations
IncrementalBSplineBuilder.ControlPoints
could potentially be modified from the outside which would disrupt the builder as it updates from the previous control points instead of completely remaking the control points every time. I don't know if this should be considered as an issue.osu.Framework.Tests_Noks6XgEXm.mp4