[WIP] Cubic bezier min bounding rect #97

o0Ignition0o · 2017-06-28T12:37:58Z

Went with an implementation so far but my unit test is not passing. I think I don't correctly understand what an inflection point is. Is my setup phase incorrect or is it the way I implement the method?

nical · 2017-06-28T14:21:17Z

Ooops! There's been some confusion about the inflection point indeed: Inflection points are points where the curve goes from turning in a direction to turning in another direction (my mental model of it is to imagine a bike riding along a curve and at some point the handlebars go from pointing to the left to pointing to the right).
X-inflection points are where the curve transitions from increasing along the x axis to decreasing and vis-versa.
What we are looking for in find_x_minimum/maximum is a local extremum of the curve along x, but the curves inflection points don't give us that (they give us points where the curvature is null), whereas x-inflection and y-inflection points would give us what we need.
So the find_x/y_minimum/maximum methods are not actually correct.

Good thing you are principled about unit testing and caught that!

nical · 2017-06-28T14:30:18Z

To calculate the x-inflection points we need to solve the equation of the derivative dx/dt = 0 with x(t) = (1 - t)³ * from.x + 3 * (1 - t)² * t * ctrl1.x + 3 * t² * (1 - t) * ctrl2.x + t³ * to.x.

o0Ignition0o · 2017-06-28T14:52:53Z

Thanks a lot for your comment, I'll try to make the required changes asap ! Btw this probably means I'll have to do a followup on the Quadratic curves too isn't it ?

nical · 2017-06-28T14:58:47Z

Cool, thanks a lot for looking into it!
I think that the quadratic bézier curve code is correct because x-/y-inflections points were computed using derivatives of x and y. Quadratic béziers don't have the other type of inflection point (if the curve starts turning to the left it always turns to the left and vis-versa, or in other words the sign of the curvature never changes) so there is less room for confusion.

o0Ignition0o · 2017-06-28T15:04:05Z

Yeah indeed, I noticed my mistake as I just went through the code again, thanks for the hint ! :)

o0Ignition0o · 2017-07-01T11:01:52Z

Ok I think I've found a way to solve this on paper, I'm gonna try to solve it in rust :)

o0Ignition0o · 2017-07-03T19:01:47Z

Hey @nical , my find_y_extremums unit test is passing, but I don't understand if it should, and why it does, mind giving me a hint on how I'm supposed to proceed plz ? thanks a lot ^^'

nical

The logic in find_y_extremum looks good although it should be renamed into find_x_inflections, and an equivalent method is needed for y. find_x/t_minimum/maximum need to be fixed up so that they use this instead of the general inflection points, and then things should be good!

nical · 2017-07-03T20:11:41Z

bezier/src/cubic_bezier.rs

