search

boku-ilen / geodot-plugin

Godot plugin for loading geospatial data
GNU General Public License v3.0
108 stars 18 forks source link

Added extra functions to extract individual layers from GDALDatasets and more #81

Closed tuto193 closed 1 year ago

tuto193 commented 1 year ago
  1. Updated the demo project.godot and one import to latest stable version 4.1.
  2. Updated .gitignore to ignore .godot local folder, for when demos are run/checked.
  3. Completed some of the TODOs that were present and deleted some that were no longer required.
  4. Refactored GeoRaster::get_as_array() and GeoImage::set_raster so there is less code duplication (it is hopefully also more readable).
  5. Added extra functions to GeoDataset and GeoRasterLayer to allow extracting individual bands.

    Added:

    • GeoDataset::get_raster_band
    • GeoDataset::get_raster_layers_by_band
    • GeoDataset::get_raster_count
    • GeoRasterLayer::get_band_image

      Changed:

    • GeoDataset::get_raster_layer now has a second argument band_index, which can be mostly be ignored if no specific band is needed, when working with layers, but was added for correctness, so bands that are extracted (using get_raster_layers) still get correct indices set.
    • GeoRaster::FORMAT format initialization got another addition for further completeness/correctness regarding a particular raster's intrinsic format. This is also not 100% in order to maintain backwards compatibility with projects relying on the particular RGB(A) settings that might be overwritten as MIXED. Otherwise this is more like added functionality than a change.
kb173 commented 1 year ago

Looks great, thank you very much!

I added a default value to the band index in GeoDataset.get_raster_layer to make the PR fully backwards-compatible.

While doing that, I got unsure why the GeoRasterLayer has a band_index in the first place - wouldn't it be better to provide the band index later in the GeoRasterLayer.get_band_image function? The old GeoRasterLayer.get_image function does not use the new band index, which is good but might be confusing when a specific band was previously specified in GeoDataset.get_raster_layer.

I think it would also be a more fitting representation of the data if GeoRasterLayers remain an abstract object representing the entire layer, while GeoImages later represent a specific section and band of that data.

Actually, while writing this, I think I figured out why you implemented it this way - you followed the GDAL way of thinking where a dataset practically equals a raster layer (or rather, there is no concept of a raster layer in GDAL) and has bands directly. Therefore, there's a bit of a mix-up between Layers and Bands in the code now. In Geodot, I added the GeoRasterLayer in-between because there are cases where a dataset can have multiple layers (that's where the subdataset stuff came from, it's mostly relevant for GeoPackages). But layers should not equal bands - they're one step higher in the hierarchy. In simple cases like GeoTIFFs where dataset does equal layer, that data should just be fetched via Geodot.get_raster_layer(path).

So following this, it would go like this: a GeoDataset has GeoRasterLayers, and GeoRasterLayers can have multiple bands. (in simple data like GeoTIFFs, the GeoDataset can be left out - so you directly get a GeoRasterLayer, which can have multiple bands which you could later fetch in get_band_image.)

Since this comment got a bit out-of-hand, I just implemented the suggested change myself so you can better see what I mean in https://github.com/boku-ilen/geodot-plugin/pull/81/commits/50c6c292d6d224a9d7c91ccc296c3f3557d6f791. Can you have a look and let me know if that works with your use-case? Everything in the raster-tile-extractor was left unchanged, only the top-level GDExtension code was updated to move the band_index from the GeoRasterLayer object to only the GeoRasterLayer.get_band_image function. I think something like GeoRasterLayer.get_band_count would also make sense to add now, I can add that if you agree with that change.

Thanks again, great to see big contributions like this!

kb173 commented 1 year ago

I realized that you had functionality for band count in the GeoDataset, so I also moved this to the GeoRasterLayer (and re-exposed the descriptions) in 5a27d24. I think all previous functionality should be here now, just with the clear hierarchy of GeoDataset -> GeoRasterLayer -> raster bands. So when you previously queried a GeoDataset for raster bands, you should now be able to do the same with a GeoRasterLayer. Let me know if that works and makes sense for you, or if I'm still missing something! (I'll also be sure to properly document this deviation from the GDAL way of thinking, which doesn't really have a concept of raster layers, somewhere.)

tuto193 commented 1 year ago

Looks great, thank you very much!

I added a default value to the band index in GeoDataset.get_raster_layer to make the PR fully backwards-compatible.

While doing that, I got unsure why the GeoRasterLayer has a band_index in the first place - wouldn't it be better to provide the band index later in the GeoRasterLayer.get_band_image function? The old GeoRasterLayer.get_image function does not use the new band index, which is good but might be confusing when a specific band was previously specified in GeoDataset.get_raster_layer.

I think it would also be a more fitting representation of the data if GeoRasterLayers remain an abstract object representing the entire layer, while GeoImages later represent a specific section and band of that data.

Actually, while writing this, I think I figured out why you implemented it this way - you followed the GDAL way of thinking where a dataset practically equals a raster layer (or rather, there is no concept of a raster layer in GDAL) and has bands directly. Therefore, there's a bit of a mix-up between Layers and Bands in the code now. In Geodot, I added the GeoRasterLayer in-between because there are cases where a dataset can have multiple layers (that's where the subdataset stuff came from, it's mostly relevant for GeoPackages). But layers should not equal bands - they're one step higher in the hierarchy. In simple cases like GeoTIFFs where dataset does equal layer, that data should just be fetched via Geodot.get_raster_layer(path).

So following this, it would go like this: a GeoDataset has GeoRasterLayers, and GeoRasterLayers can have multiple bands. (in simple data like GeoTIFFs, the GeoDataset can be left out - so you directly get a GeoRasterLayer, which can have multiple bands which you could later fetch in get_band_image.)

Since this comment got a bit out-of-hand, I just implemented the suggested change myself so you can better see what I mean in 50c6c29. Can you have a look and let me know if that works with your use-case? Everything in the raster-tile-extractor was left unchanged, only the top-level GDExtension code was updated to move the band_index from the GeoRasterLayer object to only the GeoRasterLayer.get_band_image function. I think something like GeoRasterLayer.get_band_count would also make sense to add now, I can add that if you agree with that change.

Thanks again, great to see big contributions like this!

This seems great! And a get_band_count() does also seem quite fitting indeed!

kb173 commented 1 year ago

Perfect! Merged - thanks again for the big contribution!