[3D] Use QGIS material for the terrain #58134

ptitjano · 2024-07-16T12:37:32Z

Description

This is factored out from #57899

It allows to use QGIS material for the terrain instead of native qt3D ones. It will make it possible to customize those shaders. There are 3 different cases to handle:

no texture: there is a drop-in replacement with QgsPhongMaterialSettings
texture, no shading: QgsPhongTexturedMaterialSettings is extended to handle both textures and textures from a path
texture, shading. A new material is introduced: QgsTextureMaterial

This PR needs #58114 to be merged first and thus it includes it.

github-actions · 2024-07-16T13:02:34Z

🪟 Windows builds ready!

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

(Built from commit b5b6c70)

github-actions · 2024-07-16T13:47:37Z

Tests failed for Qt 5

One or more tests failed using the build from commit 6494e5d

polygon3d_extrusion_textured_phong (testExtrudedPolygonsTexturedPhong)

polygon3d_extrusion_textured_phong

Test failed at testExtrudedPolygonsTexturedPhong at tests/src/3d/testqgs3drendering.cpp:532

Rendered image did not match tests/testdata/control_images/3d/expected_polygon3d_extrusion_textured_phong/expected_polygon3d_extrusion_textured_phong.png (found 40353 pixels different)

The full test report (included comparison of rendered vs expected images) can be found here.

Further documentation on the QGIS test infrastructure can be found in the Developer's Guide.

github-actions · 2024-07-16T13:47:38Z

Tests failed for Qt 6

One or more tests failed using the build from commit 6494e5d

polygon3d_extrusion_textured_phong (testExtrudedPolygonsTexturedPhong)

polygon3d_extrusion_textured_phong

Test failed at testExtrudedPolygonsTexturedPhong at tests/src/3d/testqgs3drendering.cpp:532

Rendered image did not match tests/testdata/control_images/3d/expected_polygon3d_extrusion_textured_phong/expected_polygon3d_extrusion_textured_phong.png (found 4813 pixels different)

The full test report (included comparison of rendered vs expected images) can be found here.

Further documentation on the QGIS test infrastructure can be found in the Developer's Guide.

python/3d/auto_generated/materials/qgsphongmaterialsettings.sip.in

python/3d/auto_generated/materials/qgsphongtexturedmaterialsettings.sip.in

src/3d/materials/qgstexturematerial.h

tests/src/3d/testqgs3drendering.cpp

There is no functional change. With this change, the terrain uses a material defined by QGIS instead of a default Qt3D. This will make it easier to customize the shader in the future.

`mShininess` is a double.

This is already waht QgsPhongTexturedMaterialSettings does. The reason for this change is to avoid the user-set values changing for them when saving/restoring projects.

src/3d/materials/qgsphongtexturedmaterialsettings.cpp

src/3d/materials/qgsphongtexturedmaterialsettings.h

src/3d/materials/qgstexturematerial.cpp

nyalldawson · 2024-07-22T23:01:04Z

src/3d/materials/qgsphongtexturedmaterialsettings.cpp

