Add some thread safety to Qgs3DMapSettings usage

[nyalldawson]

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]

@wonder-sk @ptitjano @benoitdm-oslandia as previously discussed.

[github-actions]

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 64a22e1)

[wonder-sk]

@nyalldawson few questions about this:

• what is your preference - shall we have the new "snapshot" class contain everything that may be required in 3D code responsible for creating entities just without QObject/signal stuff (kind of equivalent of QgsMapSettings for 2D rendering) or shall we have it as a subset of data (kind of equivalent of QgsRenderContext for 2D rendering) ? It looks like you were aiming for the latter, but I wanted to double check what the philosophy of the class would be (minimal vs maximal)

  • if we end up doing something equivalent to QgsRenderContext, then maybe it would be good to just extend the existing Qgs3DRenderContext class (which in fact contains Qgs3DMapSettings reference, and it has a lot of overlap with the new "snapshot" class)

• where would be the boundary between the "snapshot" class and the full Qgs3DMapSettings qobject class would be available? Right now, I see that e.g. point cloud entity uses just the snapshot class, but vector/rule-based entity uses the full Qgs3DMapSettings qobject. It would be good to draw the line somewhere - my suggestion is that 3D entity implementations could have access to Qgs3DMapSettings, to make use of its signals if needed, but anything deeper inside the code (e.g. chunk loaders, texture generators) could only have access to snapshots. If we agree on this, then probably it would be good if Qgs3DMapSettings would be a member variable of the parent Qgs3DMapSceneEntity class. What do you think?

[nyalldawson]

@wonder-sk

Thanks for the feedback!

what is your preference - shall we have the new "snapshot" class contain everything that may be required in 3D code responsible for creating entities just without QObject/signal stuff (kind of equivalent of QgsMapSettings for 2D rendering) or shall we have it as a subset of data (kind of equivalent of QgsRenderContext for 2D rendering) ? It looks like you were aiming for the latter, but I wanted to double check what the philosophy of the class would be (minimal vs maximal)

I lean toward keeping it minimal, and adding the properties only when we have a need to do so. (None of this is exposed to Python yet, so there's no disadvantage I can see in adding properties on demand when we need them in c++ code).

if we end up doing something equivalent to QgsRenderContext, then maybe it would be good to just extend the existing Qgs3DRenderContext class (which in fact contains Qgs3DMapSettings reference, and it has a lot of overlap with the new "snapshot" class)

Makes sense to me. I'll rework this and condense the new class into Qgs3DRenderContext, and remove the Qgs3DMapSettings property from Qgs3DRenderContext.

where would be the boundary between the "snapshot" class and the full Qgs3DMapSettings qobject class would be available? Right now, I see that e.g. point cloud entity uses just the snapshot class, but vector/rule-based entity uses the full Qgs3DMapSettings qobject. It would be good to draw the line somewhere - my suggestion is that 3D entity implementations could have access to Qgs3DMapSettings, to make use of its signals if needed, but anything deeper inside the code (e.g. chunk loaders, texture generators) could only have access to snapshots. If we agree on this, then probably it would be good if Qgs3DMapSettings would be a member variable of the parent Qgs3DMapSceneEntity class. What do you think?

I was originally going for a "use the snapshot wherever we can get away with it" approach, but I like the consistency of the Qgs3DMapSceneEntity change you describe better 👍

[benoitdm-oslandia]

@nyalldawson I was wondering if this new class must be read-only ie. without any setters. It disturbs me a bit a have an object acting as a clone of the main Qgs3DMapSettings but with updated properties. When and how do you update the main Qgs3DMapSettings object from the updated Qgs3DMapSettingsSnapshot?

[nyalldawson]

@wonder-sk ok, all done as discussed

@benoitdm-oslandia

I was wondering if this new class must be read-only ie. without any setters. It disturbs me a bit a have an object acting as a clone of the main Qgs3DMapSettings but with updated properties. When and how do you update the main Qgs3DMapSettings object from the updated Qgs3DMapSettingsSnapshot?

Indeed they were only used to initially set the properties from the map settings. I've removed all the setters now, and just set the members directly when first constructing Qgs3DRenderContext

[benoitdm-oslandia]

Thanks for the improvements!

[wonder-sk]

👏 well done - nice cleanup!

[nyalldawson]

Unrelated clang tidy failure