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

SFCGAL: Use WKB instead of WKT #11006

Merged
merged 4 commits into from
Dec 17, 2024
Merged

SFCGAL: Use WKB instead of WKT #11006

merged 4 commits into from
Dec 17, 2024

Conversation

lbartoletti
Copy link
Contributor

@lbartoletti lbartoletti commented Oct 14, 2024

What does this PR do?

This PR adds support for WKB (Well-Known Binary) instead of WKT (Well-Known Text) for SFCGAL versions greater than or equal to 1.5.2.

It is noted that SFCGAL introduced support for WKB in version 1.5.1, but full binary support is available starting from version 1.5.2. Therefore, this update ensures that WKB is utilized for SFCGAL versions 1.5.2 and above.

Additionally, the PR includes a check for the SFCGAL version using a configure_file to generate the ogr_sfcgal.h file, and modifications are made to the CMakeLists.txt to handle this versioning logic properly.

This PR is currently opened as a draft because I still need to verify some parts of the GDAL code to ensure everything is properly implemented and handled as expected.

What are related issues/pull requests?

Tasklist

  • Build with SFCGAL <= and >= 1.5.2
  • Make sure code is correctly formatted (cf pre-commit configuration)
  • Add test case(s)
  • Add documentation
  • Updated Python API documentation (swig/include/python/docs/)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Environment

Provide environment details, if relevant:

  • OS: Locally on FreeBSD 14 with SFCGAL 2.0.0 + GDAL CI
  • Compiler: Clang + GDAL CI

Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

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

I see alpine:edge has sfcgal 1.5.2, so we should get some CI testing for the new code path

Copy link

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link
    to any issues which this pull request fixes

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in 2 weeks.

@coveralls
Copy link
Collaborator

coveralls commented Nov 13, 2024

Coverage Status

coverage: 69.918% (-0.01%) from 69.932%
when pulling 1a3d249 on lbartoletti:wkb_sfcgal
into acdb4bd on OSGeo:master.

@rouault rouault added this to the 3.11.0 milestone Nov 13, 2024
@rouault
Copy link
Member

rouault commented Nov 13, 2024

@lbartoletti looks good to me. anything left on our side?

@rouault rouault removed the stale label Nov 13, 2024
@lbartoletti
Copy link
Contributor Author

@lbartoletti looks good to me. anything left on our side?

Yes, I have to add tests

@rouault
Copy link
Member

rouault commented Nov 14, 2024

Yes, I have to add tests

I haven't look in details, but there are SFCGAL related tests in autotest/ogr/ogr_geom.py . To be checked if they cover your added code paths.

Copy link

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link
    to any issues which this pull request fixes

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in 2 weeks.

@github-actions github-actions bot added the stale label Dec 13, 2024
@lbartoletti
Copy link
Contributor Author

Dear Stale Bot,

I know I haven't been very responsive lately, but it's the end of the year and everyone's asking for things to be done, finalizing training sessions, end of budget, etc.

I promise I'll be a good developer and finalize my tests and checks before Christmas, so you too can enjoy your holiday season and rest far away from this PR.

Best regards

@stale stale bot removed the stale label Dec 13, 2024
@lbartoletti lbartoletti marked this pull request as ready for review December 17, 2024 08:25
@lbartoletti
Copy link
Contributor Author

Yes, I have to add tests

I haven't look in details, but there are SFCGAL related tests in autotest/ogr/ogr_geom.py . To be checked if they cover your added code paths.

I added a tiny missing test to ensure a good OGR<->SFCGAL conversion. I think, it's OK for now. I plan to add more tests ; and maybe SFCGAL's function if it's ok for GDAL

@rouault rouault merged commit 4db3259 into OSGeo:master Dec 17, 2024
38 checks passed
@lbartoletti lbartoletti deleted the wkb_sfcgal branch December 17, 2024 16:40
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