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

Plane.line_xsections: Clarify the desired behavior in a degenerate case #63

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Sep 30, 2019

Geometrically, the intersection between a plane and a coplanar line is the line itself. Plane.line_xsections returns "not intersecting."

Is this desirable?

If so, would it make sense to add a second method like Plane.contains_line() that makes it really easy to check this case?

expected = np.array([[np.nan, np.nan, np.nan]])
intersections, is_intersecting = plane.line_xsections(pts, rays)
np.testing.assert_array_equal(intersections, expected)
np.testing.assert_array_equal(is_intersecting, [False])
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it's really worth adding this test, as it basically duplicates behavior from the previous test. If the decision is that this behavior is kept, I can add some comments to the method and the previous test, instead.

@paulmelnikow paulmelnikow changed the title Plane.line_xsections: Document the behavior in a degenerate case Plane.line_xsections: Clarify the desired behavior in a degenerate case Sep 30, 2019
@paulmelnikow
Copy link
Member Author

Hey @algrs, is this something you'd be able to weigh in on?

@paulmelnikow
Copy link
Member Author

paulmelnikow commented Oct 31, 2019

Decision:

  • Check for this case, and return a third array of flags

This will be a breaking change for some users.

@paulmelnikow paulmelnikow added this to the 1.x milestone Oct 31, 2019
@paulmelnikow paulmelnikow marked this pull request as draft April 22, 2020 20:00
@paulmelnikow paulmelnikow changed the base branch from master to main October 6, 2020 03:47
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.

1 participant