Merge pull request #11605 from rouault/GetDefaultArcStepSize

Add a OGRGeometryFactory::GetDefaultArcStepSize() method to get value…
OSGeo · Jan 9, 2025 · 39cbe84 · 39cbe84
2 parents 3358a9d + fadf31f
commit 39cbe84
Show file tree

Hide file tree

Showing 9 changed files with 79 additions and 34 deletions.
diff --git a/autotest/cpp/test_ogr.cpp b/autotest/cpp/test_ogr.cpp
@@ -4571,4 +4571,31 @@ TEST_F(test_ogr, OGRFieldDefnGetFieldTypeByName)
     }
 }
 
+// Test OGRGeometryFactory::GetDefaultArcStepSize()
+TEST_F(test_ogr, GetDefaultArcStepSize)
+{
+    if (CPLGetConfigOption("OGR_ARC_STEPSIZE", nullptr) == nullptr)
+    {
+        EXPECT_EQ(OGRGeometryFactory::GetDefaultArcStepSize(), 4.0);
+    }
+    {
+        CPLConfigOptionSetter oSetter("OGR_ARC_STEPSIZE", "0.00001",
+                                      /* bSetOnlyIfUndefined = */ false);
+        CPLErrorHandlerPusher oErrorHandler(CPLQuietErrorHandler);
+        EXPECT_EQ(OGRGeometryFactory::GetDefaultArcStepSize(), 1e-2);
+        EXPECT_TRUE(
+            strstr(CPLGetLastErrorMsg(),
+                   "Too small value for OGR_ARC_STEPSIZE. Clamping it to"));
+    }
+    {
+        CPLConfigOptionSetter oSetter("OGR_ARC_STEPSIZE", "190",
+                                      /* bSetOnlyIfUndefined = */ false);
+        CPLErrorHandlerPusher oErrorHandler(CPLQuietErrorHandler);
+        EXPECT_EQ(OGRGeometryFactory::GetDefaultArcStepSize(), 180);
+        EXPECT_TRUE(
+            strstr(CPLGetLastErrorMsg(),
+                   "Too large value for OGR_ARC_STEPSIZE. Clamping it to"));
+    }
+}
+
 }  // namespace
