Updated Polygon.Rectangle function to enforce flat polygon geometry.

[jamesbradleym]

BACKGROUND:

• What led to this work, and what is the broader context of this change?
  Running Polygon.Rectangle() with vectors that don't share the same Z coordinate leads to the creation of non-flat geometry.

DESCRIPTION:

• What does this PR specifically accomplish?
  Enforces flat geometry creation by projecting all points to min.Z when floor = true
• or -
  Creates pitched rectangle with point b sharing Z with max and point d sharing Z with min

TESTING:

• How should a reviewer observe the behavior desired by this pull request?
  Resultant Rectangles created with vectors not sharing the same Z should be flat

FUTURE WORK:

• Is there any future work needed or anticipated? Does this PR create any obvious technical debt?
  Future work may include solving for other points more intelligently to what user may anticipate when z units differ, not essential as other modeling software tends to project resulting rectangle onto XY plane similarly to default behavior.

REQUIRED:

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

COMMENTS:

• Any other notes.

var rectangle = Polygon.Rectangle(new Vector3(0, 0, 1), new Vector3(1, 1, 1)); output.Model.AddElement(rectangle);

var rectangle = Polygon.Rectangle(new Vector3(0, 0, 0), new Vector3(1, 1, 1)); output.Model.AddElement(rectangle);

---

This change is 

[andrewheumann]

My sense is that there isn't a foolproof default behavior here. I am not sure forcing to min z is especially logical — if a function author is supplying two points at different elevations, thinking they were creating a vertical rectangle, they might be surprised / confused by this behavior.

Happy for pushback, but my intuition is:

• If the two Zs are within Vector3.EPSILON (our global tolerance) of each other, it's probably OK to use just min.Z.
• Otherwise, we throw an ArgumentException which informs the user that their inputs are invalid, and suggests a remediation.
• Possible remediations we could implement would be:
  1. Include an optional Plane plane parameter, and if present, project the points to the Plane.
  2. Include an optional Vector3? normal parameter, and if present, project the points to the same parameter along the normal (an implicit plane, basically.)
  3. Include an optional third point such that the third point establishes a plane (another version of implicit plane)

I am inclined towards option 1 — it seems natural to want to be able to create a rectangle from two points in a plane. This is the Rhino API for similar: https://developer.rhino3d.com/docify/api/rhinocommon/rhino.geometry.rectangle3d
Their classes all explicitly differentiate between 2d and 3d in their naming convention, which is something we've talked about for Elements as well.

It would be good to know what case caused you to pursue this — what was in your mind when you accidentally tried to create a rectangle w/ two different z coordinates? what did you expect should happen? Is this mostly a mitigation for "two zs that are ever so slightly off," or were you actually trying to build a rectangle with a different orientation?

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

[jamesbradleym]

Considering this more I agree having floor as the default behavior would be surprising when creating an intentionally Z-differentiated rectangle. I've gone ahead and run over a few more options that from my perspective behave more in line with expectations.

Polygon Rectangle(double width, double height, Plane plane = null)
When a plane is provided the resulting rectangle is a projection of the original points with a provided width and height.
When a plane is not provided the resulting rectangle is drawn on the xy plane, centered at (0,0,0) with a provided width and height.

Polygon Rectangle(Vector3 min, Vector3 max, Plane plane = null)
When a plane is provided the resulting rectangle is a projection of the original points with naively found edges perpendicular to the vector between min and max. This will always result in a rectangle of equal width and height (square).
When a plane is not provided the resulting rectangle is drawn oriented to a naive reference, currently set as one of the Vector.Axis values. This results in a square rectangle as well but is more in line with my expectations of a rectangle drawn in 3D space with only 2 points provided.

I also looked into another method that takes into consideration a prevailing axis (defined as a guide for an edge to follow when constructing the rectangle). This results in a similar rectangle as other modeling software, namely Rhino's Rectangle3D. It seems when creating a Plane object rhino records the Axes of the plane based on the originally(or updated) used points. If we had a similar method to track down the plane's axes we could also implement this method.

[andrewheumann]

I buy the first one (Polygon.Rectangle(double width, double height, Plane plane = null)), and I see the challenge with the second one — a plane and two points are insufficient information. I don't think we should try to force this — an always-square projection seems useless to me.

Our Planes don't have "axes" the way rhino's do — they are just a position and a normal. Instead, when we want to represent a "coordinate frame", we use a Transform. So maybe that's a way forward.

