Refactor 3D map terrain handling (pt1) #59823
Conversation
nyalldawson
added
API
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
| @@ -174,12 +170,7 @@ void Qgs3DMapSettings::readXml( const QDomElement &elem, const QgsReadWriteConte | |||
|
|
|||
| QDomElement elemTerrain = elem.firstChildElement( QStringLiteral( "terrain" ) ); | |||
| mTerrainRenderingEnabled = elemTerrain.attribute( QStringLiteral( "terrain-rendering-enabled" ), QStringLiteral( "1" ) ).toInt(); | |||
There was a problem hiding this comment.
what do you think about getting rid of mTerrainRenderingEnabled, and no terrain would be equivalent to setTerrainSettings(nullptr) ? Or do you prefer to just deactivate terrain while keeping the settings, so that they are not lost when terrain is activated again?
There was a problem hiding this comment.
Or do you prefer to just deactivate terrain while keeping the settings, so that they are not lost when terrain is activated again
I'm ok with losing the settings. I don't think there's a really strong use case for switching on and off terrain, I think it's more likely a set-once-per-map thing.
There was a problem hiding this comment.
I prefer not to lose the settings. The cost is quite minimal (some potentially unneeded terrain tags in the xml?) and the GUI suggests you should not lose them (since it's a checkbox, and not a No Terrain entry in the combobox)
There was a problem hiding this comment.
What if we do make a "no terrain" entry in the combo and drop the checkbox? Are you ok with that?
There was a problem hiding this comment.
Yes, we would need to do this in that case.
Anyone wanting to toggle the terrain without losing settings will still be able to do it by keeping the settings page open.
There was a problem hiding this comment.
I'm going to come back to this one -- we'd need to add a lot of nullptr checks against terrainSettings() if we permit that to be null, so I'm going to have a bit of a think about a cleaner way to handle that.
nyalldawson
force-pushed
the
terrain2
branch
from
21032bc to
4b8f58d
Compare
nyalldawson
force-pushed
the
terrain2
branch
from
2329996 to
41c098e
Compare
This API only change refactors how terrain generators are handled, by splitting the settings of the generators away from the generators themselves. It's designed to follow the model used by eg QgsAbstractMaterialSettings.
This is a step toward more flexible terrain generation (including globe terrain!)