Skip to content
This repository was archived by the owner on Sep 27, 2023. It is now read-only.

Quadratic Bezier curve #140

Closed
wants to merge 2 commits into from
Closed

Quadratic Bezier curve #140

wants to merge 2 commits into from

Conversation

lf94
Copy link
Contributor

@lf94 lf94 commented Sep 23, 2021

No description provided.

@lf94
Copy link
Contributor Author

lf94 commented Sep 23, 2021

Should close #136

@@ -110,6 +110,10 @@ Shape Constructors
Right hand palm on p1, fingertips on p2, thumb is the normal vector
(points away from the half-plane).

``curve.quadratic p0 p1 p2``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't belong here, since curve.quadratic doesn't work unless you have included lib.curves, which isn't mentioned. Since this is actually documentation for lib.curves, it belongs in docs/lib/Curves.rst.

@@ -0,0 +1,66 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you include lib.curves, then you import definitions of dot2, cross2 and sdQuadraticBezier, which pollutes your namespace.

A better design is to hide the internal code like this:

let
  dot2 = ...;
  cross2 = ...;
  sdQuadraticBezier = ...;
in {
  quadratic = ...;
}

);
)
in
sqrt res * sign sgn;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the old code with the bad SDF, not the new code that you got after emailing IQ.

@doug-moen
Copy link
Member

This PR also includes PR #139 in its entirety, which should be removed.
Given the other review comments, I suggest redoing the PR from scratch.

A spline library should ideally include both "stroke" and "area" primitives, in 2D and in 3D, for various spline types. I'm not saying you should implement all of this right away. It's more that I'm thinking about a general naming convention for the different spline primitives we may want to support.

There are multiple types of splines used in computer graphics, not just Bezier, so the name "quadratic" is too vague. There are also quadratic B-splines, etc.

Also, the list of control points that define the spline ought to be specified as a single argument value, a list of points. I think this is more idiomatic in Curv, especially given the rich set of operations on lists of points.

So I'm thinking the namespace and API should look more like this:

lib.spline.quadratic_bezier.area [ [x1,y1], [x2,y2], [x3,y3] ]

@lf94 lf94 closed this Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants