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

GPGK Getextent3D semi-fast implementation #8910

Merged
merged 11 commits into from
Dec 6, 2023

Conversation

elpaso
Copy link
Collaborator

@elpaso elpaso commented Dec 4, 2023

Followup #8806:

  • provide a semi-fast impplementation for GPKG
  • use infinity for unknown/unset MinZ and MaxZ instead of NaN

@elpaso elpaso marked this pull request as draft December 4, 2023 15:17
@coveralls
Copy link
Collaborator

coveralls commented Dec 4, 2023

Coverage Status

coverage: 68.004% (+0.01%) from 67.994%
when pulling 7b34842 on elpaso:getextent3d-gpkg
into e902c44 on OSGeo:master.

ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp Outdated Show resolved Hide resolved
Comment on lines +8762 to +8763
extent3D.MinZ = sHeader.MinZ;
extent3D.MaxZ = sHeader.MaxZ;
Copy link
Member

Choose a reason for hiding this comment

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

There's one case that is probably not well covered. It is possible that a 3D geometry has a extent in the GPKG geometry header, but without the Z values (. sHeader.MinZ/sHeader.MaxZ as returned by GPkgHeaderFromWKB() are only valid if poHeader->bExtentHasZ .
Cf http://www.geopackage.org/spec/#gpb_format and bit E: envelope contents indicator code (3-bit unsigned integer).
I don't think the GeoPackage spec mandates to write a 3D envelope if the geometry is 3D (the GDAL GPKG writer will however always write a 3D envelope for a 3D geom, so this would be for .gpkg created by othersoftware). It could be just 2D.
So I believe OGRGeoPackageGetHeader() should be modified to have an extra bNeedExtent3D flag, where you would call getEnvelope3D() if only having a 2D envelope in the GeoPackage geometry header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see... Do you have any recommendation about how to build a test for that case?

Also it is not entirely clear to me when this branch is triggered (and how to test it): https://github.com/OSGeo/gdal/pull/8910/files#diff-1b62be67e99a70d80f3b1651237e60b85c47cdb406a591e5d50dd0f13676361bR529

Copy link
Member

Choose a reason for hiding this comment

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

Do you have any recommendation about how to build a test for that case?

the easiest would probably to patch GPkgGeometryFromOGR(), around lines 292-347 for the purposes of generating test datasets. One without any envelope at all, and another one with 2D envelopes

Copy link
Member

Choose a reason for hiding this comment

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

Also it is not entirely clear to me when this branch is triggered (and how to test it): https://github.com/OSGeo/gdal/pull/8910/files#diff-1b62be67e99a70d80f3b1651237e60b85c47cdb406a591e5d50dd0f13676361bR529

perhaps issue an UPDATE table_name SET geom = 'some garbage here' on an existing .gpkg ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, done.

ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp Outdated Show resolved Hide resolved
@elpaso elpaso marked this pull request as ready for review December 4, 2023 16:44
@elpaso
Copy link
Collaborator Author

elpaso commented Dec 5, 2023

Does this qualify for OLCFastGetExtent3D ? I don't think so.

@rouault
Copy link
Member

rouault commented Dec 5, 2023

Does this qualify for OLCFastGetExtent3D ? I don't think so.

in the situation where there's a GPKG header with a 3D envelope, which I'd expect to be the nominal situation, it should still be significantly faster than the generic implementation. So I'd let that capability for now

@rouault rouault merged commit f355cc1 into OSGeo:master Dec 6, 2023
31 checks passed
@rouault rouault added this to the 3.9.0 milestone Dec 6, 2023
@elpaso elpaso deleted the getextent3d-gpkg branch December 6, 2023 16:24
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.

3 participants