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

VFK: FIX invalid parcel "banana" geometries (fixes #3376) #11688

Merged
merged 16 commits into from
Jan 19, 2025

Conversation

seidlmic
Copy link
Contributor

Fixes invalid polygon geometry building by replacing old algoritm with GEOS BuildArea() function. (Added OGRGeometry::BuildArea()/OGR_G_BuildArea() and GEOSBuildArea() to load polygons geometries in VFK.

Introduces on GEOS drive with about 10% performance penalty.

Thanks to Petr Zima as the main author.

* @return a newly allocated geometry now owned by the caller,
* or NULL on failure.
*
* @since OGR 3.8.4
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @since OGR 3.8.4
* @since OGR 3.11

* @return a handle on newly allocated geometry now owned by the caller,
* or NULL on failure.
*
* @since OGR 3.8.4
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @since OGR 3.8.4
* @since OGR 3.11

@@ -29,3 +29,7 @@ if (GDAL_USE_SQLITE3)
gdal_target_link_libraries(ogr_VFK PRIVATE SQLite::SQLite3)
target_compile_definitions(ogr_VFK PRIVATE -DHAVE_SQLITE)
endif ()
if (GDAL_USE_GEOS)
Copy link
Member

Choose a reason for hiding this comment

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

Is the driver still useful from a practical point of view without the GEOS dependency ? If not, then making it conditioned to GEOS presence should be done in ogr/ogrsf_frmts/CMakeLists.txt y changing

ogr_dependent_driver(vfk "Czech Cadastral Exchange Data Format" "GDAL_USE_SQLITE3")

to

ogr_dependent_driver(vfk "Czech Cadastral Exchange Data Format" "GDAL_USE_SQLITE3;GDAL_USE_GEOS")

by the way seeing SQLite3 is already a mandatory dependency, one could remove the if() test at line 27, and perhaps other conditional code in the driver, but that's admintedly not directly in the scope of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The drive can be built without GEOS. In that case drive will warn about this situation during conversion and building polygon geometries (layers BUD, PAR} will be skipped. All other data will be imported.

Warning 6: GEOS support not enabled. Unable to build geometry for PAR.
Warning 6: GEOS support not enabled. Unable to build geometry for BUD.

@rouault
Copy link
Member

rouault commented Jan 17, 2025

Thanks to Petr Zima as the main author.

you can add at the end of the commit message (preferably using the email associated to his github account if he has one) so that git properly attributes authorship

Co-authored-by: name <email@domain.com>

@coveralls
Copy link
Collaborator

coveralls commented Jan 18, 2025

Coverage Status

coverage: 70.073% (-0.005%) from 70.078%
when pulling 28d73cf on seidlmic:fix_build_polygon_geometry
into e59bf5d on OSGeo:master.

@rouault
Copy link
Member

rouault commented Jan 18, 2025

ok, so we have a test failure on configuration https://github.com/OSGeo/gdal/actions/runs/12838383582/job/35807308362?pr=11688 where we have SQLite support but not GEOS. So depending on the answer to my question of #11688 (comment), either make the driver GEOS dependent, or add a "@pytest.mark.require_geos" decoration to the failed test

@seidlmic seidlmic force-pushed the fix_build_polygon_geometry branch from 336c427 to 0237ddc Compare January 18, 2025 15:17
@seidlmic
Copy link
Contributor Author

I have decided for "@pytest.mark.require_geos" and not fully require GEOS.

@rouault rouault added this to the 3.11.0 milestone Jan 19, 2025
@rouault rouault merged commit 607bec8 into OSGeo:master Jan 19, 2025
38 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
Development

Successfully merging this pull request may close these issues.

5 participants