Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DXF import/export: handle ellipse arc with reversed normal #1384

Merged
merged 3 commits into from
Aug 15, 2023
Merged

DXF import/export: handle ellipse arc with reversed normal #1384

merged 3 commits into from
Aug 15, 2023

Conversation

lorenzncode
Copy link
Member

Resolves #1381

@codecov
Copy link

codecov bot commented Aug 5, 2023 •
edited
Loading

Codecov Report

Merging #1384 (38ca2f9) into master (bc82cb0) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 38ca2f9 differs from pull request most recent head 9874013. Consider uploading reports for the commit 9874013 to get more accurate results

@@           Coverage Diff           @@
##           master    #1384   +/-   ##
=======================================
  Coverage   94.12%   94.13%           
=======================================
  Files          27       27           
  Lines        5671     5679    +8     
  Branches      961      962    +1     
=======================================
+ Hits         5338     5346    +8     
  Misses        199      199           
  Partials      134      134
Files Changed Coverage Δ
cadquery/occ_impl/importers/dxf.py 88.34% <ø> (ø)
cadquery/occ_impl/exporters/dxf.py 97.22% <100.00%> (+0.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lorenzncode
Copy link
Member Author

The DXF output might still be problematic for some other software. Another solution could be to convert the edge to the plane normal.

@lorenzncode lorenzncode marked this pull request as draft August 5, 2023 03:25
@adam-urbanczyk
Copy link
Member

Thanks @lorenzncode , do you have some example of this not working?

@lorenzncode
Copy link
Member Author

@adam-urbanczyk Sure, here are a few examples importing ellipse_arc.dxf created in the pytest of the PR.

The CAD Assistant result is working as expected:
image

Inkscape version 1.2.2 is not working:
image

CamBam 1.0.7233.21743 is not working:
image

@adam-urbanczyk
Copy link
Member

Thanks! It'd be good to be able to import correctly in Inkscape. Any clue how to attack this?

@lorenzncode
Copy link
Member Author

Yes, I found another way is to reverse the Sense of the ellipse with reversed normal. I checked that it now imports correctly in Inkscape.

@lorenzncode lorenzncode marked this pull request as ready for review August 7, 2023 00:58
@jmwright
Copy link
Member

Should I review this? Everything else is green, it just needs one more review.

@lorenzncode
Copy link
Member Author

@jmwright Yes, please review.

@lorenzncode lorenzncode requested a review from jmwright August 13, 2023 13:41
jmwright
jmwright approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@jmwright jmwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @lorenzncode

@jmwright jmwright merged commit 7ac13bb into CadQuery:master Aug 15, 2023
@lorenzncode lorenzncode deleted the dxf_ellipse branch August 19, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adam-urbanczyk adam-urbanczyk adam-urbanczyk approved these changes

@jmwright jmwright jmwright approved these changes

Assignees
No one assigned
Labels
dxf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DXF export issue with ellipses
3 participants
@lorenzncode @adam-urbanczyk @jmwright