+    /// Return local y extremum points or None if this curve is monotone.
+    ///
+    /// This returns the advancements along the curve, not the actual y position.
+    pub fn find_y_extremums(&self) -> UpToTwo<f32> {


In order to have a coherent API with the quadratic bézier equivalent, this should be called find_y_inflections

We also need an equivalent method for x.

nical · 2017-07-03T20:15:43Z

bezier/src/cubic_bezier.rs

    pub fn find_x_minimum(&self) -> f32 {
        let mut min_t = 0.0;
        let mut min_x = self.from.x;
        if self.to.x < min_x {
            min_t = 1.0;
            min_x = self.to.x;
        }
-        for t in self.find_inflection_points() {


You should keep this loop, but replace find_inflection_points with find_x_inflections. Otherwise, if the control point is left of the start and end points (in which case the curve will go more to the left of from and to), this would not return the correct result.
find_x_maximum and find_y_minimum/maximum also need to be fixed up to use find_x/y_inflections instead of find_inflection_points`.

nical · 2017-07-03T20:17:05Z

bezier/src/cubic_bezier.rs

+        let min_x = self.sample(self.find_x_minimum()).x;
+        let max_x = self.sample(self.find_x_maximum()).x;
+        let min_y = self.sample(self.find_y_minimum()).y;
+        let max_y = self.sample(self.find_y_maximum()).y;


A little detail, but you can write self.sample(self.find_x_minimum()).x into self.sample_x(self.find_x_minimum()); to avoid computing y (not extremely important since the code is already correct).

nical · 2017-07-03T20:17:57Z

bezier/src/cubic_bezier.rs

-            ctrl2: ctrl3a,
-            to: to,
-        });
+                CubicBezierSegment {


nit: Not sure why this is getting an extra indentation level.

nical · 2017-07-03T20:25:58Z

bezier/src/cubic_bezier.rs

+        let mut ret = UpToTwo::new();
+        // See www.faculty.idc.ac.il/arik/quality/appendixa.html for an explanation
+        // The derivative of a cubic bezier curve is a curve representing a second degree polynomial function
+        // f(x) = a * x² + b * x + c such as :


To avoid confusion, let's say that it is the derivative of the curve projected on the y axis, and use y as the variable in the formula: f(y) = a * y² + b * y + c (because we also need the equivalent for the x axis).

nical · 2017-07-03T20:39:26Z

bezier/src/cubic_bezier.rs

+    };
+
+    let mut expected_y_extremums = UpToTwo::new();
+    expected_y_extremums.push(0.5);


I don't think that 0.5 is an y-inflection point of the curve (although it is an inflection point). After drawing it on a piece of paper it looks like this curve is monotone (always increasing) along y.
I think that this curve should have no y-inflections (and also no x-inflections), even if, confusingly, 0.5 is a "normal" inflection point.

nical · 2017-07-03T20:43:50Z

bezier/src/cubic_bezier.rs

+        let discriminant_sqrt = discriminant.sqrt();
+        println!("delta_sqrt : {} ", discriminant_sqrt);
+
+        let first_extremum = (-b - discriminant_sqrt) / 2.0 * a;


It's easy to get confused about the precedence of operators. In order to avoid ambiguity, let's put some parentheses around 2.0 * a here and below.

o0Ignition0o · 2017-07-03T20:59:18Z

Thanks a lot for taking time to review my code, I'll edit it tomorrow :)

nical · 2017-07-03T21:04:29Z

I am struggling to explain the distinction of x-inflection/y-inflection, and general inflection points of the curve without making it even more confusing, don't hesitate to ask for more details and/or help!

o0Ignition0o · 2017-07-03T21:16:13Z

I'll give it a shot and let you know. Again thanks a lot for your patience ! :)

o0Ignition0o · 2017-07-04T08:13:17Z

I fixed a couple of things, I guess I'm like halfway there. About the naming convention I'm acutally not looking for inflexion points (which are points where the Second derivative equals zero), but local extremums (which are points when whe derivative equals zero), so I'm not really sure we want to rename it, do we ?

nical · 2017-07-04T08:24:29Z

I'm acutally not looking for inflexion points (which are points where the Second derivative equals zero), but local extremums (which are points when whe derivative equals zero)

That's the distinction between inflection point of the parametric curve (second dervative), and x-inflection (where the curve x(t) or x(y) has an inflection which corresponds to first derivative).
But after all of the confusion around it, I'm starting to agree that calling it extremum instead of x/y-inflection is probably a good thing!

o0Ignition0o · 2017-07-04T08:29:19Z

Oh I think I understand now ! Well it's up to you, I believe both should have different names, but I'm not sure x/y_extremums is the most obvious name. Do you have any suggestions? Else I will rename it to x/y_inflection, and hope no confusion will be made :)

nical

Getting there, you still need to make find_x_minimum and find_y_minimum use the function you added (whatever the name we end up picking for it).

On the topic of naming, if we are going for the extremum terminology, let's call the functions find_local_x_extrema since it does not find the global extrema of the curve (which are often the end points), and the plural of this word is with an 'a' according to wikipedia (and some distant latin classes in which I didn't pay enough attention).

There are a few other review comments to fix and after that, you can remove the printfs and rebase on top of master and it should be ready to merge!

nical · 2017-07-04T08:26:14Z

bezier/src/cubic_bezier.rs

+            return ret;
+        }
+
+        fn in_range(t: f32) -> bool { t >= 0.0 && t < 1.0 }


Let's use t strictly superior to zero here, since this is primarily useful to split the curve into monotone segments and computing the bounding rectangle of the curve, and in both situations there is no need to special-case of split the curve at t = 0.0.

nical · 2017-07-04T08:31:42Z

bezier/src/cubic_bezier.rs

+    /// Return local y extremum points or None if this curve is monotone.
+    ///
+    /// This returns the advancements along the curve, not the actual y position.
+    pub fn find_y_extremums(&self) -> UpToTwo<f32> {


In order to avoid duplicating this code, you could transpose the curve (flip x and y of all its points), and call find_x_extremums, like solve_t_for_y does.

nical · 2017-07-04T08:54:08Z

bezier/src/cubic_bezier.rs

@@ -183,6 +183,140 @@ impl CubicBezierSegment {
        find_cubic_bezier_inflection_points(self)
    }

+    /// Return local x extremum points or None if this curve is monotone.


nit: indentation looks like it's off by one level.

o0Ignition0o · 2017-07-04T11:19:24Z

This seems fine, although I don't feel comfortable with the magic numbers I've just added to the minimum_bounding_rect_for_cubic_bezier_segment test, what do you think ?

let expected_minimum_bounding_rect = rect(0.0, -0.57735026, 2.0, 1.1547005);

nical

Looks great! Let's fuzz that test a bit and then the PR will be ready for merge. Thanks and kudos for your determination!

nical · 2017-07-04T12:37:10Z

bezier/src/cubic_bezier.rs

+        to: Point::new(2.0, 0.0),
+    };
+
+    let expected_minimum_bounding_rect = rect(0.0, -0.57735026, 2.0, 1.1547005);


I would prefer to have a fuzzier test: something like checking that rect(0.0, -0.6, 2.0, 1.2).contains_rect(&actual_minimum_bounding_rect). That way we should catch most regressions while not break the test every time we change the order of some operations and end up with slightly different rounding errors.

Maybe also have a lower bound like also checking that actual_minimum_bounding_rect.contains_rect(&rect(0.0, 0.5, 2.0, 1.0)) in addition to the other contains_rect test.

… finding local x and y extrema

o0Ignition0o · 2017-07-04T13:58:23Z

The test is fuzzed now ! :) plus I've squashed my commits :D Thanks a lot for your patience, I hope I'll be able to tackle harder features, feel free to ping me if you think an unassigned feature you have would match my skills and help me improve! 🙌

nical · 2017-07-04T14:23:06Z

\o/

@o0Ignition0o If you are up for a bit more challenge (but reasonable still), I think that adding features to the stroke tessellator is fun to tackle because you get to actually render something and see the feature with your own eyes which is quite rewarding and the stroke tessellator is still reasonably understandable (while the fill tessellator is a really complex algorithm). For example #34 and #35 would be fun to implement. As always, don't hesitate to ask for help!

o0Ignition0o · 2017-07-04T15:16:46Z

Great, will try it then :D Oh and by the way the quadratic and cubic intersection features should be easy now that these methods are available !

This was referenced Jul 3, 2017

Compute the derivatives a cubic bézier curve. #100

Merged

Add x and y monotone cubic bézier curves. #101

Merged

o0Ignition0o force-pushed the cubic_bezier_min_bounding_rect branch from 5bcec7b to 42f3b64 Compare July 3, 2017 17:54

nical requested changes Jul 3, 2017

View reviewed changes

nical reviewed Jul 4, 2017

View reviewed changes

o0Ignition0o force-pushed the cubic_bezier_min_bounding_rect branch from 885e814 to 8e3b7f5 Compare July 4, 2017 11:18

nical reviewed Jul 4, 2017

View reviewed changes

Minimum bounding rect method for cubic bezier segment, which involves…

eca01d2

… finding local x and y extrema

o0Ignition0o force-pushed the cubic_bezier_min_bounding_rect branch from 9523847 to eca01d2 Compare July 4, 2017 13:57

nical approved these changes Jul 4, 2017

View reviewed changes

nical merged commit b73ce39 into nical:master Jul 4, 2017

o0Ignition0o deleted the cubic_bezier_min_bounding_rect branch July 4, 2017 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Cubic bezier min bounding rect #97

[WIP] Cubic bezier min bounding rect #97

o0Ignition0o commented Jun 28, 2017

nical commented Jun 28, 2017

nical commented Jun 28, 2017

o0Ignition0o commented Jun 28, 2017

nical commented Jun 28, 2017

o0Ignition0o commented Jun 28, 2017

o0Ignition0o commented Jul 1, 2017

o0Ignition0o commented Jul 3, 2017 •

edited

Loading

nical left a comment

nical Jul 3, 2017

nical Jul 3, 2017

nical Jul 3, 2017

nical Jul 3, 2017

nical Jul 3, 2017

nical Jul 3, 2017

nical Jul 3, 2017

o0Ignition0o commented Jul 3, 2017

nical commented Jul 3, 2017

o0Ignition0o commented Jul 3, 2017

o0Ignition0o commented Jul 4, 2017

nical commented Jul 4, 2017

o0Ignition0o commented Jul 4, 2017

nical left a comment

nical Jul 4, 2017

nical Jul 4, 2017

nical Jul 4, 2017

o0Ignition0o commented Jul 4, 2017

nical left a comment

nical Jul 4, 2017

nical Jul 4, 2017 •

edited

Loading

o0Ignition0o commented Jul 4, 2017

nical commented Jul 4, 2017 •

edited

Loading

o0Ignition0o commented Jul 4, 2017

[WIP] Cubic bezier min bounding rect #97

[WIP] Cubic bezier min bounding rect #97

Conversation

o0Ignition0o commented Jun 28, 2017

nical commented Jun 28, 2017

nical commented Jun 28, 2017

o0Ignition0o commented Jun 28, 2017

nical commented Jun 28, 2017

o0Ignition0o commented Jun 28, 2017

o0Ignition0o commented Jul 1, 2017

o0Ignition0o commented Jul 3, 2017 • edited Loading

nical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o commented Jul 3, 2017

nical commented Jul 3, 2017

o0Ignition0o commented Jul 3, 2017

o0Ignition0o commented Jul 4, 2017

nical commented Jul 4, 2017

o0Ignition0o commented Jul 4, 2017

nical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o commented Jul 4, 2017

nical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nical Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

o0Ignition0o commented Jul 4, 2017

nical commented Jul 4, 2017 • edited Loading

o0Ignition0o commented Jul 4, 2017

o0Ignition0o commented Jul 3, 2017 •

edited

Loading

nical Jul 4, 2017 •

edited

Loading

nical commented Jul 4, 2017 •

edited

Loading