Add some thread safety to Qgs3DMapSettings usage

Create a small, cheap to copy (non-qobject) class Qgs3DMapSettingsSnapshot which is designed to store just cheap properties of Qgs3DMapSettings. Then use this object wherever possible to avoid accessing the (non-thread safe) Qgs3DMapSettings object for retrieval of simple map properties (eg crs, extent, ...) Refs qgis/QGIS-Enhancement-Proposals#301
nyalldawson · Aug 26, 2024 · 6b203de · 6b203de
1 parent 8ab7175
commit 6b203de
Show file tree

Hide file tree

Showing 44 changed files with 421 additions and 90 deletions.
diff --git a/python/3d/auto_generated/qgs3dmapsettings.sip.in b/python/3d/auto_generated/qgs3dmapsettings.sip.in
@@ -16,6 +16,12 @@ class Qgs3DMapSettings : QObject, QgsTemporalRangeObject
 {
 %Docstring(signature="appended")
 Definition of the world.
+
+.. warning::
+
+   Qgs3DMapSettings are a QObject subclass, and accordingly are not
+   safe for access across different threads. See Qgs3DMapSettingsSnapshot instead
+   for a safe snapshot of settings from Qgs3DMapSettings.
 %End
 
 %TypeHeaderCode
@@ -28,6 +34,7 @@ Definition of the world.
     ~Qgs3DMapSettings();
 
 
+
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context );
 %Docstring
 Reads configuration from a DOM element previously written by :py:func:`~Qgs3DMapSettings.writeXml`

diff --git a/python/PyQt6/3d/auto_generated/qgs3dmapsettings.sip.in b/python/PyQt6/3d/auto_generated/qgs3dmapsettings.sip.in
@@ -16,6 +16,12 @@ class Qgs3DMapSettings : QObject, QgsTemporalRangeObject
 {
 %Docstring(signature="appended")
 Definition of the world.
+
+.. warning::
+
+   Qgs3DMapSettings are a QObject subclass, and accordingly are not
+   safe for access across different threads. See Qgs3DMapSettingsSnapshot instead
+   for a safe snapshot of settings from Qgs3DMapSettings.
 %End
 
 %TypeHeaderCode
@@ -28,6 +34,7 @@ Definition of the world.
     ~Qgs3DMapSettings();
 
 
+
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context );
 %Docstring
 Reads configuration from a DOM element previously written by :py:func:`~Qgs3DMapSettings.writeXml`

