vagran / dxf-viewer

DXF 2D viewer written in JavaScript
Mozilla Public License 2.0
290 stars 86 forks source link

handle LWPOLYLINE with non-contiguous vertices data #100

Closed cmurphy23 closed 5 months ago

cmurphy23 commented 6 months ago

I noticed that sometimes LWPOLYLINE entities have their vertices in chunks, and the existing implementation would overwrite the array with the most recent, leading to points being missing. I made the following modification and the shapes started rendering properly.

vagran commented 6 months ago

Thanks for the PR! Do you have any sample file which illustrates the problem?

cmurphy23 commented 6 months ago

No problem. I can't provide a sample file because it's not my data, but I can provide an example entity that maybe you can paste into an existing file to test with? exampleDxfLwpolyline.txt

Also, here are screenshots of the example shape before and after this commit.

Screenshot from 2024-02-19 09-24-15 Screenshot from 2024-02-19 09-23-32

vagran commented 5 months ago

I reviewed your example data (lwpolyline-vertex-id.dxf.zip) and your fix. The root cause is that LWPOLYLINE vertex tag 91 (vertex ID) was not handled by the parser. I checked with the specification and looks like it is the only tag which is not handled there, so it should be enough just to add this tag processing. So I did this in the fix above. I want to fix a problem with the most recent three.js version before releasing npm package (it has error when trying to calculate bounding sphere on geometry and fails, probably because of custom shader and buffer layout).

cmurphy23 commented 5 months ago

You're right that your fix is more correct, and it does fix things for the test file. So long as you reviewed the spec and there are no more fields that could possibly cause this issue (and new won't be added?), then my fix is not needed as you say. Of course my change doesn't hurt, and it does make the code bulletproof against other unhandled thags.

Thanks for this great project.