diff --git a/ogr/gml2ogrgeometry.cpp b/ogr/gml2ogrgeometry.cpp
@@ -1876,9 +1876,7 @@ GML2OGRGeometry_XMLNode_Internal(const CPLXMLNode *psNode, const char *pszId,
         if (bSRSUnitIsDegree && dfUOMConv > 0)
         {
             auto poLS = std::make_unique<OGRLineString>();
-            // coverity[tainted_data]
-            const double dfStep =
-                CPLAtof(CPLGetConfigOption("OGR_ARC_STEPSIZE", "4"));
+            const double dfStep = OGRGeometryFactory::GetDefaultArcStepSize();
             const double dfSign = dfStartAngle < dfEndAngle ? 1 : -1;
             for (double dfAngle = dfStartAngle;
                  (dfAngle - dfEndAngle) * dfSign < 0;
@@ -2010,8 +2008,7 @@ GML2OGRGeometry_XMLNode_Internal(const CPLXMLNode *psNode, const char *pszId,
         if (bSRSUnitIsDegree && dfUOMConv > 0)
         {
             auto poLS = std::make_unique<OGRLineString>();
-            const double dfStep =
-                CPLAtof(CPLGetConfigOption("OGR_ARC_STEPSIZE", "4"));
+            const double dfStep = OGRGeometryFactory::GetDefaultArcStepSize();
             for (double dfAngle = 0; dfAngle < 360; dfAngle += dfStep)
             {
                 double dfLong = 0.0;

diff --git a/ogr/ogr_geometry.h b/ogr/ogr_geometry.h
@@ -4381,6 +4381,8 @@ class CPL_DLL OGRGeometryFactory
         char **papszOptions,
         const TransformWithOptionsCache &cache = TransformWithOptionsCache());
 
+    static double GetDefaultArcStepSize();
+
     static OGRGeometry *
     approximateArcAngles(double dfX, double dfY, double dfZ,
                          double dfPrimaryRadius, double dfSecondaryAxis,

diff --git a/ogr/ogrgeometryfactory.cpp b/ogr/ogrgeometryfactory.cpp
@@ -4197,13 +4197,47 @@ void OGR_GeomTransformer_Destroy(OGRGeomTransformerH hTransformer)
 }
 
 /************************************************************************/
-/*                       OGRGF_GetDefaultStepSize()                     */
+/*                OGRGeometryFactory::GetDefaultArcStepSize()           */
 /************************************************************************/
 
-static double OGRGF_GetDefaultStepSize()
+/** Return the default value of the angular step used when stroking curves
+ * as lines. Defaults to 4 degrees.
+ * Can be modified by setting the OGR_ARC_STEPSIZE configuration option.
+ * Valid values are in [1e-2, 180] degree range.
+ * @since 3.11
+ */
+
+/* static */
+double OGRGeometryFactory::GetDefaultArcStepSize()
 {
-    // coverity[tainted_data]
-    return CPLAtofM(CPLGetConfigOption("OGR_ARC_STEPSIZE", "4"));
+    const double dfVal = CPLAtofM(CPLGetConfigOption("OGR_ARC_STEPSIZE", "4"));
+    constexpr double MIN_VAL = 1e-2;
+    if (dfVal < MIN_VAL)
+    {
+        static bool bWarned = false;
+        if (!bWarned)
+        {
+            bWarned = true;
+            CPLError(CE_Warning, CPLE_AppDefined,
+                     "Too small value for OGR_ARC_STEPSIZE. Clamping it to %f",
+                     MIN_VAL);
+        }
+        return MIN_VAL;
+    }
+    constexpr double MAX_VAL = 180;
+    if (dfVal > MAX_VAL)
+    {
+        static bool bWarned = false;
+        if (!bWarned)
+        {
+            bWarned = true;
+            CPLError(CE_Warning, CPLE_AppDefined,
+                     "Too large value for OGR_ARC_STEPSIZE. Clamping it to %f",
+                     MAX_VAL);
+        }
+        return MAX_VAL;
+    }
+    return dfVal;
 }
 
 /************************************************************************/
@@ -4266,7 +4300,7 @@ OGRGeometry *OGRGeometryFactory::approximateArcAngles(
     // Support default arc step setting.
     if (dfMaxAngleStepSizeDegrees < 1e-6)
     {
-        dfMaxAngleStepSizeDegrees = OGRGF_GetDefaultStepSize();
+        dfMaxAngleStepSizeDegrees = OGRGeometryFactory::GetDefaultArcStepSize();
     }
 
     // Determine maximum interpolation gap. This is the largest straight-line
@@ -5459,7 +5493,7 @@ OGRLineString *OGRGeometryFactory::curveToLineString(
     // support default arc step setting.
     if (dfMaxAngleStepSizeDegrees < 1e-6)
     {
-        dfMaxAngleStepSizeDegrees = OGRGF_GetDefaultStepSize();
+        dfMaxAngleStepSizeDegrees = OGRGeometryFactory::GetDefaultArcStepSize();
     }
 
     double dfStep = dfMaxAngleStepSizeDegrees / 180 * M_PI;

diff --git a/ogr/ogrpgeogeometry.cpp b/ogr/ogrpgeogeometry.cpp
@@ -1835,10 +1835,8 @@ static OGRCurve *OGRShapeCreateCompoundCurve(int nPartStartIdx, int nPartPoints,
                 dfStartAngle += 2 * M_PI;
             else if (dfEndAngle + M_PI < dfStartAngle)
                 dfEndAngle += 2 * M_PI;
-            // coverity[tainted_data]
             const double dfStepSizeRad =
-                CPLAtofM(CPLGetConfigOption("OGR_ARC_STEPSIZE", "4")) / 180.0 *
-                M_PI;
+                OGRGeometryFactory::GetDefaultArcStepSize() / 180.0 * M_PI;
             const double dfLengthTangentStart =
                 (dfX1 - dfX0) * (dfX1 - dfX0) + (dfY1 - dfY0) * (dfY1 - dfY0);
             const double dfLengthTangentEnd =

diff --git a/ogr/ogrsf_frmts/dwg/ogrdgnv8layer.cpp b/ogr/ogrsf_frmts/dwg/ogrdgnv8layer.cpp
@@ -574,7 +574,7 @@ static void ProcessCurve(OGRFeature *poFeature, const CPLString &osPen,
         else
             poSC = new OGRLineString();
         const double dfArcStepSize =
-            CPLAtofM(CPLGetConfigOption("OGR_ARC_STEPSIZE", "4"));
+            OGRGeometryFactory::GetDefaultArcStepSize();
         if (!ellipse.isNull())
         {
             nPoints = std::max(2, static_cast<int>(360 / dfArcStepSize));

diff --git a/ogr/ogrsf_frmts/ili/ogrili1datasource.cpp b/ogr/ogrsf_frmts/ili/ogrili1datasource.cpp
@@ -148,19 +148,12 @@ int OGRILI1DataSource::Open(const char *pszNewName, char **papszOpenOptionsIn,
     if (osModelFilename.length() > 0)
         poReader->ReadModel(poImdReader, osModelFilename.c_str(), this);
 
-    int bResetConfigOption = FALSE;
-    if (EQUAL(CPLGetConfigOption("OGR_ARC_STEPSIZE", ""), ""))
-    {
-        bResetConfigOption = TRUE;
-        CPLSetThreadLocalConfigOption("OGR_ARC_STEPSIZE", "0.96");
-    }
+    CPLConfigOptionSetter oSetter("OGR_ARC_STEPSIZE", "0.96",
+                                  /* bSetOnlyIfUndefined = */ true);
 
     // Parse model and read data - without surface join and area polygonizing.
     poReader->ReadFeatures();
 
-    if (bResetConfigOption)
-        CPLSetThreadLocalConfigOption("OGR_ARC_STEPSIZE", nullptr);
-
     return TRUE;
 }
 

diff --git a/ogr/ogrsf_frmts/ili/ogrili1layer.cpp b/ogr/ogrsf_frmts/ili/ogrili1layer.cpp
@@ -444,12 +444,9 @@ OGRErr OGRILI1Layer::CreateField(const OGRFieldDefn *poField,
 void OGRILI1Layer::JoinGeomLayers()
 {
     bGeomsJoined = true;
-    bool bResetConfigOption = false;
-    if (EQUAL(CPLGetConfigOption("OGR_ARC_STEPSIZE", ""), ""))
-    {
-        bResetConfigOption = true;
-        CPLSetThreadLocalConfigOption("OGR_ARC_STEPSIZE", "0.96");
-    }
+
+    CPLConfigOptionSetter oSetter("OGR_ARC_STEPSIZE", "0.96",
+                                  /* bSetOnlyIfUndefined = */ true);
 
     for (GeomFieldInfos::const_iterator it = oGeomFieldInfos.begin();
          it != oGeomFieldInfos.end(); ++it)
@@ -477,9 +474,6 @@ void OGRILI1Layer::JoinGeomLayers()
             }
         }
     }
-
-    if (bResetConfigOption)
-        CPLSetThreadLocalConfigOption("OGR_ARC_STEPSIZE", nullptr);
 }
 
 void OGRILI1Layer::JoinSurfaceLayer(OGRILI1Layer *poSurfaceLineLayer,

diff --git a/port/cpl_known_config_options.h b/port/cpl_known_config_options.h
@@ -664,7 +664,7 @@ constexpr static const char* const apszKnownConfigOptions[] =
    "OGR_API_SPY_SNAPSHOT_PATH", // from ograpispy.cpp
    "OGR_APPLY_GEOM_SET_PRECISION", // from ogr2ogr_lib.cpp, ogrlayer.cpp
    "OGR_ARC_MAX_GAP", // from ogrgeometryfactory.cpp
-   "OGR_ARC_STEPSIZE", // from gml2ogrgeometry.cpp, ogrdgnv8layer.cpp, ogrgeometryfactory.cpp, ogrili1datasource.cpp, ogrili1layer.cpp, ogrpgeogeometry.cpp
+   "OGR_ARC_STEPSIZE", // from ogrgeometryfactory.cpp
    "OGR_ARROW_COMPUTE_GEOMETRY_TYPE", // from ogrfeatherlayer.cpp
    "OGR_ARROW_LOAD_FILE_SYSTEM_FACTORIES", // from ogrfeatherdriver.cpp
    "OGR_ARROW_MEM_LIMIT", // from ograrrowarrayhelper.cpp