I think your suggestion about an alignment axis makes sense, but I also think we can infer a sensible one in most cases. Here's what I'd propose:

1. a version that utilizes a transform, and always projects to the transform's plane

Polygon Rectangle(Vector3 min, Vector3 max, Transform transform = null) {
    transform ??= new Transform();
    var inverse = transform.Inverted();
    var a = inverse.OfPoint(min);
    a.Z = 0;
    var c = inverse.OfPoint(max);
    c.Z = 0;
    var b = ((c.X, a.Y));
    var d = ((a.X, c.Y));
    return new Polygon(new []{a, b, c, d}).TransformedPolygon(transform);
}

The only risk with this one is that people might be confused and think the min/max are supplied in the transform's coordinate system, rather than projected to it as a plane. I think XML comments can make this clear enough.

2. One that uses a plane and an optional alignment vector. In most architectural cases, if you're drawing a rectangle that's not in the XY plane, one of its sides wants to be parallel to the XY plane.

Polygon Rectangle(Vector3 min, Vector3 max, Plane plane, Vector3? alignment = null) {
    alignment ??= plane.Normal.IsParallelTo(Vector3.ZAxis) ? Vector3.XAxis : Vector3.ZAxis;
    var transform = new Transform(plane.Origin, alignment.Value, plane.Normal);
    return Rectangle(min, max, transform);
}

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

[jamesbradleym]

It doesn't feel intuitive to me that if a min/max with varied Z values are provided that the output is flat in the Z axis, but ultimately I agree it shouldn't be something that is forced and this projection is a better outcome than bizarre non-Z plane weirdness. That being said it feels even less intuitive that inputs with Z values that are not 0 would be projected to 0 outright?

Looking at the results of Rectangle(Vector3 min, Vector3 max, Transform transform = null) it feels like there's a detachment between expectation and result. Maybe I'm reading this wrong but it looks like it projects to the XY plane, not the transform's, and then afterwards is transformed a la transform. If someone results to providing a plane to construct a rectangle it seems reasonable to assume the plane takes precedence in consideration for constructing that rectangle?

[andrewheumann]

I think you're misreading the method which uses a transform. It is projecting the provided points to the plane described by the transform. Audio got cut off at the beginning, but this video hopefully helps clarify how it behaves:
Screen Recording 2023-05-03 at 9.05.21 AM.mp4

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

[jamesbradleym]

Ah, yes I see my mistake, I had left a ZAxis Vector3 in my transform so it was (correctly) projecting everything to the XZ plane when I was doing visual tests. Thanks for the video!

[andrewheumann]

Reviewed all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @jamesbradleym)

---

