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

Make all Transform methods of OGRCoordinateTransformation and GDALTransformerFunc return FALSE as soon as one point fails to transform #11819

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

rouault
Copy link
Member

@rouault rouault commented Feb 7, 2025

Fixes #11817

Kind of a breaking change, but the current behavior was highly inconsistent and hard to reason about.

New paragraph in MIGRATION_GUIDE.TXT:

  • The following methods OGRCoordinateTransformation::Transform(size_t nCount, double *x, double *y, double *z, double *t, int *pabSuccess) and OGRCoordinateTransformation::TransformWithErrorCodes(size_t nCount, double *x, double *y, double *z, double *t, int *panErrorCodes) are modified to return FALSE as soon as at least one point fails to transform (to be consistent with the other form of Transform() that doesn't take a "t" argument), whereas previously they would return FALSE only if no transformation was found. When FALSE is returned the pabSuccess[] or panErrorCodes[] arrays indicate which point succeeded or failed to transform.

    The GDALTransformerFunc callback and its implementations (GenImgProjTransformer, RPCTransformer, etc.) are also modified to return FALSE as soon as at least one point fails to transform.

@rouault rouault added this to the 3.11.0 milestone Feb 7, 2025
@rouault rouault added the funded through GSP Work funded through the GDAL Sponsorship Program label Feb 7, 2025
@ctoney
Copy link
Contributor

ctoney commented Feb 7, 2025

This seems a little awkward. If FALSE is returned, does it mean modification in-place still may have happened but we need to check pabSuccess to find out?

@rouault
Copy link
Member Author

rouault commented Feb 7, 2025

If FALSE is returned, does it mean modification in-place still may have happened but we need to check pabSuccess to find out?

yes. I can't really think of a better alternative

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 70.063% (+0.003%) from 70.06%
when pulling 063e2a8 on rouault:fix_11817
into e5044c3 on OSGeo:master.

@rouault rouault force-pushed the fix_11817 branch 2 times, most recently from 238d312 to 18b7dce Compare February 8, 2025 15:57
@rouault
Copy link
Member Author

rouault commented Feb 8, 2025

yes. I can't really think of a better alternative

well, there's an alternative, which would be to return the number of points that have succeeded in being transformed, which would allow quickly to discriminate all-failed, all-succeeded, mixed success, but that wouldn't be any longer a boolean return value.

@ctoney
Copy link
Contributor

ctoney commented Feb 8, 2025

That sounds good, but trade-offs for breaking change I would leave up to you.

It's not too bad the way it is. I ended with something like

    int res = poCT->Transform(pts_in.nrow(), xbuf.data(), ybuf.data(),
                              nullptr, nullptr, success.data());

    if (!res &&
        std::find(success.begin(), success.end(), TRUE) == success.end()) {

        // all failed
    } else if (!res) {
        // some failed
    } else {
        // all succeeded
    }

…nsformerFunc return FALSE as soon as one point fails to transform

Fixes OSGeo#11817

Kind of a breaking change, but the current behavior was highly
inconsistent and hard to reason about.

New paragraph in MIGRATION_GUIDE.TXT:

- The following methods
  OGRCoordinateTransformation::Transform(size_t nCount, double *x, double *y,
  double *z, double *t, int *pabSuccess) and
  OGRCoordinateTransformation::TransformWithErrorCodes(size_t nCount, double *x,
  double *y, double *z, double *t, int *panErrorCodes) are modified to return
  FALSE as soon as at least one point fails to transform (to be consistent with
  the other form of Transform() that doesn't take a "t" argument), whereas
  previously they would return FALSE only if no transformation was found. When
  FALSE is returned the pabSuccess[] or panErrorCodes[] arrays indicate which
  point succeeded or failed to transform.

  The GDALTransformerFunc callback and its implementations (GenImgProjTransformer,
  RPCTransformer, etc.) are also modified to return FALSE as soon as at least
  one point fails to transform.
alg/gdaltransformer.cpp Show resolved Hide resolved
@rouault rouault merged commit 2409c7c into OSGeo:master Feb 10, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
funded through GSP Work funded through the GDAL Sponsorship Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent return values from Transform methods in OGRCoordinateTransformation
4 participants