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 Line2D.TryIntersect to pass nullable Point2D #187

Merged
merged 4 commits into from
Mar 27, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Spatial.Tests/Euclidean/LineSegment2DTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -139,14 +139,14 @@ public void IntersectWithTest(string s1, string e1, string s2, string e2, string
Assert.AreEqual(e, result);
}

[TestCase("0,0", "-2,-2", "1,0", "1,2", "1,1")]
[TestCase("0,0", "-2,-2", "0,1", "2,1", "1,1")]
[TestCase("0,0", "2,2", "-1,-5", "-1,0", "-1,-1")]
public void IntersectWithTest2(string s1, string e1, string s2, string e2, string expected)
[TestCase("0,0", "-2,-2", "1,0", "1,2")]
[TestCase("0,0", "-2,-2", "0,1", "2,1")]
[TestCase("0,0", "2,2", "-1,-5", "-1,0")]
public void IntersectWithTest2(string s1, string e1, string s2, string e2)
{
var line1 = LineSegment2D.Parse(s1, e1);
var line2 = LineSegment2D.Parse(s2, e2);
var e = string.IsNullOrEmpty(expected) ? (Point2D?)null : Point2D.Parse(expected);
var e = (Point2D?)null;
bool success = line1.TryIntersect(line2, out var result, Angle.FromRadians(0.001));
Assert.IsFalse(success);
Assert.AreEqual(e, result);
Expand Down
25 changes: 17 additions & 8 deletions src/Spatial/Euclidean/LineSegment2D.cs
Original file line number Diff line number Diff line change
Expand Up @@ -134,19 +134,19 @@ public Point2D ClosestPointTo(Point2D p)
}

/// <summary>
/// Compute the intersection between two lines if the angle between them is greater than a specified
/// Compute the intersection between two line segments if the angle between them is greater than a specified
/// angle tolerance.
/// </summary>
/// <param name="other">The other line to compute the intersection with</param>
/// <param name="intersection">When this method returns, contains the intersection point, if the conversion succeeded, or the default point if the conversion failed.</param>
/// <param name="tolerance">The tolerance used when checking if the lines are parallel</param>
/// <param name="other">The other line segment to compute the intersection with</param>
/// <param name="intersection">The intersection if it exists; otherwise null</param>
/// <param name="tolerance">The tolerance used when checking if the line segments are parallel</param>
/// <returns>True if an intersection exists; otherwise false</returns>
[Pure]
public bool TryIntersect(LineSegment2D other, out Point2D intersection, Angle tolerance)
public bool TryIntersect(LineSegment2D other, out Point2D? intersection, Angle tolerance)
{
Copy link
Member

Choose a reason for hiding this comment

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

Initialization intersection = null;

if (this.IsParallelTo(other, tolerance))
{
intersection = default(Point2D);
intersection = null;
Copy link
Member

Choose a reason for hiding this comment

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

Not needed here if the initialization is done at the function entry point

return false;
}

Expand All @@ -159,8 +159,17 @@ public bool TryIntersect(LineSegment2D other, out Point2D intersection, Angle to
var t = (q - p).CrossProduct(s) / r.CrossProduct(s);
var u = (p - q).CrossProduct(r) / s.CrossProduct(r);

intersection = p + (t * r);
return (0.0 <= t && t <= 1.0) && (0.0 <= u && u <= 1.0);
var isIntersected = (0.0 <= t && t <= 1.0) && (0.0 <= u && u <= 1.0);
if (isIntersected)
{
intersection = p + (t * r);
return true;
}
else
Copy link
Member

Choose a reason for hiding this comment

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

You can simplify all this by simply return intersection != null; and completely removing the else case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for a comment.

How about the following case?
83840266-f4d1c880-a6b2-11ea-8137-56f9fee366af

This comes from #178.

Copy link
Member

@jkalias jkalias Mar 28, 2023

Choose a reason for hiding this comment

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

If I am not mistaken, this is exactly the bug fix in this PR, correct? There is no intersection in your example, you can verify it using LineSegment2DTests.IntersectWithTest2

I helped a bit in the implementation of this PR if you don't mind, this has already been merged to master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for a comment.
Your understanding is correct that we can verify it by using LineSegment2DTests.IntersectWithTest2.

I'm happy with the merged code. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Thank YOU for opening the issue and providing a PR 😃

{
intersection = null;
return false;
}
}

/// <summary>
Expand Down