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

DXF: add creation option to set and DXF system variables #11424

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

rouault
Copy link
Member

@rouault rouault commented Dec 3, 2024

Fixes #11423

@rouault
Copy link
Member Author

rouault commented Dec 3, 2024

@atlight I was wondering if using the CRS unit as a default for INSUNITS would be a sane default ? And what about MEASUREMENT (should it be related to CRS units?) ? For now, I've kept the values from header.dxf (inches / imperial) as the default

@rouault rouault added the funded through GSP Work funded through the GDAL Sponsorship Program label Dec 3, 2024
@rouault rouault force-pushed the dxf_INSUNITS_MEASUREMENT branch from 9dd8963 to 20ed056 Compare December 3, 2024 04:07
@coveralls
Copy link
Collaborator

coveralls commented Dec 3, 2024

Coverage Status

coverage: 74.175% (+0.002%) from 74.173%
when pulling 0d98f32 on rouault:dxf_INSUNITS_MEASUREMENT
into 7d69cae on OSGeo:master.

ogr/ogrsf_frmts/dxf/ogrdxfwriterds.cpp Show resolved Hide resolved
ogr/ogrsf_frmts/dxf/ogrdxfwriterds.cpp Outdated Show resolved Hide resolved
ogr/ogrsf_frmts/dxf/ogrdxfwriterds.cpp Show resolved Hide resolved
Copy link
Contributor

@atlight atlight left a comment

Choose a reason for hiding this comment

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

Haven't tested, but looks good to me. Just minor stuff here.

ogr/ogrsf_frmts/dxf/ogrdxfwriterds.cpp Show resolved Hide resolved
ogr/ogrsf_frmts/dxf/ogrdxfwriterds.cpp Outdated Show resolved Hide resolved
ogr/ogrsf_frmts/dxf/ogrdxfwriterds.cpp Outdated Show resolved Hide resolved
ogr/ogrsf_frmts/dxf/ogrdxfwriterds.cpp Outdated Show resolved Hide resolved
@atlight
Copy link
Contributor

atlight commented Dec 3, 2024

@atlight I was wondering if using the CRS unit as a default for INSUNITS would be a sane default ? And what about MEASUREMENT (should it be related to CRS units?) ? For now, I've kept the values from header.dxf (inches / imperial) as the default

(Reading your comment after leaving my review) I think this makes a lot of sense. If your CRS is in metres you ideally should get a DXF in metres without taking any active steps to ensure that.

Not really sure about MEASUREMENT, it doesn't seem very important. It's not something my company or our clients ever use.

@rouault rouault force-pushed the dxf_INSUNITS_MEASUREMENT branch from 20ed056 to c95e280 Compare December 3, 2024 13:13
@rouault
Copy link
Member Author

rouault commented Dec 3, 2024

I think this makes a lot of sense. If your CRS is in metres you ideally should get a DXF in metres without taking any active steps to ensure that.

ok, I've remove the INSUNIT=FROM_CRS mode, and rather use a default INSUNIT=AUTO mode that tries to use the CRS info when available, and fallbacks to the value of the template header.dxf file

@rouault rouault force-pushed the dxf_INSUNITS_MEASUREMENT branch from c95e280 to 0d98f32 Compare December 3, 2024 15:35
@rouault rouault merged commit 4355190 into OSGeo:master Dec 4, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement funded through GSP Work funded through the GDAL Sponsorship Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DXF Driver - Allow Setting of Units in Header
4 participants