isce-framework / isce-framework/isce3

GDAL Dataset::SetGeoTransform updates

Open
#199 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
239
Forks
90
Avg merge
13d 1h
Merged PRs (30d)
5

Description

In #196 @scottstanie helpfully fixed a compilation issue related to a change in the GDAL API introduced in version 3.12. Specifically, the GDALDataset virtual method

virtual CPLErr SetGeoTransform(double *padfTransform);

got modified to

virtual CPLErr SetGeoTransform(const GDALGeoTransform &gt);
CPLErr SetGeoTransform(const double *padfTransform);  // deprecated

with the idea that you're supposed to use the new GDALGeoTransform type instead of double*. Likewise for the getters.

The problem that @aivazis points out is that the deprecated method had virtual removed, so there's no way to override it in derived classes. Since IH5Dataset inherits from GDALDataset, in principle we don't have a way to guarantee correct behavior whenever something like this happens:

double gt[6];
GDALDataset* dset = new IH5Dataset();
dset->SetGeoTransform(gt);  // calls method of base class, not derived class

We care about this behavior because isce3::io::Raster calls GDALDataset::SetGeoTransform(double*) here:
https://github.com/isce-framework/isce3/blob/fa46d2af09f680ac7cee599dc293ac85601bce62/cxx/isce3/io/Raster.icc#L459
and we certainly do construct a Raster from an IH5Dataset.

In practice, the current GDAL implementation is to cast the array argument and call the SetGeoTransform(const GDALGeoTransform &) method:
https://github.com/OSGeo/gdal/blob/4ec9f5211bfe98855158d881b6170a2ddfe5db68/gcore/gdaldataset.cpp#L1759
I think that means we get basically the behavior we want in Raster.

I'm not sure what the cleanest fix going forward might be.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the GDAL SetGeoTransform declarations and implementation linked in the issue, then inspect isce3/io/Raster.icc at the referenced call and the IH5Dataset inheritance path. Determine the supported fix for preserving correct dispatch through GDALDataset*, and verify that Raster construction from an IH5Dataset retains the intended geotransform behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.