Unbound, bound, and trimmed curves.

[ikeough]

BACKGROUND:

• We would like to support Ellipse and EllipticalArc.
• We would like to support curved surfaces.
• We would like to support a Polycurve type that corresponds to IfcIndexedPolycurve.
• We would like to align with certain industry standards, like STEP, to ease compatibility.

DESCRIPTION:

• Introduces BoundedCurve
  • Introduces the IBoundedCurve interface for bound curves that have a start and end.
  • Makes Polyline inherit from BoundedCurve
• Introduces TrimmedCurve
  • Introduces the ITrimmedCurve<T> interface for trimmed curves which have a basis curve and a domain.
  • Makes Circle inherit from Curve, and makes Arc a TrimmedCurve<Circle>.
  • Makes the existing Line a TrimmedCurve<InfiniteLine> (a line segment) and introduces the InfiniteLine class which represents an unbound line from a start point along a direction. Optimally, Line, should become LineSegment and InfiniteLine should be Line, but the renaming would break all serialized instances of lines. This should be considered in the future.
• Introduces IConic for curves which are planar sections of cones like Circle and (soon) Ellipse.
• Introduces "natural" parameterization.
  • Changes the parameterization of Circle to be [0,2PI], and adjusts Arc domain accordingly to use a start and end angle.
  • Changes the parameterization of Line to be [0,Length]
  • Changes the parameterization of Polyline to be [0,length]

TESTING:

• All tests currently pass, but we should visually check results to ensure that behavior is consistent.

FUTURE WORK:

• Bezier should have its domains adjusted for natural parameterization when we have better length support.
• Convenience methods to get parameterization 0->1 should be added like NormalizedParameterAt(double u).

REQUIRED:

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

---

This change is 

[ikeough]

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

---

Elements/src/Geometry/Arc.cs line 14 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

The Arc validator throws if you use a startAngle / endAngle > 360, but doesn't care anything about < -360. Since the code actually seems to tolerate start/end angles outside of this range, should we remove that validation? alternately, we can set it to clamp at the negative end too (but I would hate to lose the ability to do something like new Arc(5, -45, 45))!

I've changed the validation for arc span to check whether the proposed span is > 360.0, regardless of start /end. LMK what you think.

---

Elements/src/Geometry/Arc.cs line 49 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

an circular => a circular

Done.

---

Elements/src/Geometry/Polyline.cs line 1416 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

The fact that we can get away with this — not implement this, since the only callers are Frames and RenderVertices, which we also override in this class, feels like maybe a design flaw? If nothing else, it might be worth a comment in the base class which says something like:

You don't need to implement this so long as you override Frames and RenderVertices.

But that makes me nervous too... the minute we add another capability to BoundedCurve which relies on GetSampleParameters, now all of a sudden we have an issue. IDK, I don't have super strong feelings about this, but as-is it feels vaguely risky.

This is a good catch. I've made GetSampleParameters abstract so every curve has to implement it. I've also simplified RenderVertices() to have one implementation, which required allowing curve types to specify which GL line mode they should use to render. See the addition of the internal property IsClosedForRendering which is set to false by default and true for polygons so that the line mode switches to LOOP. All RenderVertices was doing differently for polygons was adding one more vertex to the collection so it would draw as a loop.

---

Elements/src/Geometry/Interfaces/IBoundedCurve.cs line 5 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

curces => curves

Done.

---

Elements/src/Math/Domain1d.cs line 37 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

good catch!

Done.

---

Elements/test/LineTests.cs line 646 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

Seems like we missed an automatic lint fix here, this should be
line.Length() / 2 with spaces

Done.

---

Elements.Serialization.IFC/src/Serialization/IFC/IFCExtensions.cs line 37 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

I think this can be .TransformedLine to avoid the cast.

Done.

---

Elements/src/Units.cs line 360 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

Not sure why this method belongs in Units. Also, appears to be unused by the code — remove?

Ah yes, created this and used it before realizing that Domain.Includes existed. Removed.

[andrewheumann]

Reviewed 10 of 17 files at r4, 9 of 9 files at r5, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer and @ikeough)

---

Elements/src/Geometry/Arc.cs line 14 at r3 (raw file):

Previously, ikeough (Ian Keough) wrote…

I've changed the validation for arc span to check whether the proposed span is > 360.0, regardless of start /end. LMK what you think.

LGTM

---

Elements/src/Geometry/Polygon.cs line 28 at r5 (raw file):

        internal Plane _plane;

        internal override bool IsClosedForRendering => false;

shouldn't a polygon be IsClosedForRendering true?

---

Elements/src/Geometry/Polyline.cs line 1416 at r3 (raw file):

Previously, ikeough (Ian Keough) wrote…

This is a good catch. I've made GetSampleParameters abstract so every curve has to implement it. I've also simplified RenderVertices() to have one implementation, which required allowing curve types to specify which GL line mode they should use to render. See the addition of the internal property IsClosedForRendering which is set to false by default and true for polygons so that the line mode switches to LOOP. All RenderVertices was doing differently for polygons was adding one more vertex to the collection so it would draw as a loop.

That feels a lot cleaner!

[anthonie-kramer]

Reviewed 10 of 48 files at r1, 1 of 10 files at r3, all commit messages.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @ikeough)

---

Elements/src/Geometry/Arc.cs line 98 at r3 (raw file):

        /// </summary>
        /// <param name="radius">The radius of the arc.</param>
        /// <param name="startAngle">The angle from 0.0, in degrees, at which the arc will start with respect to the positive X axis.</param>

assuming counter-clockwise, but good to add hints here...

[anthonie-kramer]

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @ikeough)

---

Elements/src/Geometry/Line.cs line 578 at r5 (raw file):

        public override Vector3 Mid()
        {
            return Start.Average(End);

or PointAt(0.5) ? Fewer operations = more precise (though probably negligible anyway)

---

Elements/src/Search/LeftMostPointComparer.cs line 35 at r5 (raw file):

            var b = _getSegment(t2);

            const double small = 0.001;

I don't know if we use constants like these in other places, but there should probably be global constants for tolerances, etc.

[ikeough]

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

---

Elements/src/Geometry/Arc.cs line 98 at r3 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

assuming counter-clockwise, but good to add hints here...

Done.

---

Elements/src/Geometry/Line.cs line 578 at r5 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

or PointAt(0.5) ? Fewer operations = more precise (though probably negligible anyway)

Parameterization of lines is now 0->length, so PointAt would have to know the length of the line. Average is actually the shortest path.

---

Elements/src/Geometry/Polygon.cs line 28 at r5 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

shouldn't a polygon be IsClosedForRendering true?

Done

---

Elements/src/Geometry/Polyline.cs line 1416 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

That feels a lot cleaner!

Done.

---

Elements/src/Search/LeftMostPointComparer.cs line 35 at r5 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

I don't know if we use constants like these in other places, but there should probably be global constants for tolerances, etc.

We can address in a separate PR. The idea here was to keep the functionality as close to the original functionality, while accommodating the reparameterization.

[andrewheumann]

As discussed, when we're ready to merge this, let's try to get an elements release out the door with any in-flight bug fixes before we make the 2.0 bump. but LGTM!

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @anthonie-kramer)

[anthonie-kramer]

Reviewed 19 of 48 files at r1, 1 of 2 files at r2, 2 of 10 files at r3, 10 of 17 files at r4, 9 of 9 files at r5, all commit messages.
Reviewable status: complete! 2 of 1 approvals obtained