mozman / ezdxf

Python interface to DXF
https://ezdxf.mozman.at
MIT License
906 stars 189 forks source link

Crash drawing hatch #482

Closed mbway closed 3 years ago

mbway commented 3 years ago

This is a hatch which is crashing ezdxf. As you can see from the code, there are edge path: associative hatch not supported messages so the hatch is missing parts, but it still triggers the crash and AutoCAD is able to read it

bug_report.zip

image (47)

$ ezdxf draw bad_entity.dxf
Traceback (most recent call last):
  File "ezdxf/bin/ezdxf", line 33, in <module>
    sys.exit(load_entry_point('ezdxf', 'console_scripts', 'ezdxf')())
  File "ezdxf/__main__.py", line 75, in main
    run(args)
  File "ezdxf/commands.py", line 357, in run
    frontend.draw_layout(layout, finalize=True)
  File "ezdxf/addons/drawing/frontend.py", line 148, in draw_layout
    self.draw_entities(
  File "ezdxf/addons/drawing/frontend.py", line 173, in draw_entities
    self.draw_entity(entity, properties)
  File "ezdxf/addons/drawing/frontend.py", line 192, in draw_entity
    draw_method(entity, properties)
  File "ezdxf/addons/drawing/frontend.py", line 363, in draw_hatch_entity
    self.out.draw_filled_paths(external_paths, holes, properties)
  File "ezdxf/addons/drawing/matplotlib.py", line 128, in draw_filled_paths
    v1, c1 = _get_path_patch_data(path.counter_clockwise())
  File "ezdxf/path/path.py", line 264, in counter_clockwise
    if self.has_clockwise_orientation():
  File "ezdxf/path/path.py", line 132, in has_clockwise_orientation
    return has_clockwise_orientation(self.control_vertices())
  File "ezdxf/math/_construct.py", line 26, in has_clockwise_orientation
    raise ValueError('At least 3 vertices required.')
ValueError: At least 3 vertices required.
mozman commented 3 years ago

@mbway Just for your information: My simple island detection algorithm can never handle such complex (weird) hatches correctly :)

mbway commented 3 years ago

that's OK , not crashing is the main thing! The original (which happens to be the same hatch from https://github.com/mozman/ezdxf/issues/466) still draws correctly anyway, the hatch in this issue is missing some parts because of dxf2code not supporting all the edges)

mozman commented 3 years ago

This may need further investigations: image

  1. Checking for correct interpretation of the orientation flag.
  2. Why the ARC start- and end points do not match the HATCH vertices?
mozman commented 3 years ago

@mbway I found an error in the EdgePath class not really related to this issue: The methods add_arc() and add_ellipse() do not swap start- end angle for clockwise oriented curves, which leads to incorrect results, because internally arcs and ellipses are stored in counter-clockwise orientation to reuse the existing transformation functions.

    doc = ezdxf.new()
    msp = doc.modelspace()
    hatch = msp.add_hatch()
    edge_path = hatch.paths.add_edge_path()
    edge_path.add_line((5, 0), (0, -5))
    edge_path.add_line((0, -5), (-5, 0))
    edge_path.add_arc((0, 0), 5, start_angle=180, end_angle=0, ccw=False)
    doc.saveas(DIR / "clockwise_arc.dxf")

Incorrect result: clockwise-arc-incorrect

    ...
    # swapping start- and end angle:
    edge_path.add_arc((0, 0), 5, start_angle=0, end_angle=180, ccw=False)
   ... 

Correct result: clockwise-arc-correct

This is only relevant for edges added by the user, edge paths of loaded and stored DXF files work as they should.

What is your opinion: should i change the add_arc() and add_ellipse() methods to work correctly and break existing code, which may have workarounds for this error, or is better to leave the methods as they are and just document this behavior, which means the user has to swap the start- and end angles.

I tend to change the behavior and break existing code. I assume not many (maybe no one) has noticed this issue, because there is no error report about that.

EDIT: This will also break every code generated by the dxf2code add-on which uses clockwise arcs or ellipses, i'm not so sure anymore if i really want to break the code.

mbway commented 3 years ago

This will also break every code generated by the dxf2code add-on

isn't the issue that dxfs created with the dxf2code addon have problems because of this bug? (the dxf in my initial post was created with the addon)

Personally I think in the long run, correctness should take priority. Versioning can be used to not introduce breaking changes between minor version changes, so people can upgrade when they are prepared to deal with breaking changes. potentially invalidating automatically generated code is a bit tricky, but having start and end be the wrong way round sounds like a bug, so in a way the generated code is already incorrect

