mathnet / mathnet-spatial

Math.NET Spatial
http://spatial.mathdotnet.com
MIT License
376 stars 132 forks source link

Checking for equal input points in Plane.FromPoints() is redundant? #185

Closed DaveInCaz closed 1 year ago

DaveInCaz commented 3 years ago

Ref: https://github.com/mathnet/mathnet-spatial/blob/master/src/Spatial/Euclidean/Plane.cs

Here's the main body of FromPoints:

        if (p1 == p2 || p1 == p3 || p2 == p3)
        {
            throw new ArgumentException("Must use three different points");
        }

        var v1 = new Vector3D(p2.X - p1.X, p2.Y - p1.Y, p2.Z - p1.Z);
        var v2 = new Vector3D(p3.X - p1.X, p3.Y - p1.Y, p3.Z - p1.Z);
        var cross = v1.CrossProduct(v2);

        if (cross.Length <= float.Epsilon)
        {
            throw new ArgumentException("The 3 points should not be on the same line");
        }

It seems to me that the initial check for having two or more equal points is redundant to the later check that the cross product result is > 0. (Two or three equal points would obviously also all fall on the same line.)

Is it worth having the first check? It incurs some extra overhead.

Although in cases where two points really are equal the function would do more work without the first check, since an exception is going to fire anyway I'm not sure that is a real concern.

jkalias commented 1 year ago

Hi,

I don't think this is a big issue. Conceptually it seems logical to progress in increasing detail, so the first thing to check would be that all points are distinct. Then, once we make sure that this is satisfied, we must control that the points should not be collinear.

Let me know that you think.

jkalias commented 1 year ago

Nothing to fix