Added Rational Curves #1
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
Co-authored-by: JohnTheCoolingFan <[email protected]>
|
I would like to wait for @NthTensor to review as well before merging |
|
Looking it over now. |
| fn segment(&self, t: f32) -> (&RationalSegment<P>, f32) { | ||
| if self.segments.len() == 1 { | ||
| (&self.segments[0], t) | ||
| } else { | ||
| let i = (t.floor() as usize).clamp(0, self.segments.len() - 1); | ||
| (&self.segments[i], t - i as f32) | ||
| } | ||
| } |
There was a problem hiding this comment.
Segments should be determined by spans in the knot vector.
Each successive pair of knots k_i, k_i+1 represents the interval [k_i, k_i+1) on which the input t maps to piecewise segment i. In the event than k_i = k_i+1, the span is zero and we skip segment i.
We have rationality, but we still have to get non-uniformity right.
There was a problem hiding this comment.
Ah, I thought the matrix already encoded that information.
| /// | ||
| /// Segments can be chained together to form a longer compound curve. | ||
| #[derive(Clone, Debug, Default, PartialEq)] | ||
| pub struct RationalSegment<P: Point> { |
There was a problem hiding this comment.
We should probably also add a span: Range<f32> here derived from the knot value, then use it to look up the appropriate segment.
| #[inline] | ||
| fn to_curve(&self) -> RationalCurve<P> { | ||
| let segments = self | ||
| .control_points.windows(4) |
There was a problem hiding this comment.
In some cases, as when the knot vector has multiplicity at the ends, we may actually need to use the same control point multiple times.
| @@ -427,11 +430,12 @@ impl<P: Point> CubicNurbs<P> { | |||
|
|
|||
There was a problem hiding this comment.
I think we also need to normalize the knot vector so that it covers the interval [0, n], because this is the domain of our parameterized curve.
There was a problem hiding this comment.
Ok this is a tremendous, fantastic improvement but there are still seem to be issues remaining with the knot vector, segmentation, and parameterization.
I think the best option would be to merge this first, then do the fixes. I can PR this myself, but if either of you prefer to tackle this I'm happy to provide a more comprehensive description of the problem.
| @@ -913,4 +1173,42 @@ mod tests { | |||
| assert!(bezier.ease(0.5) < -0.5); | |||
| assert_eq!(bezier.ease(1.0), 1.0); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
This is a really good test. Ideally, I would like at-least one more using comparing points on a nonuniform, weighted, 3d curve against "ground-truth" data computed using tinyspline (with appropriate deltas to accommodate floating point error).
There was a problem hiding this comment.
Yeah I agree a ground-truth test is also needed, but I felt that was over the scope of what I was doing.
There was a problem hiding this comment.
You can either add it yourself when this gets merged, or if you have a test-case in mind, send me the data and I'll put it in.
There was a problem hiding this comment.
Yep, just noting what I'm sure Alice will want. We should totally merge this into the PR first.
|
Awesome review @NthTensor. Tell me when you're done with this guys and I'll merge and we can continue with other things. |
JohnTheCoolingFan
merged commit c9a2f19
into
JohnTheCoolingFan:cubic-splines-overhaul
…3905) # Objective - first part of bevyengine#13900 ## Solution - split `check_light_mesh_visibility `into `check_dir_light_mesh_visibility `and `check_point_light_mesh_visibility` for better review
Objective
Solution
RationalCurveandRationalSegmentand use them to implement NURBS curves.Changelog
Added
RationalCurveandRationalSegment, made NURBS curves a RationalGenerator, added aFrom<CubicCurve> for RationalCurveimpl and added a test for it.