Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-120241

Incorrect handling of ambient color in phong shaders in qt3d (Regression)

    XMLWordPrintable

Details

    • Bug
    • Resolution: Fixed
    • P2: Important
    • 6.7.0 Beta2, 6.8.0 FF
    • 6.7.0
    • Qt3D
    • None
    • All
    • 491e25e10 (dev), 9cc525731 (6.7)

    Description

      In https://code.qt.io/cgit/qt/qt3d.git/commit/?id=fb0c9421b1e172833f3ac2166b9a39bd83a8b6ef the phong shader was refactored to move the phong shading function into a common function reused across different shaders.

      The incorrect definition of the shader was included however. Originally, the definition in phong.frag / phongalpha.frag / etc was

      fragColor = vec4( ka + kd * diffuseColor + ks * specularColor, alpha );

      note ambient "ka" color contribution is independant of diffuseColor

      After the refactoring, the color is calculated using:

      color = (ambient.rgb + diffuseColor) * diffuse.rgb + specularColor * specular.rgb

      now ambient color contribution is scaled by diffuse. This is incorrect, and the original definition was the correct one to use.

      I believe this may have been introduced because SOME of the original shaders used

      fragColor = vec4( diffuseTextureColor * ( ka + diffuseColor ) + ks * specularColor, 1.0 );

      eg diffusemap.frag. I'm not sure if that was originally intentional, or an existing bug which predated fb0c9421b1e172833f3ac2166b9a39bd83a8b6ef...

      Attachments

        Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            People

              seanharmer Sean Harmer
              ndawson Nyall Dawson
              Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Gerrit Reviews

                  There are no open Gerrit changes