Elements/src/Geometry/Polygons.cs line 21 at r4 (raw file):

        {
            Vector3 a, b, c, d;
            if (plane != null && plane.Normal.Dot(Vector3.ZAxis) != 1)

If the normal is anti-parallel to the Z Axis, this method will fail. Check the absolute value of the dot product, and use a small amount of tolerance to account for floating point error. plane.Normal.Dot(Vector3.ZAxis) != 1 => Math.Abs(plane.Normal.Dot(Vector3.ZAxis)) > 1- Vector3.EPSILON is probably what I would do

---

Elements/src/Geometry/Polygons.cs line 23 at r4 (raw file):

            if (plane != null && plane.Normal.Dot(Vector3.ZAxis) != 1)
            {
                Vector3 origin = plane.ClosestPoint(Vector3.Origin);

Why the closest point — why not the plane's own origin?

---

Elements/src/Geometry/Polygons.cs line 25 at r4 (raw file):

                Vector3 origin = plane.ClosestPoint(Vector3.Origin);
                // calculate Vector3 for each component based on plane normal
                Vector3 XZ = plane.Normal.Cross(Vector3.ZAxis).Unitized() * width / 2;

I would call these something else — XZ/YZ makes it sound like we're talking about the world XZ/YZ planes, when we're talking about a vector along the width / height of the rectangle

---

Elements/src/Geometry/Polygons.cs line 62 at r4 (raw file):

            var c = inverse.OfPoint(max);
            c.Z = 0;
            var b = ((c.X, a.Y));

are these double parens necessary? (Sorry, I realize those are from my code snippet, but I don't think they're needed)

[jamesbradleym]

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

---

Elements/src/Geometry/Polygons.cs line 21 at r4 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

If the normal is anti-parallel to the Z Axis, this method will fail. Check the absolute value of the dot product, and use a small amount of tolerance to account for floating point error. plane.Normal.Dot(Vector3.ZAxis) != 1 => Math.Abs(plane.Normal.Dot(Vector3.ZAxis)) > 1- Vector3.EPSILON is probably what I would do

100%, didn't catch this in the first go around. Had trouble finding info on lamba in if statements so I re-wrote this as

Code snippet:

// Confirm the provided plane is valid and is not either Z-Normal or would produce a valid rect from U or V
// any abs less than 1-Epsilon would be a valid normal
if (plane != null && Math.Abs(plane.Normal.Dot(Vector3.ZAxis)) > (1 - Vector3.EPSILON))
{ ... }

---

Elements/src/Geometry/Polygons.cs line 23 at r4 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

Why the closest point — why not the plane's own origin?

No reason, makes sense to use the plane.origin as it exists - implemented.

---

Elements/src/Geometry/Polygons.cs line 25 at r4 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

I would call these something else — XZ/YZ makes it sound like we're talking about the world XZ/YZ planes, when we're talking about a vector along the width / height of the rectangle

Switched over to a more conventional U/V syntax which I think is more appropriate here.

---

Elements/src/Geometry/Polygons.cs line 62 at r4 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

are these double parens necessary? (Sorry, I realize those are from my code snippet, but I don't think they're needed)

Nope, removed!

[jamesbradleym]

Additionally added some clarification to the returns xml for

/// <returns>A rectangular Polygon with its lower left corner projected from min and its upper right corner projected from max onto a plane with a sided alignment.</returns> Polygon Rectangle(Vector3 min, Vector3 max, Transform transform = null)

and

/// <returns>A rectangular Polygon with its lower left corner projected from min and its upper right corner projected from max onto a plane with a sided alignment.</returns> Polygon Rectangle(Vector3 min, Vector3 max, Plane plane, Vector3? alignment = null):

[jamesbradleym]

if (plane != null && Math.Abs(plane.Normal.Dot(Vector3.ZAxis)) < (1 - Vector3.EPSILON)) { ... }

Corrected this check to pass for values below 1 - Epsilon. If Math.Abs(plane.Normal.Dot(Vector3.ZAxis)) < (1 - Vector3.EPSILON) then the plane.Normal is not parallel to ZAxis so we should calculate the vertices. If Math.Abs(plane.Normal.Dot(Vector3.ZAxis)) >= (1 - Vector3.EPSILON) then the plane effectively has a ZAxis normal and should be constructed on the XY plane.

[andrewheumann]

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

---

Elements/src/Geometry/Polygons.cs line 21 at r4 (raw file):

Previously, jamesbradleym wrote…

100%, didn't catch this in the first go around. Had trouble finding info on lamba in if statements so I re-wrote this as

Oh apologies for the confusion — I wasn't suggesting a lambda, just using an "arrow" symbol for the before => after

---

Elements/src/Geometry/Polygons.cs line 25 at r4 (raw file):

Previously, jamesbradleym wrote…

Switched over to a more conventional U/V syntax which I think is more appropriate here.

nit — parameters are not capitalized in c# (https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions?redirectedfrom=MSDN). use u and v.

[jamesbradleym]

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

---

Elements/src/Geometry/Polygons.cs line 21 at r4 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

Oh apologies for the confusion — I wasn't suggesting a lambda, just using an "arrow" symbol for the before => after

Gotcha 👍🏻

---

Elements/src/Geometry/Polygons.cs line 25 at r4 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

nit — parameters are not capitalized in c# (https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions?redirectedfrom=MSDN). use u and v.

Done!

[ikeough]

Any updates here?

[andrewheumann]

This code does not compile.

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained

[andrewheumann]

Specifically, it is using newer language features than Elements is set up to use, and introduces an ambiguity which means calls to Polygon.Rectangle(Vector3 a, Vector b) get mistakenly treated as calls to Polygon.Rectangle(double a, double b). I think we can resolve this ambiguity by making Polygon Rectangle(Vector3 min, Vector3 max, Transform transform = null) public static.

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

[jamesbradleym]

Corrected 👍🏻

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)

[andrewheumann]

Reviewable status: complete! 1 of 1 approvals obtained