mozman commented 3 years ago

isn't the issue that dxfs created with the dxf2code addon have problems because of this bug? (the dxf in my initial post was created with the addon)

I don't think so, because you extracted the code from a loaded DXF file, which swaps the start- and end angle at the loading stage. Exporting the DXF also swaps the angels and everything is okay.

But there are maybe other related errors because i forget the fact of always counter-clockwise oriented arcs and ellipses. That's what i am working on right now.

Maybe a solution is to leave the current API (add_arc() and add_ellipse()) as they are (including proper documentation about the issue) and create new APIs: add_cw_arc(), add_ccw_arc(), add_cw_ellipse() and add_ccw_ellipse().

mbway commented 3 years ago

that could be very confusing if add_cw_arc != add_arc(ccw=False) though, unless you put a deprecation warning in add_arc or something to let the user know? but then deprecating add_arc seems like a shame

mozman commented 3 years ago

The old add_arc() has to remain, because i can not know for which ezdxf version the dxf2code code was generated, except i use your suggestion about API versioning, but i found only REST related articles online. Any suggestion how to do proper API versioning for a library like ezdxf? Or is the simple idea, just adding a keyword argument version=1 good enough? New generated code by dxf2code would add this keyword, but users also have to add this keyword manually to use the new correct version.

mbway commented 3 years ago

the idea of specifying the version is like an idea I had, but i wasn't that confident it was a good idea so I didn't mention it. In theory you could have a global setting for the library so at the top of any generated code there could be a call like ezdxf.set_version(0.16) and it would behave like ezdxf 0.16.x. Code where this isn't specified behaves like the latest. Obviously the more versions you maintain compatibility for, the more work there is, but you could slowly deprecate and remove versions over time I suppose.

What I meant by versioning is that you would expect 0.16.1 and 0.16.2 to behave the same, there should only be superficial changes to the API and bug fixes, but between 0.16 and 0.17 you might not expect code to be compatible. This was already my experience recently when upgrading to the latest ezdxf which I had frozen for quite a while resulted in a lot of breaking changes that I needed to fix (such as the issue in this bug report and some changes around the addition of multileader support)

mozman commented 3 years ago

I tried to do the API changes only in the main versions, but the code base is already too big and the API not good enough (too much distributed changes) to keep branches a long time separated. It is what it is.

There is no perfect solution for the add_arc() and add_ellipse() methods, so i stick to the simplest solution - do nothing and just document the strange behavior of swapping angels for clockwise oriented curves. The lack of feedback from users, let me assume that the API is not used directly very often and existing code from dxf2code does not break.

mozman commented 3 years ago

@mbway Found some related errors and the boundary edge paths (converted to DXF entities) are now the same as the path elements marked by BricsCAD:

image

Can you share the original DXF file of this HATCH? Maybe there is an error in the dxf2code add-on.

mbway commented 3 years ago

it should be the same entity as from https://github.com/mozman/ezdxf/issues/466 but if that dxf is no good I may be able to create a new one for you

mozman commented 3 years ago

dxf2code is okay, the error (and destruction of the HATCH entity) must have occurred through a transformation that was applied to the HATCH 2AB in hole.dxf

mozman commented 3 years ago

Tested transformation (rotation, scaling and reflection 1000x) of HATCH entity 2AB in DXF file "hole.dxf" #466 and found no problems, no HATCH was destroyed:

image

The arcs in the "bad entity" of this issue have the same radii and ccw flags, only the start- and end angles are messed up.

@mbway Any idea how this "bad entity" was created?

test script: transform_466_hatch.zip

mbway commented 3 years ago

sorry. I wasn't able to reproduce the situation that I originally created the above example with. If I check out an older ezdxf (before this bug was fixed) and try to draw the original CAD file where this entity originates, it no longer crashes for some reason. I think what I was doing was drawing the CAD file with my own drawing code, and catching any exceptions and exporting the entity being drawn at the time. I think that's where the entity came from. However there are other components that might be involved such as ODAFileConverter which I have updated since posting this bug report.

What I do know is that the entity was nested 2 levels deep inside INSERT entities which might have caused some problems. So I would have been exporting a virtual entity since it was expanded for drawing.

mozman commented 3 years ago

Okay, i close this issue. Open a new issue if there is still an error which shows up in another example.