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

r.in.gdal/r.external: add basic support for GDALs GDT_Int8 #4256

Merged
merged 13 commits into from
Sep 4, 2024

Conversation

ninsbl
Copy link
Member

@ninsbl ninsbl commented Aug 29, 2024

This PR adds basic support for importing GDT_Int8 datatype via GDAL.

I am no C-programmer, so I would be happy if a knowledgeable person could look into this. I am especially unsure about the #ifndef part (note that 14 is the constant representing GDT_Int8).

Addresses #4230

@ninsbl ninsbl added bug Something isn't working C Related code is in C labels Aug 29, 2024
@ninsbl ninsbl added this to the 8.4.1 milestone Aug 29, 2024
@github-actions github-actions bot added raster Related to raster data processing libraries module labels Aug 29, 2024
@neteler
Copy link
Member

neteler commented Aug 30, 2024

It compiles fine on Fedora 39 with GDAL 3.7.3.

CI: There is an issue with this approach on systems with older GDAL:

gdal.c: In function ‘Rast_get_gdal_link’:
gdal.c:132:5: error: case value ‘14’ not in enumerated type ‘GDALDataType’ [-Werror=switch]
  132 |     case GDT_Int8:
      |     ^~~~
cc1: all warnings being treated as errors

I am afraid that only a GDAL version check might work, like:

#if GDAL_VERSION_NUM >= 3000000

or by combining in a test (see raster/r.out.gdal/main.c for variants):

#if GDAL_VERSION_MAJOR >= 3

and

#if GDAL_VERSION_MINOR >= 7
?

@rouault
Copy link
Member

rouault commented Aug 30, 2024

#if GDAL_VERSION_NUM >= 3000000

Better use
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3,7,0)

lib/raster/get_row.c Outdated Show resolved Hide resolved
@ninsbl
Copy link
Member Author

ninsbl commented Aug 30, 2024

Thanks @neteler and @rouault ! Your feedback is very much appreciated. I tried to implement yor suggestions in the latest commits...

@nilason
Copy link
Contributor

nilason commented Aug 31, 2024

Stefan, you should also add #include <stdint.h> (which is where the C99 fixed width integer types are defined, see more) to get_row.c.

@github-actions github-actions bot added Python Related code is in Python tests Related to Test Suite labels Sep 1, 2024
@ninsbl
Copy link
Member Author

ninsbl commented Sep 1, 2024

Stefan, you should also add #include <stdint.h> (which is where the C99 fixed width integer types are defined, see more) to get_row.c.

Thanks for the hint. Added in 1565b1d

I also added a test for this case.

nilason
nilason previously approved these changes Sep 2, 2024
@ninsbl
Copy link
Member Author

ninsbl commented Sep 3, 2024

Thanks for your approval @nilason . I had to add a switch to skip the test for systems with GDAL < 3.7 (like Ubuntu 22.04, where tests failed). That commit dismissed your review, it would be great if you could re-approve...

@ninsbl ninsbl enabled auto-merge (squash) September 3, 2024 21:09
@ninsbl ninsbl merged commit 66389f0 into OSGeo:main Sep 4, 2024
26 checks passed
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Sep 5, 2024
* add support for Int8 from GDAL >= 3.7

* add int8 test (skiped if GDAL < 3.7)
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Sep 5, 2024
* add support for Int8 from GDAL >= 3.7

* add int8 test (skiped if GDAL < 3.7)
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Sep 5, 2024
* add support for Int8 from GDAL >= 3.7

* add int8 test (skiped if GDAL < 3.7)
Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
* add support for Int8 from GDAL >= 3.7

* add int8 test (skiped if GDAL < 3.7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C libraries module Python Related code is in Python raster Related to raster data processing tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants