Merge pull request #2818 from rouault/fix_2817

ConcatenatedOperation::fixStepsDirection(): fix bad chaining of steps…

3 files changed, 133 insertions, 4 deletions

diff --git a/src/iso19111/io.cpp b/src/iso19111/io.cpp
index 867e08ed..c11fc5dc 100644
--- a/src/iso19111/io.cpp
+++ b/src/iso19111/io.cpp
@@ -7425,6 +7425,8 @@ const std::string &PROJStringFormatter::toString() const {
         } else if (step.name == "pop" && step.inverted) {
             step.name = "push";
             step.inverted = false;
+        } else if (step.name == "noop" && d->steps_.size() > 1) {
+            iter = d->steps_.erase(iter);
         } else {
             ++iter;
         }
diff --git a/src/iso19111/operation/concatenatedoperation.cpp b/src/iso19111/operation/concatenatedoperation.cpp
index 20bbce6f..185ebb4a 100644
--- a/src/iso19111/operation/concatenatedoperation.cpp
+++ b/src/iso19111/operation/concatenatedoperation.cpp
@@ -335,13 +335,26 @@ void ConcatenatedOperation::fixStepsDirection(
                 }
             }
         } else if (conv && i > 0 && i < operationsInOut.size() - 1) {
-            // For an intermediate conversion, use the target CRS of the
-            // previous step and the source CRS of the next step
+
             l_sourceCRS = operationsInOut[i - 1]->targetCRS();
             l_targetCRS = operationsInOut[i + 1]->sourceCRS();
+            // For an intermediate conversion, use the target CRS of the
+            // previous step and the source CRS of the next step
             if (l_sourceCRS && l_targetCRS) {
-                op->setCRSs(NN_NO_CHECK(l_sourceCRS), NN_NO_CHECK(l_targetCRS),
-                            nullptr);
+                // If the sourceCRS is a projectedCRS and the target a
+                // geographic one, then we must inverse the operation. See
+                // https://github.com/OSGeo/PROJ/issues/2817
+                if (dynamic_cast<const crs::ProjectedCRS *>(
+                        l_sourceCRS.get()) &&
+                    dynamic_cast<const crs::GeographicCRS *>(
+                        l_targetCRS.get())) {
+                    op->setCRSs(NN_NO_CHECK(l_targetCRS),
+                                NN_NO_CHECK(l_sourceCRS), nullptr);
+                    op = op->inverse();
+                } else {
+                    op->setCRSs(NN_NO_CHECK(l_sourceCRS),
+                                NN_NO_CHECK(l_targetCRS), nullptr);
+                }
             } else if (l_sourceCRS && l_targetCRS == nullptr &&
                        conv->method()->getEPSGCode() ==
                            EPSG_CODE_METHOD_HEIGHT_DEPTH_REVERSAL) {
@@ -380,6 +393,11 @@ void ConcatenatedOperation::fixStepsDirection(
             // whereas we should instead use the reverse path.
             auto prevOpTarget = (i == 0) ? concatOpSourceCRS.as_nullable()
                                          : operationsInOut[i - 1]->targetCRS();
+            if (prevOpTarget == nullptr) {
+                throw InvalidOperation(
+                    "Cannot determine targetCRS of operation at step " +
+                    toString(static_cast<int>(i)));
+            }
             if (compareStepCRS(l_sourceCRS.get(), prevOpTarget.get())) {
                 // do nothing
             } else if (compareStepCRS(l_targetCRS.get(), prevOpTarget.get())) {
diff --git a/test/unit/test_factory.cpp b/test/unit/test_factory.cpp
index a60296d1..e99072a8 100644
--- a/test/unit/test_factory.cpp
+++ b/test/unit/test_factory.cpp
@@ -3117,6 +3117,115 @@ TEST_F(FactoryWithTmpDatabase,
 
 // ---------------------------------------------------------------------------
 
+TEST_F(FactoryWithTmpDatabase,
+       check_fixup_direction_concatenated_inverse_map_projection) {
+
+    // This tests https://github.com/OSGeo/PROJ/issues/2817
+
+    createStructure();
+    populateWithFakeEPSG();
+
+    ASSERT_TRUE(execute(
+        "INSERT INTO other_transformation "
+        "VALUES('EPSG','NOOP_TRANSFORMATION_32631','name',NULL,"
+        "'PROJ','PROJString','+proj=noop',"
+        "'EPSG','32631','EPSG','32631',0.0,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,0);"))
+        << last_error();
+
+    ASSERT_TRUE(
+        execute("INSERT INTO usage VALUES('EPSG',"
+                "'other_transformation_NOOP_TRANSFORMATION_32631_usage',"
+                "'other_transformation',"
+                "'EPSG','NOOP_TRANSFORMATION_32631',"
+                "'EPSG','1262','EPSG','1024');"))
+        << last_error();
+
+    ASSERT_TRUE(execute(
+        "INSERT INTO other_transformation "
+        "VALUES('EPSG','NOOP_TRANSFORMATION_4326','name',NULL,"
+        "'PROJ','PROJString','+proj=noop',"
+        "'EPSG','4326','EPSG','4326',0.0,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,"
+        "NULL,NULL,NULL,NULL,NULL,NULL,0);"))
+        << last_error();
+
+    ASSERT_TRUE(execute("INSERT INTO usage VALUES('EPSG',"
+                        "'other_transformation_NOOP_TRANSFORMATION_4326_usage',"
+                        "'other_transformation',"
+                        "'EPSG','NOOP_TRANSFORMATION_4326',"
+                        "'EPSG','1262','EPSG','1024');"))
+        << last_error();
+
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation "
+                        "VALUES('EPSG','TEST_CONCATENATED','name',NULL,"
+                        "'EPSG','4326','EPSG'"
+                        ",'4326',NULL,NULL,0);"))
+        << last_error();
+    ASSERT_TRUE(execute("INSERT INTO usage VALUES('EPSG',"
+                        "'concatenated_operation_TEST_CONCATENATED_usage',"
+                        "'concatenated_operation',"
+                        "'EPSG','TEST_CONCATENATED',"
+                        "'EPSG','1262','EPSG','1024');"))
+        << last_error();
+
+    // Forward map projection
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation_step "
+                        "VALUES('EPSG','TEST_CONCATENATED',1,"
+                        "'EPSG','16031');"))
+        << last_error();
+
+    // Noop projected
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation_step "
+                        "VALUES('EPSG','TEST_CONCATENATED',2,"
+                        "'EPSG','NOOP_TRANSFORMATION_32631');"))
+        << last_error();
+
+    // Inverse map projection
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation_step "
+                        "VALUES('EPSG','TEST_CONCATENATED',3,"
+                        "'EPSG','16031');"))
+        << last_error();
+
+    // Noop geographic
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation_step "
+                        "VALUES('EPSG','TEST_CONCATENATED',4,"
+                        "'EPSG','NOOP_TRANSFORMATION_4326');"))
+        << last_error();
+
+    // Forward map projection
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation_step "
+                        "VALUES('EPSG','TEST_CONCATENATED',5,"
+                        "'EPSG','16031');"))
+        << last_error();
+
+    // Noop projected
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation_step "
+                        "VALUES('EPSG','TEST_CONCATENATED',6,"
+                        "'EPSG','NOOP_TRANSFORMATION_32631');"))
+        << last_error();
+
+    // Inverse map projection
+    ASSERT_TRUE(execute("INSERT INTO concatenated_operation_step "
+                        "VALUES('EPSG','TEST_CONCATENATED',7,"
+                        "'EPSG','16031');"))
+        << last_error();
+
+    auto dbContext = DatabaseContext::create(m_ctxt);
+    auto authFactory = AuthorityFactory::create(dbContext, std::string("EPSG"));
+    const auto op =
+        authFactory->createCoordinateOperation("TEST_CONCATENATED", false);
+    auto wkt = op->exportToPROJString(PROJStringFormatter::create().get());
+    EXPECT_EQ(wkt, "+proj=noop");
+}
+
+// ---------------------------------------------------------------------------
+
 TEST(factory, createObjectsFromName) {
     auto ctxt = DatabaseContext::create();
     auto factory = AuthorityFactory::create(ctxt, std::string());