Fixes to planar trimming. #908

ikeough · 2022-10-21T17:25:17Z

BACKGROUND:
While working on calculating complex allowable envelope shapes, involving clipping lots of polygons with planes at odd angles, I found that there were numerous failure modes for polygon trim operations with planes.

DESCRIPTION:

Simplifies the Trimmed(...) method while handling all existing cases and new cases like planes which intersect a polygon exactly at its corner.
Improves Ray intersection. Previously we didn't disambiguate parallel and coincident rays correctly.
Standardizes on 2D winding calculation for point inclusion in polygons and removes ray casting entirely. For 3D point inclusion, we now flatten and use winding.

TESTING:
Several new tests have been added with specific test cases taken from the work mentioned above. In addition, several existing test cases have been split apart into "one assertion per test" tests for easier debugging of failure cases.

REQUIRED:

All changes are up to date in CHANGELOG.md. n/a

Here's an example of the kind of geometry which demonstrated failures to trim previously. Note that in many cases for the planes labelled "sky", that the plane often intersects some of the polygons exactly at a corner.

This change is

andrewheumann

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

Elements/src/Search/Vector3ToleranceComparer.cs line 9 at r1 (raw file):

    /// Vector3 
    /// </summary>
    public class Vector3ToleranceComparer : IEqualityComparer<Vector3>

This seems like nearly a duplicate of Vector3Comparer. Can we consolidate the two?
Vector3Comparer has a caution about Distinct and other methods that use GetHashCode — I didn't realize if it encountered the same hash code it would fall through to Equals, so perhaps it is safe to do this after all (and revise the caution accordingly)

andrewheumann

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

Elements/src/Search/Vector3ToleranceComparer.cs line 9 at r1 (raw file):

Previously, andrewheumann wrote…

This seems like nearly a duplicate of Vector3Comparer. Can we consolidate the two?
Vector3Comparer has a caution about Distinct and other methods that use GetHashCode — I didn't realize if it encountered the same hash code it would fall through to Equals, so perhaps it is safe to do this after all (and revise the caution accordingly)

Also — am I crazy or is this new Comparer not being used anywhere?

into planar-trim-fixes

ikeough

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

Elements/src/Search/Vector3ToleranceComparer.cs line 9 at r1 (raw file):

Previously, andrewheumann wrote…

Also — am I crazy or is this new Comparer not being used anywhere?

It's not! Apologies. It was used in an earlier iteration of this work. It has been removed.

Yes, it's how the comparers work that they first try the hash and fall back to the actual comparison if they have hash collision. I'll leave revising that comparer to another PR though because it's now completely out of scope :)

andrewheumann

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

Elements/src/Geometry/Polygon.cs line 548 at r2 (raw file):

        /// the default test axis.</param>
        /// <returns>True if the point is contained in the polygon, otherwise false.</returns>
        internal bool Contains3D(Vector3 point, bool unique = true, bool useRandomRay = false)

useRandomRay makes me twitchy..... anything non-deterministic can cause issues that are impossible to reproduce and test. If this was failing with the X axis, then it will occasionally happen to fail along the random ray, and it will be impossible to figure out why it failed, because it won't fail the next time. It seems like Contains3D should be able to work without this.

ikeough

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

Elements/src/Geometry/Polygon.cs line 548 at r2 (raw file):

Previously, andrewheumann wrote…

useRandomRay makes me twitchy..... anything non-deterministic can cause issues that are impossible to reproduce and test. If this was failing with the X axis, then it will occasionally happen to fail along the random ray, and it will be impossible to figure out why it failed, because it won't fail the next time. It seems like Contains3D should be able to work without this.

I've solved this using the strategy recommended in Preparata and Shamos and the wikipedia article which is to only add a vertex intersection when the other vertex of the edge is to the "left" of the ray. This is a clever way to deduplicate ray intersections through vertices and it removes a call to Contains.

into planar-trim-fixes

ikeough · 2022-11-16T20:53:41Z

Elements/test/ObstacleTests.cs

@@ -235,7 +235,7 @@ public void IntersectsObstacleFromLine()
            Assert.True(horizontalObstacle.Intersects(horizontalLineOnBottom));

            var horizontalLineOnSide = horizontalLine.TransformedLine(new Transform(offset, 0, 0));
-            Assert.False(horizontalObstacle.Intersects(horizontalLineOnSide));


@wynged @DmytroMuravskyi This image shows this test geometrically. The horizontalLine is in the plane of the face of the obstacle. This is rightly classified as an intersection by our code now, but was previously asserted to be false. Why?

ikeough · 2022-11-16T20:54:56Z

Elements/test/ObstacleTests.cs

@@ -113,7 +113,7 @@ public void IntersectsRotatedPolygon()

            var result = obstacle.Intersects(polyline);

-            Assert.False(result);


@wynged @DmytroMuravskyi This image shows this test geometrically. The polyline is along the edge of the rotated obstacle. This is rightly classified as an intersection by our code now, but was previously asserted to be false. Why?

ikeough

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

Elements/src/Geometry/Polygon.cs line 548 at r2 (raw file):

Previously, ikeough (Ian Keough) wrote…

I've solved this using the strategy recommended in Preparata and Shamos and the wikipedia article which is to only add a vertex intersection when the other vertex of the edge is to the "left" of the ray. This is a clever way to deduplicate ray intersections through vertices and it removes a call to Contains.

We already had winding number calculation for the flat case. Instead of implementing yet another way to check for containment in a polygon, I updated this to use the "flat" algorithm by transforming the polygon into the XY plane. The ray/polygon intersection now simply calls the Contains3D method which transforms and calls the Contains method into the 3D plane.

andrewheumann

One little comment, otherwise looks good to me!

