Skip to content
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

Real mid and more #993

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Real mid and more #993

wants to merge 26 commits into from

Conversation

jamesbradleym
Copy link
Contributor

@jamesbradleym jamesbradleym commented Jul 21, 2023

BACKGROUND:

  • In our transition to 2.0 much of our curve behavior changes in lieu of their parameterization, here we introduce a new interface IHasArcLength which enforces Midpoint(), PointAtLength(), PointAtNormalizedLength(), and DivideByLength() which applies to Circles, Lines, Polylines, and Beziers.
  • Additionally Bezier felt unloved given the push to supporting curves, to perk it up we introduce Split functionality and normalized parameterization + parameter at functionality, and Bezier.ConstructPiecewiseCubicBezier()

DESCRIPTION:

  • Gives Circles, Lines, Polylines, and Beziers more flexibility in the information they can return.

TESTING:

  • Play around with Circles, Lines, Polylines, and Beziers looking at Midpoint(), PointAtLength(), PointAtNormalizedLength(), and DivideByLength() and for Bezier poke around ParameterAt(), PointAtNormalized() Building multi point Bezier.ConstructPiecewiseCubicBezier(), dividing Beziers, spliting Beziers.

FUTURE WORK:

  • Is there any future work needed or anticipated? Does this PR create any obvious technical debt?

REQUIRED:

  • All changes are up to date in CHANGELOG.md.

COMMENTS:

  • Any other notes.

This change is Reviewable

Copy link
Contributor

@anthonie-kramer anthonie-kramer left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 11 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann, @ikeough, @jamesbradleym, and @wynged)


Elements/src/Geometry/Interfaces/IHasArcLength.cs line 7 at r2 (raw file):

    /// Implementing classes define methods for computing points and performing operations based on arc length.
    /// </summary>
    public interface IHasArcLength

In general, I feel like this should just be bundled with IBoundedCurve. I don't see the distinction here for Arc Lengths, these seem to just be general curve methods.

It seems like maybe Circle is the exception since it is not a BoundedCurve...

Either way, it seems like we can abstract these into ICurve or IBoundedCurve unless there is an Arc specific method in here, which I would then recommend it gets named appropriately. Although, I don't see anything specific to arcs, such as arguments that are an Angle type.


Elements/src/Geometry/Interfaces/IHasArcLength.cs line 22 at r2 (raw file):

        /// <param name="normalizedLength">The normalized length along the curve.</param>
        /// <returns>The point on the curve at the specified normalized length.</returns>
        Vector3 PointAtNormalizedLength(double normalizedLength);

Is this different than PointAtNormalized in IBoundedCurve?


Elements/src/Geometry/Interfaces/IHasArcLength.cs line 28 at r2 (raw file):

        /// </summary>
        /// <returns>The midpoint of the curve.</returns>
        Vector3 MidPoint();

How is this different than Mid()?


Elements/src/Geometry/Interfaces/IHasArcLength.cs line 35 at r2 (raw file):

        /// <param name="length">The desired length for dividing the curve.</param>
        /// <returns>A list of points representing the divisions along the curve.</returns>
        Vector3[] DivideByLength(double length);

I think this needs some more arguments to allow for division types (remainder at one end, remainder at both ends, evenly divided, etc). See how Grid divisions are handled

@ikeough ikeough added this to the 2.1 milestone Jul 24, 2023
Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r1, all commit messages.
Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer, @ikeough, @jamesbradleym, and @wynged)


Elements/src/Geometry/Interfaces/IHasArcLength.cs line 7 at r2 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

In general, I feel like this should just be bundled with IBoundedCurve. I don't see the distinction here for Arc Lengths, these seem to just be general curve methods.

It seems like maybe Circle is the exception since it is not a BoundedCurve...

Either way, it seems like we can abstract these into ICurve or IBoundedCurve unless there is an Arc specific method in here, which I would then recommend it gets named appropriately. Although, I don't see anything specific to arcs, such as arguments that are an Angle type.

I agree I would like to see this get bundled up in some parent classes / potentially other interfaces. Are there any BoundedCurves that don't / cant support this? (re: naming though, "ArcLength" is the term for measured length along any curve, not just arcs )

@ikeough
Copy link
Contributor

ikeough commented Jul 24, 2023

A couple of high level comments. I'm moving this out to 2.1 for further design. Much of what is in here should be, if we choose to bring it in, part of the IBoundedCurve interface, which is the base interface for all bounded curves. Circle is not a bounded curve, so it should not have an arc length. However, Arc which is a trimmed curve with a Circle for its basis curve should have an arc length. As should EllipticalArc. We can discuss more when 2.0 is out the door.

Copy link
Contributor Author

@jamesbradleym jamesbradleym left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer, @ikeough, and @wynged)


Elements/src/Geometry/Interfaces/IHasArcLength.cs line 22 at r2 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

Is this different than PointAtNormalized in IBoundedCurve?

Yes, This does not normalize the parameter value to the length of the curve, it simply remaps the parameter space from 0->1. As Polyline and Bezier are not equally parameterized you would not get the midpoint from something like PointAtNormalized(0.5).

Screenshot 2023-07-25 at 12.11.22 PM.png
Screenshot 2023-07-25 at 12.13.16 PM.png


Elements/src/Geometry/Interfaces/IHasArcLength.cs line 28 at r2 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

How is this different than Mid()?

Mid() finds the mid parameter, for a Line or Circle this is fine as the middle of the parameter space is the same as the length based midpoint. For Polyline and Bezier, however, parameter space is not length based. For bezier the parameter is weighted by control points and for polyline it is vertex based.

Left side = MidPoint()
Right side = Mid()

Screenshot 2023-07-25 at 12.02.31 PM.png
Screenshot 2023-07-25 at 12.06.12 PM.png

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants