SOFAgh
Plane.FromPoints returning -1 Z normal for some planar splines causing inverted arcs.
See the attached drawings in the zip which contain splines that demonstrate the problem.
When adding a plane to a spline using FromPoints, there are situations where a planar flat spline on Z 0 will receive a normal of 0, 0, -1. When importing a DXF with splines and approximating them, this creates Ellipse GeoObjects with a Plane and Normal of 0, 0, -1.
It appears that the symptom of this was noticed and remedied in ExportDxf.cs by inverting the normal when a clockwise ellipse was found. The original symptoms would likely look like an arc with inverted bulge value: https://github.com/FriendsOfCADability/CADability/blob/60baaf2a3c13e19c7fb063e545440adf0c9cf6d2/CADability/ExportDxf.cs#L425-L427
This fix caused the resulting arcs to look good in 2D, but the normals of the impacted arcs were inverted to 0, 0, -1. This shouldn't matter for flat objects, but it causes geometry linkage issues in AutoCAD and in CAM software where up matters in 2D. If you attempt to join planar arcs with a -1 Z normal to lines in AutoCAD, they will join into a spline instead of a polyline. I've had this problem previously with geometry exported from Rhino and implemented a similar fix as below in an AutoCAD plugin that swapped the start and end points.
You can demonstrate the issue with this code:
var cadabDxf = new CADability.DXF.Import(dxfFile).Project;
var newProj = Project.CreateSimpleProject();
var newModel = newProj.GetModel(0);
var allEntites = cadabDxf.GetModel(0).AllObjects;
var contours = new GeoObjectList(allEntites
.Where(ent => ent.Layer.Name == "Outer_Loop" || ent.Layer.Name == "Interior_Loops").ToList());
var compShp = CompoundShape.CreateFromList(contours, 0.001, out Plane plane);
var simpOutline = compShp.SimpleShapes[0].Outline;
var simpHoles = compShp.SimpleShapes[0].Holes;
var simpOutlinePath = simpOutline.AsPath();
var simpOutlineGeo = simpOutlinePath.MakeGeoObject(plane);
newModel.Add(simpOutlineGeo);
foreach (var hole in simpHoles)
{
var segments = hole.GetClonedSegments();
var path = hole.AsPath();
var pathGeo = path.MakeGeoObject(plane);
newModel.Add(pathGeo);
}
var export = new Export(DxfVersion.AutoCad2000);
export.WriteToFile(newProj, $"{outDir}{Path.GetFileNameWithoutExtension(dxfFile)}-2.dxf");
Open the resulting DXF files and check the normal Z value on any arcs generated in the outline border on the simple shape. I've worked around this issue using the changes in this commit. In Plane.FromPoints I'm checking for a < 0 normal Z and 0.0 Z value for all items in the Points array. If this condition arises, the plane is reversed. In ExportEllipse, I've changed the CW/CCW check to swap the start and end points of the ellipse instead of inverting the normal. This so far appears to have resolved the issue in all of my test drawings. I've been unable find any unexpected issues from the plane check change.
My question before I submit this as a PR: Are there any unexpected side effects that I'm not thinking of that could come about by changing the Plane.FromPoints logic as I have? I assume this is some floating point shenanigans in the math that is calculating the plane from points and I'm not sure if there's a better way to address this earlier. I'm also getting some floating point errors in my final geometry but I'm assuming that's a result of not having a way to export chained entities currently so start/end points can be off by miniscule amounts.
Since plane.FromPoints always has two different solutions with the normal pointing in one direction or the opposite direction, and the points in the parameter cannot specify, which direction shoule be preferred, I don't see any problem with always returning the normal vector with z>0. I even don't think it is necessary to check, whether the points are above the XY-plane. This even would make FromPoints more predictable than it is now, where the orientation of the normal is random.
I think there is no clockwise/couterclockwise property for (elliptical) arcs in DXF. This is why the plane is reversed to keep the orientation in DXF. So I think, we shouldn't change the export behaviour.
I'm seeing what I believe is another issue with the same root cause in the Plane constructor, where the check for the X axis direction is returning unpredictable results due to floating point comparisons close to 0.
Using the attached example DXF, I ran the above code and observed the plane being constructed for the comp shape. The behavior I'm seeing is that the Plane constructor called from Plane.FromPoints is getting a normal of 1.1526487981188832E-15, 1.9964611032487207E-17, -1. This causes some randomization on which code path the constructor chooses to set the X axis direction. In my example DXF, the X axis direction will be 0, 1, 0 instead of the expected 1, 0, 0.
Modifying line 160 to use an epsilon comparison of 1e-6 is triggering the correct if block. However immediately following is an if statement which sets the X axis direction to 0, 1, 0 if normal Z <= 0. It's not clear to me why this is being done.
if (Math.Abs(normal.x) < 1e-6 && Math.Abs(normal.y) < 1e-6)
{ // two very common cases
if (normal.z > 0) coordSys = new CoordSys(location, GeoVector.XAxis, GeoVector.YAxis);
else coordSys = new CoordSys(location, GeoVector.YAxis, GeoVector.XAxis);
}
Three questions:
- Does this axis rotation on a non-positive normal Z serve any real use case?
- If yes, and if it's decided that we can always return a positive normal Z in Plane.FromPoints(), what would be the preferred way to ensure than an incorrect negative normal Z is not polluting the Plane construction in Plane.FromPoints()?
- Should this be spun off into a larger issue about floating point comparison predictability? It would seem there are quite a few places where we could improve predictability by making the background comparisons either use a common-sense standard epsilon tolerance or make a new user-exposed setting with a sane default. Cleaning up floating point comparisons may address this issue and more unknown issues with a more holistic approach while avoiding a bunch of edge case brute force corrections.
Regarding No.3:
Floating-point precision can indeed be tricky. Precision.eps has been utilized in many parts of the code to handle similar issues, but it hasn't been consistently applied everywhere it should have been.
This particular case is an example I recently came across. https://github.com/FriendsOfCADability/CADability/issues/208
if (pc < radius - Precision.eps)
return new GeoPoint2D[0]; // no solution
Regarding No.3:
Floating-point precision can indeed be tricky. Precision.eps has been utilized in many parts of the code to handle similar issues, but it hasn't been consistently applied everywhere it should have been.
This particular case is an example I recently came across. #208
if (pc < radius - Precision.eps) return new GeoPoint2D[0]; // no solution
Good to know that's available, thanks! I will go through the Plane code I'm working on and prepare a PR for all of these 0 comparisons that should have a tolerance.
My original fix for returning positive Z normals is going to need to be moved to an earlier stage in light of how they're treated by the Plane constructor. I have an idea but I need to run it through some testing first.