Reviewed 4 of 4 files at r3, 11 of 14 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ikeough)

Elements/test/SolidTests.cs line 610 at r5 (raw file):

            Assert.Equal(20, result1.Faces.Count);
            Assert.Equal(32, result1.Vertices.Count);
            // Assert.Equal(48, result1.Edges.Count);

This was commented out — intentional? should we remove / update the test / uncomment?

DmytroMuravskyi

It's hard to tell for sure what's the problem with Obstacle.Intersects as i'm not the author of the function but it looks like it doesn't find all the intersections when it's on the border. It feels inconsistent as it says that some lines on border are intersected but other don't. It should behave the same way for all borders or at least have "includeBorders" parameter as sometimes you want to go around obstacle but not inside. I will keep this problem in mind for the next time I'll have spare time

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ikeough)

DmytroMuravskyi

Oh, so it IS consistent now. I think the changes are great and if we would get any troubles in downstream function then I'll adjust downstream function or add a parameter here

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ikeough)

into planar-trim-fixes

ikeough

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @andrewheumann)

Elements/test/SolidTests.cs line 610 at r5 (raw file):

Previously, andrewheumann wrote…

This was commented out — intentional? should we remove / update the test / uncomment?

Done.

ikeough and others added 2 commits October 21, 2022 02:27

Fixes to planar trimming.

ce59aff

Merge branch 'master' into planar-trim-fixes

ec29b19

ikeough marked this pull request as ready for review October 21, 2022 17:25

ikeough requested a review from andrewheumann October 21, 2022 17:26

A little clarification.

dd23370

andrewheumann requested changes Oct 22, 2022

View reviewed changes

ikeough added 5 commits October 22, 2022 23:39

Handle corner on trimming plane.

9718839

Remove unused comparer.

d6c2c31

Merge branch 'planar-trim-fixes' of https://github.com/hypar-io/Elements

0453b55

into planar-trim-fixes

Merge branch 'master' into planar-trim-fixes

a323246

Some better notes.

a7d0082

ikeough commented Oct 24, 2022

View reviewed changes

andrewheumann requested changes Oct 24, 2022

View reviewed changes

ikeough added 2 commits October 25, 2022 11:29

Remove need for random ray casting.

eddfa58

Spelling correction.

49ae136

ikeough commented Oct 25, 2022

View reviewed changes

ikeough added 13 commits October 25, 2022 15:43

Merge branch 'master' into planar-trim-fixes

1b062e5

Merge branch 'master' into planar-trim-fixes

af6d0c3

Vector tweaks.

cad3de5

Better ray intersection testing.

17371bc

Better distance to ray handling.

c44b374

Renaming.

6ab0dff

I screwed up. This is counterclockwise.

bea43f6

Use parameter overlap on rays for better intersection testing.

526c1d0

We don't need this Vector method for now.

4bec82c

Fix polygon tests and solid tests.

be1eded

Merge branch 'planar-trim-fixes' of https://github.com/hypar-io/Elements

b7d1177

into planar-trim-fixes

Revert some docs.

def8db2

Update Contains3D to handle non coplanar test point.

a5ac93d

ikeough added 6 commits November 11, 2022 15:39

Update obstacle to new API.

38b513b

Set this name back to the original.

866645e

Cleanup errant partial.

9b70e5c

Fix ray intersection and test naming.

82672ee

Simplify ray poly intersection by using existing methods.

f70ca38

Flip obstacle assertions.

62af2c5

ikeough commented Nov 16, 2022

View reviewed changes

Merge branch 'master' into planar-trim-fixes

ac12cb9

andrewheumann approved these changes Nov 17, 2022

View reviewed changes

DmytroMuravskyi reviewed Nov 17, 2022

View reviewed changes

ikeough added 2 commits November 17, 2022 12:26

Reenable disabled assertion.

d0084bb

Merge branch 'planar-trim-fixes' of https://github.com/hypar-io/Elements

1d60593

into planar-trim-fixes

ikeough commented Nov 17, 2022

View reviewed changes

ikeough merged commit ff11d20 into master Nov 17, 2022

ikeough deleted the planar-trim-fixes branch November 17, 2022 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to planar trimming. #908

Fixes to planar trimming. #908

ikeough commented Oct 21, 2022 •

edited

andrewheumann left a comment

andrewheumann left a comment

ikeough left a comment

andrewheumann left a comment

ikeough left a comment

ikeough Nov 16, 2022

ikeough Nov 16, 2022

ikeough left a comment

andrewheumann left a comment

DmytroMuravskyi left a comment

DmytroMuravskyi left a comment

ikeough left a comment

		@@ -113,7 +113,7 @@ public void IntersectsRotatedPolygon()

		var result = obstacle.Intersects(polyline);

		Assert.False(result);

Fixes to planar trimming. #908

Fixes to planar trimming. #908

Conversation

ikeough commented Oct 21, 2022 • edited

andrewheumann left a comment

Choose a reason for hiding this comment

andrewheumann left a comment

Choose a reason for hiding this comment

ikeough left a comment

Choose a reason for hiding this comment

andrewheumann left a comment

Choose a reason for hiding this comment

ikeough left a comment

Choose a reason for hiding this comment

ikeough Nov 16, 2022

Choose a reason for hiding this comment

ikeough Nov 16, 2022

Choose a reason for hiding this comment

ikeough left a comment

Choose a reason for hiding this comment

andrewheumann left a comment

Choose a reason for hiding this comment

DmytroMuravskyi left a comment

Choose a reason for hiding this comment

DmytroMuravskyi left a comment

Choose a reason for hiding this comment

ikeough left a comment

Choose a reason for hiding this comment

ikeough commented Oct 21, 2022 •

edited