Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving correctness of gamma application: It shouldn't be applied to material color parameters #5838

Closed
bhouston opened this issue Dec 31, 2014 · 12 comments

Comments

@bhouston
Copy link
Contributor

Right now when gammInput is turned on, gamma is applied to all input colors to the shader -- this is usually not how it is done and is generally not correct.

In particular, I am referring to these applications of input gamma: diffuse color, emissive color, specular as well as vertex colors. Also the ambient light color, the directional light color, the point light color, the spot light color, the hemisphere light colors.

As @WestLangley likely knows, use of gamma is to compensate for the sRGB standard used in a number of files formats, particularly JPG, PNG, GIF, etc. These images are pre-gamma corrected (by approximately pow( RGB, 1/2.2) so that when displayed on a standard monitor (which has a built in gamma of pow(RGB, 2.2) they look correct.

As we want to do rendering in linear space, we want bring the sRGB images into linear space, so we gamma correct them to remove their built-in gamma.

But all shader parameters are not pre-gamma corrected by pow( RGB, 1/ 2.2), they are linear, true values. Thus to apply an input gamma correction, e.g. pow( RGB, 2.2 ), to them in incorrect, we are making our linear colors darker than they should be in the mid range, around 0.5. This gets progressively worse as you get HDR colors, such as light colors above (1,1,1) (or Light.intensity), as applying the gamma input "correction" will make them quadratically brighter than they should be.

So I am proposing removing gamma correction from all inputs that are not texture inputs. This will simplify the code, make it smaller and make it more correct.

@WestLangley
Copy link
Collaborator

(This post has been edited for clarity based on comments below.)

I am glad we are having this discussion!

For reference, see the related discussion in #1488.

I made some notes on this some time ago, which I am posting here.

The following color properties should be assumed to be in linear space:

  • material.color
  • material.ambient ( this is being removed)
  • material.emissive
  • material.specular
  • vertex colors
  • ambientLight.color
  • light.color (and light.intensity ?) for directionalLight, pointLight, spotLight, hemiLight, hemiLight-ground

The following maps should be assumed to be in sRGB space, but have an override option:

  • material.map
  • material.envMap (except HDR maps -- they need to be decoded, but are in linear space)
  • material.specularMap (future, if/when is it changed to a 3-channel map)
  • material.emissiveMap (future)
  • material.iblMap (IBL map, future)

Everything else should be assumed to be in linear space:

  • alpha channels of color maps
  • material.lightMap
  • material.specularMap (for now, only as long as it is a grayscale)
  • material.alphaMap
  • material.bumpMap
  • material.normalMap
  • material.aoMap (future)
  • material.glossMap (future)
  • material.displacementMap
  • HDR environment maps

This gets really incorrect as you get HDR colors

My understanding is that HDR images are in linear space. ref: http://www.hdrsoft.com/resources/dri.html#gamma

Although we should still apply an output gamma to everything.

Only at the very end. In fact, renderer.gammaOutput should be false, and a post-processing step should perform the gamma-correction.

@bhouston
Copy link
Contributor Author

If everyone is okay with these non-texture parameters being in sRGB space, then I guess it is okay... but generally this isn't how things are normally done. I believe this assumption is currently unique to THREE.js.

Most systems are generally linear everywhere except when dealing with the sRGB encoded textures and optionally sRGB encoded final output.

The way that high end renderers handle this situation, such as V-Ray, is that they analyze the source of texture map, and if it is in a format is sRGB encoded, they correct it, otherwise they don't. Thus sRGB correction, occurs on a per-texture map basis, based on whether it is truly needed or not.

Most all HDR images are linear encoded. I believe all floating point texture formats are linear. Some mid-range formats like TIFF 16 bit may or may not be sRGB.

I'd prefer to assume everything is linear except for a subset of textures that truly are sRGB encoded. To handle the sRGB texture encoding issue, we could move the gammaFactor to the Texture, which would isolate the decode there where appropriate.

My suggestion would be to do this in two parts. The first would be to remove gamma corrections from the non-texture inputs in THREE.js to be more similar to other systems -- this is especially important for bright colors (> 1.0.) The second part, which can come later, would be to move gammaInput to be texture specific, rather than a global setting with arbitrary texture application.

The global setting for gammaInput sort of works right now, and is lower priority for me, because nearly all native image formats supported by the browser are usually sRGB encoded. Thus even though it isn't completely correct, it is right enough for most cases.

@WestLangley
Copy link
Collaborator

I'd prefer to assume everything is linear except for a subset of textures

I, too, would have no problem assuming everything is linear, except for the sRGB-encoded maps. Those who are most familiar with conventional pipelines can debate what is best.

I believe, then, that the gammaInput flag can be removed from the renderer. And, as I said previously, gammaOutput will have to be handled differently -- as a postprocessing step, somehow.

Also, in hindsight, I'd rename the related methods to sRGBtoLinear() and linearTosRGB(), and use the correct formula. ( @bhouston is going to kill me : - ) )

@bhouston
Copy link
Contributor Author

I, too, would have no problem assuming everything is linear, except for the sRGB-encoded maps. Those who are most familiar with conventional pipelines can debate what is best.

How about after these two PRs are accepted: #5785 #5834, I make another that removes the gamma correction of the various non-texture shader parameters? That would be the first step.

Also, in hindsight, I'd rename the related methods to sRGBtoLinear() and linearTosRGB(), and use the correct formula. ( @bhouston is going to kill me : - ) )

I'm okay with that, as long as it is a separate PR. This just ensures that the current PRs that clean up the code and are exactly compatible with the current workflow, are accepted. Then we can further polish.

If we can get a-per texture sRGB flag, then getting sRGBToLinear and linearTosRGB() functions along with it, makes a lot of sense.

@WestLangley
Copy link
Collaborator

This sounds good to me.

@bhouston
Copy link
Contributor Author

bhouston commented Jan 1, 2015

@mrdoob
Copy link
Owner

mrdoob commented Jan 1, 2015

This all sounds great to me.
Thanks a lot for the explanations by the way, I never had a good understanding of all this O:)

bhouston added a commit to bhouston/three.js that referenced this issue Jan 1, 2015
@Wandalen
Copy link
Contributor

Hi guys. Currently gamma factor used by:

  • Materials
  • Lights
  • Renderer

I would say it should be in Material exclusively. That's my suggestion.

@Wandalen
Copy link
Contributor

Most challenging part to move it out of lights.
To achieve it, it is possible to make several color uniform a light type.
For example for spot light:

  'spotLightColor' : new Uniform({ type: 'fv', value: [] }),
  'spotLightColorGamma' : new Uniform({ type: 'fv', value: [] }),

And in shaders we will get:

    '#ifdef GAMMA_INPUT',
    '  uniform vec3 ambientLightColorGamma;',
    '  #define ambientLightColor ambientLightColorGamma';
    '#else'
    '  uniform vec3 ambientLightColor;',
    '#endif GAMMA_INPUT',

    '#if MAX_DIR_LIGHTS > 0',

    '  #ifdef GAMMA_INPUT',
    '    uniform vec3 directionalLightColorGamma[ MAX_DIR_LIGHTS ];',
    '    #define directionalLightColor directionalLightColorGamma';
    '  #else'
    '    uniform vec3 directionalLightColor[ MAX_DIR_LIGHTS ];',
    '  #endif GAMMA_INPUT',

    '  uniform vec3 directionalLightDirection[ MAX_DIR_LIGHTS ];',

    '#endif',

    '#if MAX_HEMI_LIGHTS > 0',

    '  #ifdef GAMMA_INPUT',
    '    uniform vec3 hemisphereLightSkyColorGamma[ MAX_DIR_LIGHTS ];',
    '    #define hemisphereLightSkyColor hemisphereLightSkyColorGamma';
    '  #else'
    '    uniform vec3 hemisphereLightSkyColor[ MAX_DIR_LIGHTS ];',
    '  #endif GAMMA_INPUT',

    '  #ifdef GAMMA_INPUT',
    '    uniform vec3 hemisphereLightGroundColorGamma[ MAX_DIR_LIGHTS ];',
    '    #define hemisphereLightGroundColor hemisphereLightGroundColorGamma';
    '  #else'
    '    uniform vec3 hemisphereLightGroundColor[ MAX_DIR_LIGHTS ];',
    '  #endif GAMMA_INPUT',

    '  uniform vec3 hemisphereLightDirection[ MAX_HEMI_LIGHTS ];',

    '#endif',

    '#if MAX_POINT_LIGHTS > 0',

    '  #ifdef GAMMA_INPUT',
    '    uniform vec3 pointLightColorGamma[ MAX_DIR_LIGHTS ];',
    '    #define pointLightColor pointLightColorGamma';
    '  #else'
    '    uniform vec3 pointLightColor[ MAX_DIR_LIGHTS ];',
    '  #endif GAMMA_INPUT',

    '  uniform vec3 pointLightPosition[ MAX_POINT_LIGHTS ];',
    '  uniform float pointLightDistance[ MAX_POINT_LIGHTS ];',

    '#endif',

    '#if MAX_SPOT_LIGHTS > 0',

    '  #ifdef GAMMA_INPUT',
    '    uniform vec3 spotLightColorGamma[ MAX_DIR_LIGHTS ];',
    '    #define spotLightColor spotLightColorGamma';
    '  #else'
    '    uniform vec3 spotLightColor[ MAX_DIR_LIGHTS ];',
    '  #endif GAMMA_INPUT',

    '  uniform vec3 spotLightPosition[ MAX_SPOT_LIGHTS ];',
    '  uniform vec3 spotLightDirection[ MAX_SPOT_LIGHTS ];',
    '  uniform float spotLightAngleCos[ MAX_SPOT_LIGHTS ];',
    '  uniform float spotLightExponent[ MAX_SPOT_LIGHTS ];',

    '  uniform float spotLightDistance[ MAX_SPOT_LIGHTS ];',

    '#endif',

@Wandalen
Copy link
Contributor

It's easier to make this parameter material-specific. Later it could be done texture-specific. But texture is not only thing which depends of Gamma.

@vorg
Copy link

vorg commented Sep 18, 2015

How is that supposed to work in practice?

If I don't correct input but correct the output then the color with be brighter, no?

Let's say I have material

var m = new THREE.MeshLambertMaterial( { color: 0xe74c3c, shading: THREE.FlatShading } ) ;

I'm trying to use peach #74c3c color:

screen shot 2015-09-18 at 02 05 30

screen shot 2015-09-18 at 02 00 49

  • On the left is Unity 5 with linear space lighting
  • In the middle default ThreeJS setup (gammaInput and gammaOutput set to false) with valid color but invalid light falloff (gradient)
  • On the rightThreeJS setup (gammaInput and gammaOutput set to true) sharp light falloff due to gamma correction but invalid (too bright) color

@bhouston
Copy link
Contributor Author

bhouston commented Mar 3, 2016

Solved via: PR #8117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants