From 685a64466a3bc25a8d0bdb0b0f932ad7e977620d Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 25 Nov 2019 04:54:17 +0100 Subject: normalizeForVisualization() and other methods applying on a ProjectedCRS: do not mess the derivingConversion object of the original object (fixes #1736) normalizeForVisualization(), promoteTo3D(), demoteTo2D(), alterGeodeticCRS(), alterCSLinearUnit() and alterParametersLinearUnit() all used the object returned by derivingConversionRef() to create a new ProjectedCRS. While doing so, this caused the derivingConversion of the original object to have its targetCRS set to the object returned by normalizeForVisualization() and similar. If that object died, then the weak pointer would be reset, and the original ProjectedCRS() has now its derivingConversionRef()->targetCRS() nullptr So bottom line is use derivingConversion() for anything that is not pure reading !!! This is confirmed to be the fix for the QGIS scenario in https://github.com/qgis/QGIS/issues/30569#issuecomment-540060919 In QGIS use case, the issue arised when using a projected CRS with a non-GIS friendly axis (that is where normalizeForVisualization() created a new projectedCRS) --- include/proj/crs.hpp | 3 ++ src/iso19111/crs.cpp | 21 +++++++------ test/unit/test_crs.cpp | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 11 deletions(-) diff --git a/include/proj/crs.hpp b/include/proj/crs.hpp index 2b919c71..356c29d9 100644 --- a/include/proj/crs.hpp +++ b/include/proj/crs.hpp @@ -514,6 +514,9 @@ class PROJ_GCC_DLL DerivedCRS : virtual public SingleCRS { PROJ_PRIVATE : //! @cond Doxygen_Suppress + + // Use this method with extreme care ! It should never be used + // to recreate a new Derived/ProjectedCRS ! PROJ_INTERNAL const operation::ConversionNNPtr & derivingConversionRef() PROJ_PURE_DECL; diff --git a/src/iso19111/crs.cpp b/src/iso19111/crs.cpp index cf533bd3..eb15b0d8 100644 --- a/src/iso19111/crs.cpp +++ b/src/iso19111/crs.cpp @@ -237,7 +237,7 @@ CRSNNPtr CRS::alterGeodeticCRS(const GeodeticCRSNNPtr &newGeodCRS) const { auto projCRS = dynamic_cast(this); if (projCRS) { return ProjectedCRS::create(createPropertyMap(this), newGeodCRS, - projCRS->derivingConversionRef(), + projCRS->derivingConversion(), projCRS->coordinateSystem()); } @@ -264,7 +264,7 @@ CRSNNPtr CRS::alterCSLinearUnit(const common::UnitOfMeasure &unit) const { if (projCRS) { return ProjectedCRS::create( createPropertyMap(this), projCRS->baseCRS(), - projCRS->derivingConversionRef(), + projCRS->derivingConversion(), projCRS->coordinateSystem()->alterUnit(unit)); } } @@ -675,9 +675,8 @@ CRSNNPtr CRS::normalizeForVisualization() const { axisList[0]) : cs::CartesianCS::create(util::PropertyMap(), axisList[1], axisList[0], axisList[2]); - return util::nn_static_pointer_cast( - ProjectedCRS::create(props, projCRS->baseCRS(), - projCRS->derivingConversionRef(), cs)); + return util::nn_static_pointer_cast(ProjectedCRS::create( + props, projCRS->baseCRS(), projCRS->derivingConversion(), cs)); } } @@ -844,7 +843,7 @@ CRSNNPtr CRS::promoteTo3D(const std::string &newName, !newName.empty() ? newName : nameStr()), NN_NO_CHECK( util::nn_dynamic_pointer_cast(base3DCRS)), - projCRS->derivingConversionRef(), cs)); + projCRS->derivingConversion(), cs)); } } @@ -3379,10 +3378,10 @@ bool ProjectedCRS::_isEquivalentTo( ProjectedCRSNNPtr ProjectedCRS::alterParametersLinearUnit(const common::UnitOfMeasure &unit, bool convertToNewUnit) const { - return create(createPropertyMap(this), baseCRS(), - derivingConversionRef()->alterParametersLinearUnit( - unit, convertToNewUnit), - coordinateSystem()); + return create( + createPropertyMap(this), baseCRS(), + derivingConversion()->alterParametersLinearUnit(unit, convertToNewUnit), + coordinateSystem()); } //! @endcond @@ -3791,7 +3790,7 @@ ProjectedCRS::demoteTo2D(const std::string &newName, return ProjectedCRS::create( util::PropertyMap().set(common::IdentifiedObject::NAME_KEY, !newName.empty() ? newName : nameStr()), - newBaseCRS, derivingConversionRef(), cs); + newBaseCRS, derivingConversion(), cs); } return NN_NO_CHECK(std::dynamic_pointer_cast( diff --git a/test/unit/test_crs.cpp b/test/unit/test_crs.cpp index 24f4ba52..427fd741 100644 --- a/test/unit/test_crs.cpp +++ b/test/unit/test_crs.cpp @@ -5320,3 +5320,83 @@ TEST(crs, promoteTo3D_and_demoteTo2D) { nullptr); } } + +// --------------------------------------------------------------------------- + +TEST(crs, projected_normalizeForVisualization_do_not_mess_deriving_conversion) { + + auto authFactory = + AuthorityFactory::create(DatabaseContext::create(), "EPSG"); + // Something with non standard order + auto projCRS = authFactory->createProjectedCRS("3035"); + { + auto src = GeographicCRS::EPSG_4326; + auto op = + CoordinateOperationFactory::create()->createOperation(src, projCRS); + ASSERT_TRUE(op != nullptr); + // Make sure to run that in a scope, so that the object get destroyed + op->normalizeForVisualization(); + } + EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get()); +} + +// --------------------------------------------------------------------------- + +TEST(crs, projected_promoteTo3D_do_not_mess_deriving_conversion) { + + auto projCRS = createProjected(); + { + // Make sure to run that in a scope, so that the object get destroyed + projCRS->promoteTo3D(std::string(), nullptr); + } + EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get()); +} + +// --------------------------------------------------------------------------- + +TEST(crs, projected_demoteTo2D_do_not_mess_deriving_conversion) { + + auto projCRS = nn_dynamic_pointer_cast( + createProjected()->promoteTo3D(std::string(), nullptr)); + { + // Make sure to run that in a scope, so that the object get destroyed + projCRS->demoteTo2D(std::string(), nullptr); + } + EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get()); +} + +// --------------------------------------------------------------------------- + +TEST(crs, projected_alterGeodeticCRS_do_not_mess_deriving_conversion) { + + auto projCRS = createProjected(); + { + // Make sure to run that in a scope, so that the object get destroyed + projCRS->alterGeodeticCRS(NN_NO_CHECK(projCRS->extractGeographicCRS())); + } + EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get()); +} + +// --------------------------------------------------------------------------- + +TEST(crs, projected_alterCSLinearUnit_do_not_mess_deriving_conversion) { + + auto projCRS = createProjected(); + { + // Make sure to run that in a scope, so that the object get destroyed + projCRS->alterCSLinearUnit(UnitOfMeasure("my unit", 2)); + } + EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get()); +} + +// --------------------------------------------------------------------------- + +TEST(crs, projected_alterParametersLinearUnit_do_not_mess_deriving_conversion) { + + auto projCRS = createProjected(); + { + // Make sure to run that in a scope, so that the object get destroyed + projCRS->alterParametersLinearUnit(UnitOfMeasure::METRE, false); + } + EXPECT_EQ(projCRS->derivingConversion()->targetCRS().get(), projCRS.get()); +} -- cgit v1.2.3