diff --git a/src/3d/CMakeLists.txt b/src/3d/CMakeLists.txt
@@ -13,6 +13,7 @@ set(QGIS_3D_SRCS
   qgs3dmapexportsettings.cpp
   qgs3dmapscene.cpp
   qgs3dmapsettings.cpp
+  qgs3dmapsettingssnapshot.cpp
   qgs3dmaptool.cpp
   qgs3dmapcanvas.cpp
   qgs3dsceneexporter.cpp
@@ -120,6 +121,7 @@ set(QGIS_3D_HDRS
   qgs3daxissettings.h
   qgs3dmapscene.h
   qgs3dmapsettings.h
+  qgs3dmapsettingssnapshot.h
   qgs3dmaptool.h
   qgs3dsceneexporter.h
   qgs3dtypes.h

diff --git a/src/3d/mesh/qgsmesh3dentity_p.cpp b/src/3d/mesh/qgsmesh3dentity_p.cpp
@@ -20,12 +20,12 @@
 #include <Qt3DRender/QGeometryRenderer>
 
 #include "qgsmeshlayer.h"
-#include "qgs3dmapsettings.h"
+#include "qgs3dmapsettingssnapshot.h"
 #include "qgsmesh3dmaterial_p.h"
 
 
 
-QgsMesh3DEntity::QgsMesh3DEntity( const Qgs3DMapSettings &map,
+QgsMesh3DEntity::QgsMesh3DEntity( const Qgs3DMapSettingsSnapshot &map,
                                   const QgsTriangularMesh &triangularMesh,
                                   const QgsMesh3DSymbol *symbol )
   : mMapSettings( map )
@@ -34,7 +34,7 @@ QgsMesh3DEntity::QgsMesh3DEntity( const Qgs3DMapSettings &map,
 {}
 
 QgsMeshDataset3DEntity::QgsMeshDataset3DEntity(
-  const Qgs3DMapSettings &map,
+  const Qgs3DMapSettingsSnapshot &map,
   const QgsTriangularMesh &triangularMesh,
   QgsMeshLayer *meshLayer,
   const QgsMesh3DSymbol *symbol )
@@ -77,7 +77,7 @@ QgsMeshLayer *QgsMeshDataset3DEntity::layer() const
 }
 
 QgsMesh3DTerrainTileEntity::QgsMesh3DTerrainTileEntity(
-  const Qgs3DMapSettings &map,
+  const Qgs3DMapSettingsSnapshot &map,
   const QgsTriangularMesh &triangularMesh,
   const QgsMesh3DSymbol *symbol,
   QgsChunkNodeId nodeId,

diff --git a/src/3d/mesh/qgsmesh3dentity_p.h b/src/3d/mesh/qgsmesh3dentity_p.h
@@ -21,7 +21,7 @@
 #include <Qt3DCore/QEntity>
 
 #include "mesh/qgsmesh3dgeometry_p.h"
-#include "qgs3dmapsettings.h"
+#include "qgs3dmapsettingssnapshot.h"
 #include "qgsmesh3dsymbol.h"
 #include "qgsterraintileentity_p.h"
 
@@ -52,13 +52,13 @@ class QgsMesh3DEntity
     void build();
   protected:
     //! Constructor
-    QgsMesh3DEntity( const Qgs3DMapSettings &map,
+    QgsMesh3DEntity( const Qgs3DMapSettingsSnapshot &map,
                      const QgsTriangularMesh &triangularMesh,
                      const QgsMesh3DSymbol *symbol );
 
     virtual ~QgsMesh3DEntity() = default;
 
-    Qgs3DMapSettings mMapSettings;
+    Qgs3DMapSettingsSnapshot mMapSettings;
     QgsTriangularMesh mTriangularMesh;
     std::unique_ptr< QgsMesh3DSymbol > mSymbol;
 
@@ -74,7 +74,7 @@ class QgsMeshDataset3DEntity: public Qt3DCore::QEntity, public QgsMesh3DEntity
 
   public:
     //! Constructor
-    QgsMeshDataset3DEntity( const Qgs3DMapSettings &map,
+    QgsMeshDataset3DEntity( const Qgs3DMapSettingsSnapshot &map,
                             const QgsTriangularMesh &triangularMesh,
                             QgsMeshLayer *meshLayer,
                             const QgsMesh3DSymbol *symbol );
@@ -94,7 +94,7 @@ class QgsMesh3DTerrainTileEntity: public QgsTerrainTileEntity, public QgsMesh3DE
     Q_OBJECT
 
   public:
-    QgsMesh3DTerrainTileEntity( const Qgs3DMapSettings &map,
+    QgsMesh3DTerrainTileEntity( const Qgs3DMapSettingsSnapshot &map,
                                 const QgsTriangularMesh &triangularMesh,
                                 const QgsMesh3DSymbol *symbol,
                                 QgsChunkNodeId nodeId,

diff --git a/src/3d/mesh/qgsmeshterraingenerator.cpp b/src/3d/mesh/qgsmeshterraingenerator.cpp
@@ -25,7 +25,7 @@
 #include "qgsterrainentity_p.h"
 #include "qgsterraintextureimage_p.h"
 #include "qgsmeshlayerutils.h"
-
+#include "qgs3dmapsettings.h"
 
 QgsMeshTerrainTileLoader::QgsMeshTerrainTileLoader( QgsTerrainEntity *terrain, QgsChunkNode *node, const QgsTriangularMesh &triangularMesh, const QgsMesh3DSymbol *symbol )
   : QgsTerrainTileLoader( terrain, node )
@@ -37,7 +37,7 @@ QgsMeshTerrainTileLoader::QgsMeshTerrainTileLoader( QgsTerrainEntity *terrain, Q
 
 Qt3DCore::QEntity *QgsMeshTerrainTileLoader::createEntity( Qt3DCore::QEntity *parent )
 {
-  QgsMesh3DTerrainTileEntity *entity = new QgsMesh3DTerrainTileEntity( terrain()->map3D(), mTriangularMesh, mSymbol.get(), mNode->tileId(), parent );
+  QgsMesh3DTerrainTileEntity *entity = new QgsMesh3DTerrainTileEntity( terrain()->map3D().snapshot(), mTriangularMesh, mSymbol.get(), mNode->tileId(), parent );
   entity->build();
   createTexture( entity );
 
@@ -145,7 +145,7 @@ void QgsMeshTerrainGenerator::readXml( const QDomElement &elem )
   mSymbol->readXml( elem.firstChildElement( "symbol" ), rwc );
 }
 
-float QgsMeshTerrainGenerator::heightAt( double x, double y, const Qgs3DMapSettings & ) const
+float QgsMeshTerrainGenerator::heightAt( double x, double y, const Qgs3DMapSettingsSnapshot & ) const
 {
   return QgsMeshLayerUtils::interpolateZForPoint( mTriangularMesh, x, y );
 }

diff --git a/src/3d/mesh/qgsmeshterraingenerator.h b/src/3d/mesh/qgsmeshterraingenerator.h
@@ -85,7 +85,7 @@ class _3D_EXPORT QgsMeshTerrainGenerator: public QgsTerrainGenerator
     QgsRectangle rootChunkExtent() const override;
     void writeXml( QDomElement &elem ) const override;
     void readXml( const QDomElement &elem ) override;
-    float heightAt( double x, double y, const Qgs3DMapSettings & ) const override;
+    float heightAt( double x, double y, const Qgs3DMapSettingsSnapshot &map ) const override;
 
   private slots:
     void updateTriangularMesh();

diff --git a/src/3d/qgs3dmapsettings.cpp b/src/3d/qgs3dmapsettings.cpp
@@ -21,16 +21,15 @@
 #include "qgsmeshterraingenerator.h"
 #include "qgsonlineterraingenerator.h"
 #include "qgsprojectviewsettings.h"
-#include "qgsvectorlayer3drenderer.h"
-#include "qgsmeshlayer3drenderer.h"
-#include "qgspointcloudlayer3drenderer.h"
 #include "qgsprojectelevationproperties.h"
 #include "qgsterrainprovider.h"
 #include "qgslightsource.h"
 #include "qgscolorutils.h"
 #include "qgsrasterlayer.h"
 #include "qgspointlightsettings.h"
 #include "qgsdirectionallightsettings.h"
+#include "qgsthreadingutils.h"
+#include "qgs3dmapsettingssnapshot.h"
 
 #include <QDomDocument>
 #include <QDomElement>
@@ -118,6 +117,25 @@ Qgs3DMapSettings::~Qgs3DMapSettings()
   qDeleteAll( mLightSources );
 }
 
+Qgs3DMapSettingsSnapshot Qgs3DMapSettings::snapshot() const
+{
+  QGIS_PROTECT_QOBJECT_THREAD_ACCESS
+
+  Qgs3DMapSettingsSnapshot res;
+  res.setCrs( mCrs );
+  res.setTransformContext( mTransformContext );
+  res.setOrigin( mOrigin );
+  res.setExtent( mExtent );
+  res.setTemporalRange( temporalRange() );
+  res.setSelectionColor( mSelectionColor );
+  res.setOutputDpi( mDpi );
+  res.setFieldOfView( mFieldOfView );
+  res.setTerrainRenderingEnabled( mTerrainRenderingEnabled );
+  res.setTerrainVerticalScale( mTerrainVerticalScale );
+  res.setTerrainGenerator( mTerrainGenerator.get() );
+  return res;
+}
+
 void Qgs3DMapSettings::readXml( const QDomElement &elem, const QgsReadWriteContext &context )
 {
   QgsProjectDirtyBlocker blocker( QgsProject::instance() );

diff --git a/src/3d/qgs3dmapsettings.h b/src/3d/qgs3dmapsettings.h
@@ -40,13 +40,17 @@ class QgsLightSource;
 class QgsAbstract3DRenderer;
 class QgsReadWriteContext;
 class QgsProject;
+class Qgs3DMapSettingsSnapshot;
 
 class QDomElement;
 
 /**
  * \ingroup 3d
  * \brief Definition of the world.
  *
+ * \warning Qgs3DMapSettings are a QObject subclass, and accordingly are not
+ * safe for access across different threads. See Qgs3DMapSettingsSnapshot instead
+ * for a safe snapshot of settings from Qgs3DMapSettings.
  */
 class _3D_EXPORT Qgs3DMapSettings : public QObject, public QgsTemporalRangeObject
 {
@@ -59,6 +63,13 @@ class _3D_EXPORT Qgs3DMapSettings : public QObject, public QgsTemporalRangeObjec
 
     Qgs3DMapSettings &operator=( Qgs3DMapSettings const & ) = delete;
 
+    /**
+     * Returns a snapshot of settings from this object in a thread-safe, cheap-to-copy structure.
+     *
+     * \since QGIS 3.40
+     */
+    Qgs3DMapSettingsSnapshot snapshot() const SIP_SKIP;
+
     //! Reads configuration from a DOM element previously written by writeXml()
     void readXml( const QDomElement &elem, const QgsReadWriteContext &context );
     //! Writes configuration to a DOM element, to be used later with readXml()

diff --git a/src/3d/qgs3dmapsettingssnapshot.cpp b/src/3d/qgs3dmapsettingssnapshot.cpp
@@ -0,0 +1,27 @@
+/***************************************************************************
+  qgs3dmapsettingssnapshot.cpp
+  --------------------------------------
+  Date                 : August 2024
+  Copyright            : (C) 2024 by Nyall Dawson
+  Email                : nyall dot dawson at gmail dot com
+ ***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+
+#include "qgs3dmapsettingssnapshot.h"
+#include "qgs3dutils.h"
+
+QgsVector3D Qgs3DMapSettingsSnapshot::mapToWorldCoordinates( const QgsVector3D &mapCoords ) const
+{
+  return Qgs3DUtils::mapToWorldCoordinates( mapCoords, mOrigin );
+}
+
+QgsVector3D Qgs3DMapSettingsSnapshot::worldToMapCoordinates( const QgsVector3D &worldCoords ) const
+{
+  return Qgs3DUtils::worldToMapCoordinates( worldCoords, mOrigin );
+}