Limit DXF BSpline Degree #1226
Limit DXF BSpline Degree #1226
Conversation
Most DXF renderers and processing tools can't handle BSplines with degree >= 3 (order >= 4). For maximum compatibility, we should approximate such BSplines using degree-3 splines. This change uses the OpenCascade facilities to do so, though ezdxf.math also provides some spline approximation facilities that could be used. Using the OpenCascade approach allows us to match FreeCAD's parameters which are presumably tuned on a diversity of real-world designs. Fixes CadQuery#1225
sapphire-arches
force-pushed
the
approx-curves
branch
from
32cda0a to
612ad80
Compare
|
This ends up having a pretty severe impact on the accuracy of the exported DXFs, degrading the output quality by a few orders of magnitude. We could probably recover that by reducing the allowed approximation error in |
Codecov Report
@@ Coverage Diff @@
## master #1226 +/- ##
==========================================
+ Coverage 94.07% 94.14% +0.06%
==========================================
Files 26 26
Lines 5421 5451 +30
Branches 919 925 +6
==========================================
+ Hits 5100 5132 +32
+ Misses 190 189 -1
+ Partials 131 130 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
DXF is also used to generate CNC gcode, and the current behavior is non-approximated, so I don't know if this approximation should be the default. |
|
The tolerance specified is for 1 micrometer in a mm scale drawing, or 1 thou on an inch-scale drawing. I suppose the later is probably worth worrying about. If you have suggestions on how we should plumb through specifying whether or not to approximate, I'd be happy to implement that change. |
|
@bobtwinkles I'd like to hear from others about this too, but my first thought was to leave the default at un-approximated and add an option somewhere to approximate splines during the export. However, that forces the CQ dev to know about the option and use it correctly, and end users of the DXF will be stuck if the dev chooses incorrectly. I guess I don't really understand the scope of the problem. How many applications are not able import our DXF splines now versus how many will have that problem after approximation? Do we know if FreeCAD approximates splines during their DXF export? If they do it, we'd probably be safe to as well. |
|
Thanks for the PR @bobtwinkles . I agree with @jmwright , this has to be an option (e.g. You had to modify the tests significantly, so there is something wrong in the approximation. 1e-6 error cannot result in up to 50% change of the area. -1 for merging until this is fixed/understood. BTW: you can look into |
Right, this is my concern as well. If I do implement an option I'll try to drop a note in the docs to point out this potential footgun which will hopefully help avoid the problem.
Nothing I have to hand (LibreCAD and the Ponoko/SendCutSend online processors) can import the unapproximated DXFs correctly (except round-tripping to other OpenCascade-based software like cadquery and FreeCAD). My implementation is basically a direct port of the FreeCAD 0.20 DXF export logic.
I don't think the test changes indicate there's a 50% change in area? The changes are loosening the acceptable order of magnitude difference by 2-3 notches, not changing the value being compared with. All cases still require 1E-3 agreement or better, which matches the tolerance of the approximation (1E-3). Or really it's better, since most of the tests check area while the approximation is a tolerance on the wires.
Looks like that uses ApproxCurve internally, so I don't know if we would see any benefit from using the more generic interface. |
|
My bad, I confused the tolerance for the test value. I think you are making the problem bigger than it is, random online dxf viewer does display your shape correctly: https://sharecad.org/. Regarding the function - I'd prefer that the conversion is handled at the |
Ah, I see. Do you mean we should add a method to |
|
The idea would be to implement the approximation functionality in say If |
This adds plumbing through the option infrastructure to make the DXF approximation optional, and expose the important control parameters to the user with reasonable defaults. Includes some additional documentation to ease discovery and explain why that should be important.
|
I've updated this PR with plumbng to make the approximation optional, and some documentation about why it might be required. I ended up reusing the |
There was a problem hiding this comment.
I started testing toSplines and found a case where the degree was not restricted to the specified value (3).
pts = [(0, 0), (10, 10), (20, -10)]
wp = cq.Workplane().splineApprox(pts, 1e-3, 3, 6)This resulted in degree = 5. After toSplines (with default args or with reduced tol) the degree was not reduced.
Original edge on left, with translated and restricted version on right (with display of poles).
Changing pts to the following resulted in degree = 6.
pts = [(0, 0), (10, 10), (20, -10), (30, 10)]
With toSplines, it resulted in degree = 5.
|
@lorenzncode thanks! Using C0 or G2 seems to solve the issue. |
There was a problem hiding this comment.
I tested the approx="arc" and did not find any problem. Added minor doc suggestions. Thanks @adam-urbanczyk and @bobtwinkles!
Co-authored-by: Jeremy Wright <[email protected]> Co-authored-by: Lorenz <[email protected]>
Co-authored-by: Jeremy Wright <[email protected]>
|
Merging, thank you @bobtwinkles ! |
Most DXF renderers and processing tools can't handle BSplines with degree >= 3 (order >= 4). For maximum compatibility, we should approximate such BSplines using degree-3 splines. This change uses the OpenCascade facilities to do so, though ezdxf.math also provides some spline approximation facilities that could be used. Using the OpenCascade approach allows us to match FreeCAD's parameters which are presumably tuned on a diversity of real-world designs.
Fixes #1225