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.

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

[SVG] The user should be able to provide the units of measurement for an svg #1325

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

giannissc
Copy link

Closes: #1323

CC: @voneiden

@jmwright
Copy link
Member

How does this relate to this existing draft PR?

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #1325 (c85154e) into master (3b9ead1) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master    #1325      +/-   ##
==========================================
- Coverage   94.24%   94.14%   -0.11%     
==========================================
  Files          26       26              
  Lines        5495     5485      -10     
  Branches      932      929       -3     
==========================================
- Hits         5179     5164      -15     
- Misses        187      189       +2     
- Partials      129      132       +3     
Impacted Files Coverage Δ
cadquery/occ_impl/exporters/svg.py 91.81% <100.00%> (-4.85%) ⬇️

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

@giannissc
Copy link
Author

I wasn't actually aware of that PR @jmwright

@giannissc
Copy link
Author

Since this is such a small and straightforward change should we get this merged? @voneiden, @jmwright

@adam-urbanczyk
Copy link
Member

@lorenzncode what do you think? Do you want to finish your PR first?

@lorenzncode
Copy link
Member

I find this PR breaks SVG export with exporters.export. For example in the following width is not respected.

cq.exporters.export(result, "out.svg", opt={"width":100})

This PR adds a units parameter but I don't see where the units are put to use. In #1317 it looks like the idea is to influence scaling.

I'd also like to have optional control of the scale and that is one of the features of #1277.

@giannissc
Copy link
Author

Right now this has nothing to do with scaling. uom is used internally to set the scaling factor. What I changed here is the ability of the user to explicitly set the unit of measure instead of the current heuristic which works like this: <0.1 must be inches and >10 must be millimeter. I am preparing a separate PR for the scaling changing for which I will be taking a look at your PR to combine them @lorenzncode

@jmwright jmwright mentioned this pull request Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SVG] The user should be able to provide the units of measurement for an svg
4 participants