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 writer: do not set 0 as the value for DXF code 5 (HANDLE) #11304

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

rouault
Copy link
Member

@rouault rouault commented Nov 20, 2024

as it is apparently a reserved value.

Fixes #11299

@rouault rouault requested a review from atlight November 20, 2024 01:32
@atlight
Copy link
Contributor

atlight commented Nov 20, 2024

Fixes #11303

That would be #11299 .

Honestly the code at

poDS->WriteEntityID(fp, nGotFID, poFeature->GetFID());
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
Copy link
Member Author

rouault commented Nov 20, 2024

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
Copy link
Contributor

atlight commented Nov 20, 2024

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
Copy link
Member Author

rouault commented Nov 20, 2024

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
Copy link
Collaborator

Coverage Status

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

@rouault rouault merged commit bf808a0 into OSGeo:master Nov 20, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants