diff --git a/src/iso19111/io.cpp b/src/iso19111/io.cpp index 867e08ed8b..c11fc5dcbd 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 20bbce6f06..185ebb4a4f 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( + l_sourceCRS.get()) && + dynamic_cast( + 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(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 a60296d10a..e99072a8f7 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());