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

Add Line.DistanceTo(Line) #942

Merged
merged 6 commits into from
Feb 24, 2023

Conversation

DmytroMuravskyi
Copy link
Contributor

@DmytroMuravskyi DmytroMuravskyi commented Feb 13, 2023

BACKGROUND:

  • At one moment I needed a function that calculates distance between two lines. Just taking minimum between endpoints and lines doesn't work in all cases. So, this function was created. At the end of the day was not used but I decided that it may be a good addition to the Elements.

DESCRIPTION:

  • Line.DistanceTo(line) compute distance between any two lines, even those that are not on the same plane.

TESTING:

  • Added two tests: one with simple handwritten lines, one with complex parametric numbers.

FUTURE WORK:

  • Closest point is not returned but there is enough information to do so. There is no clear closest point for several cases of parallel or overlapping lines. I guess the close end point of other line can be returned in this case. Let me know if this is something you want.

REQUIRED:

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

COMMENTS:


This change is Reviewable

@wynged wynged self-requested a review February 13, 2023 15:49
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 2 of 4 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @DmytroMuravskyi)


Elements/src/Geometry/Line.cs line 864 at r1 (raw file):

            Vector3 cross = vThis.Cross(vOther);
            // line vectors are collinear - two segments share the same infinite line on the infinite line.
            if (cross.IsZero())

I think we have methods for colinearity on lines, can we use that?


Elements/test/LineTests.cs line 954 at r1 (raw file):

            Vector3 pt = new Vector3(-2.685818406894334, -2.476879934864206, -0.5565494179776103);
            Vector3 v = new Vector3(-2.3537985903693657, 2.407193267710168, -2.771078003386066);
            double q1 = -1.7567568535640823, q2 = -0.21721224020356145, q3 = 1.006813808941921, q4 = 2.112519713576212;

can you use a more descriptive variable name than q1,2,3 ?

Code quote (from Elements/src/Geometry/Line.cs):

                // other + dStartStart and this are now in the same plane
                // check if other is fully on one side of this or vice versa,
                // in other words, if  this and `other + dStartStart` intersect on a shred plane

Elements/test/LineTests.cs line 955 at r1 (raw file):

            Vector3 v = new Vector3(-2.3537985903693657, 2.407193267710168, -2.771078003386066);
            double q1 = -1.7567568535640823, q2 = -0.21721224020356145, q3 = 1.006813808941921, q4 = 2.112519713576212;
            Vector3 pt1 = pt + q1 * v, pt2 = pt + q2 * v, pt3 = pt + q3 * v, pt4 = pt + q4 * v;

I think put these vectors on separate lines, this is pretty hard to read.
Same in the below sections.


Elements/test/LineTests.cs line 986 at r1 (raw file):

            //Two groups of points on two lines.
            pt = new Vector3(2.798721214152833, -0.3556044049478837, -2.9550511796484766);

let's make these different sections actually be different DistanceTo tests. they can run in parallel and the method name can be the description.

Copy link
Contributor Author

@DmytroMuravskyi DmytroMuravskyi 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/Line.cs line 864 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

I think we have methods for colinearity on lines, can we use that?

We could, but cross product is answering the same question and it's used in distance finding later anyway. Following strict tolerance is not critical here, as any non-zero difference between two vectors will calculate precise distance.

Code quote:

cross

Elements/test/LineTests.cs line 954 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

can you use a more descriptive variable name than q1,2,3 ?

I don't think renaming it to quantity1 will make more difference since it's just a parameter in line equation.
I tried to add extra clarity by comments. Those are experimental way to do tests for sure, just wanted to do something that is similar to real numbers, now just lines on X axis. Tell me if it's better to remove them for good.


Elements/test/LineTests.cs line 955 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

I think put these vectors on separate lines, this is pretty hard to read.
Same in the below sections.

Done.


Elements/test/LineTests.cs line 986 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

let's make these different sections actually be different DistanceTo tests. they can run in parallel and the method name can be the description.

Done.

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 r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained

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

@wynged wynged merged commit 8b8a234 into hypar-io:master Feb 24, 2023
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