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

Fix coplanar check #973

Merged
merged 9 commits into from
Apr 24, 2023
Merged

Fix coplanar check #973

merged 9 commits into from
Apr 24, 2023

Conversation

andrewheumann
Copy link
Member

@andrewheumann andrewheumann commented Apr 20, 2023

BACKGROUND:

  • Our Vector3 coplanarity check failed for certain input that it should have accepted, which was causing immediate failures on deserialization, breaking several functions.

DESCRIPTION:

  • Rewrites the coplanarity check to use a dot product method instead of a triple-product method. The triple-product computes a volume, which means there's no sensible way to think about it in terms of Vector3.EPSILON — the dot product lends itself nicely to the interpretation of "check that the points are within EPSILON distance of a plane passing through the first three non-coplanar points"

TESTING:

  • I added some tests, including one that would definitely have failed with the old method.

REQUIRED:

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

This change is Reviewable

Copy link
Member

@wynged wynged left a comment

Choose a reason for hiding this comment

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

one refactor suggestion

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


Elements/src/Geometry/Vector3Extensions.cs line 24 at r1 (raw file):

            int p1Index = -1;
            int p2Index = -1;
            for (int i = 1; i < points.Count; i++)

might be nice to pull this first step out into it's own method. FirstThreeNonCollinearIndices
Maybe even TryGetThreeNonCollinearIndices which would let you retrun true below without additional checks, but that might be messy with out variables etc...

Copy link
Member Author

@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.

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


Elements/src/Geometry/Vector3Extensions.cs line 24 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

might be nice to pull this first step out into it's own method. FirstThreeNonCollinearIndices
Maybe even TryGetThreeNonCollinearIndices which would let you retrun true below without additional checks, but that might be messy with out variables etc...

OK, did my best... I think it's still a little ugly, but it works.

Copy link
Member

@wynged wynged 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 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained


Elements/src/Geometry/Vector3Extensions.cs line 24 at r1 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

OK, did my best... I think it's still a little ugly, but it works.

wfm!

@andrewheumann andrewheumann merged commit 8769cbd into master Apr 24, 2023
1 of 2 checks passed
@andrewheumann andrewheumann deleted the fix-coplanar-check branch April 24, 2023 19:37
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

2 participants