OSGeo / gdal

GDAL is an open source MIT licensed translator library for raster and vector geospatial data formats.
https://gdal.org
Other
4.92k stars 2.56k forks source link

DXF writer: do not set 0 as the value for DXF code 5 (HANDLE) #11304

Closed rouault closed 1 day ago

rouault commented 1 day ago

as it is apparently a reserved value.

Fixes #11299

atlight commented 1 day ago

Fixes #11303

That would be #11299 .

Honestly the code at https://github.com/OSGeo/gdal/blob/1b32bc87538a116bb7d4d6ccef1b9831fb04acb8/ogr/ogrsf_frmts/dxf/ogrdxfwriterlayer.cpp#L173 just looks wrong. There is no such thing as an "entity ID"; DXF entities have an "entity handle". The value of the EntityHandle field, if set, should be used as the entity handle, otherwise we should just be making one up. Using the FID for the entity handle is nonsense and should never have been done.

rouault commented 1 day ago

That would be #11299 .

corrected, thanks

The value of the EntityHandle field, if set, should be used as the entity handle, otherwise we should just be making one up. Using the FID for the entity handle is nonsense and should never have been done.

yes, I agree that looks dubious. But perhaps as a temporary measure (especially for the sake of a 3.10 backport), we can live with that in the mean time ? I can create an issue to remember we should do something along the line you mention.

atlight commented 1 day ago

But perhaps as a temporary measure (especially for the sake of a 3.10 backport), we can live with that in the mean time ? I can create an issue to remember we should do something along the line you mention.

Yes, if this fixes the user's immediate issue I am not opposed to it going in, but it is certainly something that needs addressing properly down the track.

Incidentally, it would help me if you could briefly explain why ogr2ogr doesn't appear to hit this code path? It seems to always write an entity handle of 20000 no matter what id value I set in the user's GeoJSON minimal example.

rouault commented 1 day ago

if you could briefly explain why ogr2ogr doesn't appear to hit this code path?

ogr2ogr by default doesn't set the FID of the new feature from the FID of the source feature, when the target driver has no layer FID creation option. It lets its to OGRUnsetFID == -1. Whereas GDALDataset::CopyLayer() always set the FID of the new feature from the source one.

coveralls commented 1 day ago

Coverage Status

coverage: 73.702% (-0.002%) from 73.704% when pulling 5ecc1631e60c119219f4cbb4ffbf1ededb0e2220 on rouault:fix_11299 into a00fbc1a4007db108ebc13715c1c72729d651835 on OSGeo:master.