Indexed polycurve

[ikeough]

BACKGROUND:
We previously built a class called Contour that was IEnumerable<Curve>. This class was meant to support planar compositions of any curve type. It was not thoroughly designed. After completing work on trimmed curves in #957 #961 #959, we decided to rebuild a composite curve type capable of representing networks of lines and arcs.

DESCRIPTION:
This PR implements a new class IndexedPolycurve which is a composite curve of line and arc segments. The external representation of the curve is a set of vertices and a set of sets of of indices which implicitly describe the constructing curves. In this way it is similar to IFCIndexedPolycurve. In addition to being IEnumerable<BoundedCurve>, allowing the user to do foreach(var curve in polycurve), it is also a BoundedCurve, allowing the user to find the start, end, point at at parameter, and transforms along the curve.
The Contour class is deprecated, and locations that used the Contour class, such as the creation of a filleted polycurve from a polygon, now return an IndexedPolycurve.

TESTING:

• How should a reviewer observe the behavior desired by this pull request?

FUTURE WORK:

• We need to take a position on whether this curve is strictly planar. We do not check and we do not enforce planarity. As such, you can make a polycurve that represents, for example, a duct run, with arc sections and lines running in 3D. This seems useful.
• This implementation sacrifices memory for inheritance. The indexed polycurve has a set of curves that are derived from its vertices. We don't want to have to recreate these whenever someone uses the iterator, so we cache them. This is an improvement over what we used to do with polygons, allowing people to call Segments(), but it creates curves even for the polygon case where they might not ever be used.
• This is a mutable class, which is problematic because of the curves that we cache. If the caller adds or removes vertices to the polygon, the index sets and the internal curve sets need to be rebuilt. We can handle this by making the vertex collection read only and adding modifier methods, or we can make the iterator construct the curves.

REQUIRED:

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

An indexed polycurve created by filleting a polygon.

An indexed polycurve created by filleting a polyline.

---

This change is 

[ikeough]

Set this back to draft to fix all test failures and to unify parameters on polygons as well.

[andrewheumann]

Reviewed 1 of 19 files at r2.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough)

[ikeough]

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

---

Elements/src/StructuralFraming.cs line 124 at r2 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

Can this just be if(this.Curve is IndexPolycurve pc)? (maybe I'm missing some nuance that requires the GetType()/typeof comparison)

Nice catch! I don't yet default to pattern matching in C# and the linter doesn't tell me to, so I'm sure there's lot of places that can be improved. This is done.

---

Elements/src/Geometry/IndexedPolycurve.cs line 27 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

(we seem to say "polyline" in a lot of places internally, might be good to update throughout.)

Yes! This is because this class used to be the polyline. Updated all.

---

Elements/src/Geometry/Line.cs line 76 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

startParameter and endParameter are entirely unused — this seems like a mistake (uncaught from an earlier PR)

Done.

---

Elements/src/Geometry/Line.cs line 94 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

It was initially super strange to me that a Line would have an ArcLength. I recognize that this is technically correct, mathematically speaking, but it might risk confusing users. I wonder whether it would be clearer to call this "SubSegmentLength" or "SubCurveLength" or something? I could be convinced that we should keep ArcLength.

Also, this can result in a negative ArcLength — is that desirable?

Fixed the negative arc length.

It's a good question about ArcLength. I'm of a mind to leave it alone even though it's a bit weird. It's an override that all bounded curves share. I thought about what it should be called when I named it there and for arcs, circles, beziers, and ellipses, arc length, is the terminology. This has to be implemented because the base class has this marked as abstract. So we can't change the name unless we want to change it in the base. And I'd argue that if users are looking for "length" they'll probably naturally try myLine.Length() first.

---

Elements/src/Geometry/Interfaces/ICurve.cs line 11 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

if the curve is infinite, does it have domain.min / domain.max? you can get PointAt on an infinite line.

Done.

---

Elements/test/PolygonTests.cs line 159 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

These new[] {} aren't necessary, did you add them for a reason?

While taking a cue from the surrounding code, I forgot that we had an overload that took params. I've removed the new[] from this method and the one mentioned above.

---

Elements/test/PolygonTests.cs line 1062 at r3 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

is there an Assert that goes along with this, or was it something temporary?

Nope. Was for debuggging. Removed.

[ikeough]

This is such a good question about scale transforms. This is probably pervasive in the library. We don't really have a story here. I think you could make the argument that conics only need a plane instead of a transform to represent the slice through a cone (a cone which we also don't describe), which would avoid being able to scale that transform and expect something interesting to happen.

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

[ikeough]

On the deserialization. There wasn't a JsonConstructor with the exact right signature. There is one now, and it's hit during deserialization.

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

[andrewheumann]

These changes look good, and I don't think we have to totally resolve the Transform problem right now. Still curious about the MinimumArcLength change, and would love at the very least a warning in the xml doc comments for TransformedArc, Transformed etc about the kinds of transforms that may cause length or area to be computed incorrectly (or whatever other unexpected consequences users should be aware of.) an example from rhino docs — I don't think it has to be much more than this:

Reviewed 5 of 6 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough)

---

Elements/src/Geometry/Line.cs line 94 at r3 (raw file):

Previously, ikeough (Ian Keough) wrote…

Fixed the negative arc length.

It's a good question about ArcLength. I'm of a mind to leave it alone even though it's a bit weird. It's an override that all bounded curves share. I thought about what it should be called when I named it there and for arcs, circles, beziers, and ellipses, arc length, is the terminology. This has to be implemented because the base class has this marked as abstract. So we can't change the name unless we want to change it in the base. And I'd argue that if users are looking for "length" they'll probably naturally try myLine.Length() first.

Yeah, I buy that. The term technically applies to all curves, so I think it's reasonable.

[ikeough]

I was wondering how other libraries handled this. Thanks for the Rhino reference. I've updated the docs and, for extra credit, I've used <inheritdoc/>, which I didn't know of until just now, to write docs for this method on the base class and have those docs inherited everywhere.

Reviewable status: 1 change requests, 0 of 1 approvals obtained

---

Elements/src/Geometry/Curve.cs line 16 at r2 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

Why this change?

I have to think on a solution here. This got bumped up because in places where we were previously converting circles into polygons for the purpose of making solid geometry, we're now using actual circles and the faceting became far too small (increasing the duration of our test suite to ~15min. due to some CSG tests). We need to be able to set this on an as-needed basis.

[ikeough]

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

---

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

Previously, andrewheumann (Andrew Heumann) wrote…

There's a worrying inconsistency here — a transformed Arc may not be an Arc (and same goes for a Circle.). Transforms applied to a circle or arc can result in any sort of conic section. Some possible solutions:

• Just leave a note/warning in the XML doc comments. (This appears to be what RhinoCommon does — transformed Circles stay Circles, and they just give you a warning. For most operations though, they get turned into Nurbs Curves which can handle this transform logic more appropriately.)
• throw an exception for transformations that don't preserve arc-ness (everything except euclidean transforms and uniform scales)
• Throw an exception for transformations that are non-affine, and return an EllipticalArc.

Comments are now in the docs.

[andrewheumann]

Reviewed 11 of 11 files at r6, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ikeough)