Adds new LinePlaneIntersection method. #2873
Conversation
|
@dotnet-policy-service agree |
| /// <param name="point">When the method completes, contains the point of intersection, | ||
| /// or <see cref="Vector3.Zero"/> if there was no intersection.</param> | ||
| /// <returns>Whether the two objects intersected.</returns> | ||
| public static bool LinePlaneIntersection(Plane plane, Vector3 pt1, Vector3 pt2, out Vector3 point) |
There was a problem hiding this comment.
Other methods in this CollisionHelper class uses names such as vertex1, point, etc. Maybe there could be more descriptive names for arguments pt1 and pt2? Something like point1, point2, intersection, or something along those lines.
|
|
||
| float denominator = Vector3.Dot(N, V); | ||
|
|
||
| if (denominator == 0) |
There was a problem hiding this comment.
Floating point comparison against zero is usually a bad idea because of precission issues. Better to use relational operators and/or compare against an epsilon value. Look at how it is done in ClosestPointPointTriangle():
float d1 = Vector3.Dot(ab, ap);
float d2 = Vector3.Dot(ac, ap);
if (d1 <= 0.0f && d2 <= 0.0f)
{
result = vertex1; //Barycentric coordinates (1,0,0)
return;
}|
|
||
| float t = -(Vector3.Dot(N, S) + D) /denominator; | ||
|
|
||
| point = S + (t * (V)); |
There was a problem hiding this comment.
This one is a minor nitpick.
I know this may come from mathematical formulas where variables are named S, t, N, or V. But in code it helps immensely (specially when debugging or reading the code) to have more descriptive names.
Also, if you have variables that only store the value of some other and are never modified (like float D = -plane.D), why not use -plane.D directly?
Make code style adjustments from feedback.
Ok I think I made the changes you wanted, thanks. |
PR Details
Related Issue
Types of changes
Checklist