Skip to content

Commit

Permalink
Merge pull request #2818 from rouault/fix_2817
Browse files Browse the repository at this point in the history 
ConcatenatedOperation::fixStepsDirection(): fix bad chaining of steps…
  • Loading branch information
@rouault @github-actions
rouault authored and github-actions[bot] committed Aug 20, 2021
1 parent ff682bf commit c26bc40
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 4 deletions.
2 changes: 2 additions & 0 deletions src/iso19111/io.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
26 changes: 22 additions & 4 deletions src/iso19111/operation/concatenatedoperation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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())) {
Expand Down
109 changes: 109 additions & 0 deletions test/unit/test_factory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down

0 comments on commit c26bc40

Please sign in to comment.