+  if ( texture )
+  {
+    mDiffuseTexturePath.clear();
+    mDiffuseTexture.reset( texture );


I still don't get how this can be stable. QTexture2D is a QObject subclass, so we'll have both thread safety issues (if a QgsPhongTexturedMaterialSettings is copied across threads) + the risk of a parent object for the QTexture2D instance causing object deletion outside of the shared_ptr .

@wonder-sk i'd love your insights here too, given that you're the resident expert in Qt3D internals 😆

+1 to what Nyall said... passing QTexture2D around looks quite scary (and just like it was pointed out - QObject-based class in shared_ptr is not safe).

Looking at the QgsPhongTexturedMaterialSettings::toMaterial() implementation, we end up extracting QImage from it, so why not have QgsPhongTexturedMaterialSettings::setDiffuseTexture( const QImage &diffuse ) instead? It would simplify things, not requiring two variants (either a path or a texture). Another argument against using QTexture2D is that PyQGIS users would not be able to use that (since we do not have PyQt3D linked).

Please double check also the functionality of terrain texture updates (e.g. when a map layer in project participating in the terrain's texture images gets enabled/disabled) - in such case, we do not recreate all the entities and materials again, instead we just update the image in QgsTerrainTextureImage. I believe this functionality is lost in the current PR, because the material now does not use the QgsTerrainTextureImage object (which gets new image), so this link seems to be lost. We may need some other mechanism to make sure that terrain's texture can be updated without a complete rebuild of the material...

+1 to what Nyall said... passing QTexture2D around looks quite scary (and just like it was pointed out - QObject-based class in shared_ptr is not safe).

This was never my intention to use a smart pointer, that was @nyalldawson's idea 😆 !

Looking at the QgsPhongTexturedMaterialSettings::toMaterial() implementation, we end up extracting QImage from it, so why not have QgsPhongTexturedMaterialSettings::setDiffuseTexture( const QImage &diffuse ) instead? It would simplify things, not requiring two variants (either a path or a texture). Another argument against using QTexture2D is that PyQGIS users would not be able to use that (since we do not have PyQt3D linked).

Indeed. And that's the case now.
Thanks for your comment. I came to the same conclusion. At first, I was trying to copy/paste Qt3D behavior but It creates a Material on the fly while QGIS stores settings and then creates a Material with toMaterial(). When I realized that the QImage from the stored QTexture2Dneeds to be copied to create a newQTexture2D, I thought that it would be much simpler to directly use a QImage`.

Please double check also the functionality of terrain texture updates (e.g. when a map layer in project participating in the terrain's texture images gets enabled/disabled) - in such case, we do not recreate all the entities and materials again, instead we just update the image in QgsTerrainTextureImage. I believe this functionality is lost in the current PR, because the material now does not use the QgsTerrainTextureImage object (which gets new image), so this link seems to be lost. We may need some other mechanism to make sure that terrain's texture can be updated without a complete rebuild of the material...

I don't understand. On current master, on terrain texture updates, QgsTerrainTileLoader::createTextureComponent gets called and thus the material is recreated. I don't see how only the image is updated. Based on my understanding, this is is not the case on current master.
Anyway, the terrain texture updates works on this PR.

Thanks for the updates!

I don't understand. On current master, on terrain texture updates, QgsTerrainTileLoader::createTextureComponent gets called and thus the material is recreated. I don't see how only the image is updated. Based on my understanding, this is is not the case on current master.
Anyway, the terrain texture updates works on this PR.

Please try this use case:

open a 3D view

enable or disable a map layer (one that does not have a 3D renderer so that the terrain texture gets updated) - I expect this still works because QgsTextureMaterial + createTexture() are used

now in 3D view settings, enable terrain shading

enable or disable a map layer - here I am expecting that the terrain texture will not get updated anymore

The mechanism how the terrain texture is updated works like this (verified on qgis master):

when the list of map layers for terrain texture gets modified, QgsTerrainEntity::invalidateMapImages() gets called

this will create tile update jobs (TerrainMapUpdateJob) from TerrainMapUpdateJobFactory

when an updated texture is ready, TerrainMapUpdateJob::onTileReady is called and it uses QgsTerrainTileEntity::textureImage() to get QgsTerrainTextureImage and update the image

As far as I can tell, when terrain shading is enabled, QgsTerrainTileLoader::createTextureComponent() now does not use createTexture() which is the key bit here - it wraps texture's QImage into QgsTerrainTextureImage (which allows updates of the image without full re-creation of terrain's tile entity).

For terrain tile load, probably you may want to have an approach similar to QgsTextureMaterial::setTexture()... the QgsPhongTexturedMaterialSettings object would be created without any texture image, and in a second step you would pass the texture image wrapped into QgsTerrainTextureImage...

QgsPhongTexturedMaterial allows to use a texture based on the path of an image. With this change, it is now possible to also directly use an image.

This allows to create a phong textured material settings from a phong settings and a texture.

There is no functional change. With this change, the terrain uses a material defined by QGIS instead of a default Qt3D. This will make it easier to customize the shader in the future.

This is the same material as the qt3d one: `Qt3DExtras::QTextureMaterial`. It will also be used in the following commit by the terrain.

There is no functional change. With this change, the terrain uses a material defined by QGIS instead of a default Qt3D. This will make it easier to customize the shader in the future.

wonder-sk · 2024-07-24T05:09:46Z

src/3d/materials/qgsphongtexturedmaterialsettings.cpp

-      const QImage textureSourceImage = QgsApplication::imageCache()->pathAsImage( mDiffuseTexturePath, QSize(), true, 1.0, fitsInCache );
-      ( void )fitsInCache;
+      Qt3DRender::QMaterial *material = nullptr;
+      QImage textureSourceImage = mDiffuseTextureImage.copy();


why are we doing explicit copy of the QImage here, and in QgsPhongTexturedMaterialSettings::setDiffuseTexture() ?

github-actions bot added this to the 3.40.0 milestone Jul 16, 2024

ptitjano self-assigned this Jul 16, 2024

ptitjano added the 3D Relates to QGIS' 3D engine or rendering label Jul 16, 2024

ptitjano force-pushed the terrain-qgis-shaders branch from 3ef1853 to b292e94 Compare July 16, 2024 15:03

nyalldawson reviewed Jul 18, 2024

View reviewed changes

python/3d/auto_generated/materials/qgsphongmaterialsettings.sip.in Outdated Show resolved Hide resolved

nyalldawson reviewed Jul 18, 2024

View reviewed changes

ptitjano force-pushed the terrain-qgis-shaders branch 3 times, most recently from 6494e5d to 9664e26 Compare July 19, 2024 13:19

ptitjano added 5 commits July 19, 2024 15:23

terrain: Use PhongMaterialSettings material for the non texture case

3037904

There is no functional change. With this change, the terrain uses a material defined by QGIS instead of a default Qt3D. This will make it easier to customize the shader in the future.

qgsphongmaterialsettings: Fix setShininess signature

5f3ee94

`mShininess` is a double.

qgsphongtexturedmaterialsettings: Use a double to store shininess

fd2bbf7

This is already waht QgsPhongTexturedMaterialSettings does. The reason for this change is to avoid the user-set values changing for them when saving/restoring projects.

qgsphongtexturedmaterialsettings: Use a double to store opacity

a60e0fe

This is already waht QgsPhongTexturedMaterialSettings does. The reason for this change is to avoid the user-set values changing for them when saving/restoring projects.

qgsphongtexturedmaterialsettings: Do proper opacity cast in toMaterial

c9eec06

ptitjano force-pushed the terrain-qgis-shaders branch 2 times, most recently from 14446d6 to f192260 Compare July 19, 2024 13:30

nyalldawson reviewed Jul 19, 2024

View reviewed changes

ptitjano force-pushed the terrain-qgis-shaders branch 3 times, most recently from 5f4552d to 4655e3a Compare July 20, 2024 09:53

nyalldawson reviewed Jul 22, 2024

View reviewed changes

ptitjano added 6 commits July 23, 2024 23:41

qgsphongtexturedmaterial: Allow to directly set a texture image

15bf984

QgsPhongTexturedMaterial allows to use a texture based on the path of an image. With this change, it is now possible to also directly use an image.

qgsphongtexturedmaterialsettings: Add constructor from QgsPhongMaterial

98b4220

This allows to create a phong textured material settings from a phong settings and a texture.

qgsterraintileloader: Fix typo

8875889

terrain: Use QgsPhongTexturedMaterialSettings for shading texture

a21778a

There is no functional change. With this change, the terrain uses a material defined by QGIS instead of a default Qt3D. This will make it easier to customize the shader in the future.

3d/material: Introduce QgsTextureMaterial

e05d637

This is the same material as the qt3d one: `Qt3DExtras::QTextureMaterial`. It will also be used in the following commit by the terrain.

terrain: Use QgsTextureMaterial for non shading texture

b5b6c70

There is no functional change. With this change, the terrain uses a material defined by QGIS instead of a default Qt3D. This will make it easier to customize the shader in the future.

ptitjano force-pushed the terrain-qgis-shaders branch from 4655e3a to b5b6c70 Compare July 23, 2024 21:57

wonder-sk reviewed Jul 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3D] Use QGIS material for the terrain #58134

[3D] Use QGIS material for the terrain #58134

ptitjano commented Jul 16, 2024

github-actions bot commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading

nyalldawson Jul 22, 2024

wonder-sk Jul 23, 2024

ptitjano Jul 23, 2024

wonder-sk Jul 24, 2024

wonder-sk Jul 24, 2024

[3D] Use QGIS material for the terrain #58134

Are you sure you want to change the base?

[3D] Use QGIS material for the terrain #58134

Conversation

ptitjano commented Jul 16, 2024

Description

github-actions bot commented Jul 16, 2024 • edited Loading

🪟 Windows builds ready!

github-actions bot commented Jul 16, 2024 • edited Loading

Tests failed for Qt 5

polygon3d_extrusion_textured_phong (testExtrudedPolygonsTexturedPhong)

polygon3d_extrusion_textured_phong

github-actions bot commented Jul 16, 2024 • edited Loading

Tests failed for Qt 6

polygon3d_extrusion_textured_phong (testExtrudedPolygonsTexturedPhong)

polygon3d_extrusion_textured_phong

nyalldawson Jul 22, 2024

Choose a reason for hiding this comment

wonder-sk Jul 23, 2024

Choose a reason for hiding this comment

ptitjano Jul 23, 2024

Choose a reason for hiding this comment

wonder-sk Jul 24, 2024

Choose a reason for hiding this comment

wonder-sk Jul 24, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading