search

qgis / QGIS

QGIS is a free, open source, cross platform (lin/win/mac) geographical information system (GIS)
https://qgis.org
GNU General Public License v2.0
10.59k stars 3k forks source link

Phong Shader Ambient Produces Desaturated Plots #57938

Closed tsjordan-eng closed 4 months ago

tsjordan-eng commented 4 months ago

What is the bug or the crash?

With the new Phong shader introduced in 3.36 my 3D views look desaturated, almost like "lightness" has been turned up. Before the ambient color increased "brightness".

I have tried adjusting "Strength" and a variety of colors, but I can't get the colors to remain saturated.

Steps to reproduce the issue

In < 3.34

  1. Add 3D symbol
  2. Adjust the Ambient color to brighten the scene

Value-Density

Open in > 3.36

  1. Note that the colors are the same, but now there are more settings like Strength
  2. Notice the 3D scene is pale and desaturated now

Value-Density_3 36

image

Versions

Broken Version

Ambient increase lightness and desaturates colors. Still broken on 3.38, too.

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd">

QGIS version | 3.36.0-Maidenhead | QGIS code revision | 09951dc0acf -- | -- | -- | -- Qt version | 5.15.3 Python version | 3.10.12 GDAL/OGR version | 3.4.1 PROJ version | 8.2.1 EPSG Registry database version | v10.041 (2021-12-03) GEOS version | 3.10.2-CAPI-1.16.0 SQLite version | 3.37.2 PDAL version | 2.3.0 PostgreSQL client version | 14.10 (Ubuntu 14.10-0ubuntu0.22.04.1) SpatiaLite version | 5.0.1 QWT version | 6.1.4 QScintilla2 version | 2.11.6 OS version | Pop!_OS 22.04 LTS   |   |   |   Active Python plugins qgiscloud | 3.9.10 Qgis2threejs | 2.7.3 qgis2web | 3.20.0 db_manager | 0.1.20 MetaSearch | 0.3.6 processing | 2.12.99 grassprovider | 2.12.99 QGIS version 3.36.0-Maidenhead QGIS code revision [09951dc0acf](https://github.com/qgis/QGIS/commit/09951dc0acf) Qt version 5.15.3 Python version 3.10.12 GDAL/OGR version 3.4.1 PROJ version 8.2.1 EPSG Registry database version v10.041 (2021-12-03) GEOS version 3.10.2-CAPI-1.16.0 SQLite version 3.37.2 PDAL version 2.3.0 PostgreSQL client version 14.10 (Ubuntu 14.10-0ubuntu0.22.04.1) SpatiaLite version 5.0.1 QWT version 6.1.4 QScintilla2 version 2.11.6 OS version Pop!_OS 22.04 LTS Active Python plugins qgiscloud 3.9.10 Qgis2threejs 2.7.3 qgis2web 3.20.0 db_manager 0.1.20 MetaSearch 0.3.6 processing 2.12.99 grassprovider 2.12.99 # Working Version *Ambient causes brightness, retaining saturation* QGIS version | 3.34.0-Prizren | QGIS code revision | ffbdd678812 -- | -- | -- | -- Qt version | 5.15.3 Python version | 3.10.12 GDAL/OGR version | 3.4.1 PROJ version | 8.2.1 EPSG Registry database version | v10.041 (2021-12-03) GEOS version | 3.10.2-CAPI-1.16.0 SQLite version | 3.37.2 PDAL version | 2.3.0 PostgreSQL client version | 14.9 (Ubuntu 14.9-0ubuntu0.22.04.1) SpatiaLite version | 5.0.1 QWT version | 6.1.4 QScintilla2 version | 2.11.6 OS version | Pop!_OS 22.04 LTS   |   |   |   Active Python plugins qgiscloud | 3.9.10 Qgis2threejs | 2.7.3 qgis2web | 3.20.0 db_manager | 0.1.20 MetaSearch | 0.3.6 processing | 2.12.99 grassprovider | 2.12.99 QGIS version 3.34.0-Prizren QGIS code revision [ffbdd678812](https://github.com/qgis/QGIS/commit/ffbdd678812) Qt version 5.15.3 Python version 3.10.12 GDAL/OGR version 3.4.1 PROJ version 8.2.1 EPSG Registry database version v10.041 (2021-12-03) GEOS version 3.10.2-CAPI-1.16.0 SQLite version 3.37.2 PDAL version 2.3.0 PostgreSQL client version 14.9 (Ubuntu 14.9-0ubuntu0.22.04.1) SpatiaLite version 5.0.1 QWT version 6.1.4 QScintilla2 version 2.11.6 OS version Pop!_OS 22.04 LTS Active Python plugins qgiscloud 3.9.10 Qgis2threejs 2.7.3 qgis2web 3.20.0 db_manager 0.1.20 MetaSearch 0.3.6 processing 2.12.99 grassprovider 2.12.99 ### Supported QGIS version - [x] I'm running a supported QGIS version according to [the roadmap](https://www.qgis.org/en/site/getinvolved/development/roadmap.html#release-schedule). ### New profile - [ ] I tried with a new [QGIS profile](https://docs.qgis.org/latest/en/docs/user_manual/introduction/qgis_configuration.html#working-with-user-profiles) ### Additional context _No response_
nyalldawson commented 4 months ago

Can you try setting the ambient colour to black?

tsjordan-eng commented 4 months ago

Yes, it gets very dark

tsjordan-eng commented 4 months ago

I found that b8b2a844762: "Fix handling of phong shader ambient color" introduces the problem I am seeing; it works on the previous commit.

It looks like this commit adds specular color, but loses the behavior specified in the comment: "Combine spec with ambient+diffuse for final fragment color". The diffuse.rbg is not multplied by ambdient.rgb anymore. It works if I make that change.

diff --git a/src/3d/shaders/phong.inc.frag b/src/3d/shaders/phong.inc.frag
index 40536b0dd73..65133551e40 100644
--- a/src/3d/shaders/phong.inc.frag
+++ b/src/3d/shaders/phong.inc.frag
@@ -15,8 +15,8 @@ vec4 phongFunction(const in vec4 ambient,
     adsModel(worldPosition, worldNormal, worldView, shin, diffuseColor, specularColor);

     // Combine spec with ambient+diffuse for final fragment color
-    vec3 color = ambient.rgb
-               + diffuseColor * diffuse.rgb
+    vec3 color = (ambient.rgb
+               + diffuseColor) * diffuse.rgb
                + specularColor * specular.rgb;

     return vec4(color, diffuse.a);

Should I submit a pull request for this?

nyalldawson commented 4 months ago

@tsjordan-eng no, it's correct as is. The ambient should always be added without any impact by the light sources

tsjordan-eng commented 4 months ago

Dang, I rather like the look I could get before that "fix" in b8b2a844762358f44ee99b78e71eb93965bd570b, haha. Ambient seemed to brighten the scene. Any tips on brightening the colors?

Should we close this issue?

nyalldawson commented 4 months ago

@tsjordan-eng can you try copying the data defined diffuse expression to ambient colour, and then wrapping it in the "darker" function to get a darker shade?

tsjordan-eng commented 4 months ago

Oh man, that's perfect. I had tried that, but the using the darker function is the nice key to tuning it.

Thanks, so much!

nyalldawson commented 4 months ago

@tsjordan-eng no problem, glad to hear you got the result you're after!

Can I close this now?

tsjordan-eng commented 4 months ago

Yes, thank you!