[renderer] Add color management option, replacing gammaOutput.

[donmccurdy]

For discussion and not ready to merge, see #3509.

[dmarcos]

It looks good to me. What's pending?

[donmccurdy]

Some questions in #3509 (comment). This code is working, I'm just not sure if this is the right API to give users a colorspace option. I would probably like to create a new example rather than editing the anime-UI one, too.

[ngokevin]

How about a material system because it only applies through the material component? e.g., material="colorSpace: srgb"

[dmarcos]

It should be camel case: colorSpace

[ngokevin]

as is, it's an html attribute

[dmarcos]

Yep, probably a property of material makes more sense.

[donmccurdy]

@dmarcos just to clarify — are you OK with this being a material system? I think something at the scene level is preferable to including colorspace on every material component.

[dmarcos]

@donmccurdy Yep, probably a system is better.

[donmccurdy]

This is ready for review. On the logo page, note that the result looks the same as before, with the exception of the (COLLADA) trees, which are washed out. That should be fixed with #3866, but this PR should be merged first.

[donmccurdy]

At some point I think three.js may deprecate the gammaOutput: true property, at which point maybe the material colorSpace property would do both things. But for now it feels safer to keep them separate, until we have more testing and feedback on this.

[donmccurdy]

I'm thinking of reversing the names on this, actually. The current names indicate what your input values are considered, assuming gammaOutput: true. But Unity handles this differently, per https://docs.unity3d.com/Manual/LinearRendering-LinearOrGammaWorkflow.html. Matching that sounds better to me, with:

• material="workflow: gamma;" (default, current)
• material="workflow: linear;" (new)

In the second case, all material.color, material.src, and emissive properties are automatically considered sRGB inputs, and decoded to linear for rendering, then encoded for display with gammaOutput: true. The linear workflow essentially requires gammaOutput: true, except that users might turn that off when using certain aframe-effects shaders.

[donmccurdy]

Ok, I did what I described in the previous post. Two workflows, gamma and linear. Where gamma is what we have today, and linear is preferred. The difference for scenes that just rely on glTF models and the material component should be minor:

gamma (current)	linear

The trees are brighter, because they're COLLADA models, but that will be addressed in the other PR. Otherwise things are close. Mainly, lighting changes are less pronounced.

Usage:

<a-scene renderer="workflow: linear | gamma;">
  <!-- ... -->
</a-scene>

The situation where users will see major differences is when they're adding objects and materials directly to the scene graph without using the material component. For example, the environment component needs to be updated to check the workflow. That requirement would go away if three.js had a built-in equivalent to this workflow setting, which may eventually happen.

I've made linear the default in this PR, but would be glad to change it back to gamma. I moved the setting into the renderer system, so that the gammaOutput option can just be removed for simplicity. This makes things pretty similar to Unity.

[ngokevin]

So the only concern is for previous scenes that had created their materials directly. But going forward, everything will default under linear and be consistent?

For example, the environment component needs to be updated to check the workflow.

Can you describe a tiny bit more what it would do differently? Would it read the workflow and adjust the material colors? Just to understand.

And is "workflow" a term used? Or does it describe "color space"?

[donmccurdy]

So the only concern is for previous scenes that had created their materials directly. But going forward, everything will default under linear and be consistent?

Right. I'm hoping this option will eventually shift up into three.js — then we can rely on that, and THREE.Color should be adjusted automatically so even custom components like environment won't need to worry about it. But getting that right at the three.js level may take a while..

Can you describe a tiny bit more what it would do differently? Would it read the workflow and adjust the material colors?

Right, so for example:

var workflow = sceneEl.systems.material.data.workflow;
if ( workflow === 'linear' ) {
  colorPalette.forEach((color) => color.convertGammaToLinear( 2.2 ));
}

The gist here is that using material="color: #4285f4" will give results that vary dramatically from CSS #4285f4 when using gammaOutput=true today, whereas this workflow=linear option corrects for that so they should match.

And is "workflow" a term used? Or does it describe "color space"?

I think either term could be used. C4D has a "Linear Workflow" checkbox. Unity calls it a workflow in their docs, but the checkbox says "Color Space: Linear". Maya requires you to set a bunch of colorspace and color profile settings, all of which together are documented as a "workflow".

The important thing to know is that this is the colorspace in which renderer calculations are done, and not the color space textures and colors are input as (which is pretty much always sRGB), or the color space output from the renderer. Because that's easy to misunderstand, I think that workflow is a bit clearer than colorSpace.

[donmccurdy]

I'll go through and do a bunch of before/after screenshots to compare here.

[ngokevin]

Thanks, should be good to merge, just slowly absorbing the implications.

What's behind the decision for linear vs gamma for default?

[donmccurdy]

Here's a comparison:

• workflow:gamma (demo)
• worklflow:linear (demo)

gamma	linear

In these examples, I see three effects:

• glTF models in scenes with gammaOutput disabled, which were previously shown too dark, now look correct.
• COLLADA models now look too bright. (see [collada-model] Deprecate 'collada-model'. #3817)
• Lighting is slightly more even. On the A-Frame Logo, note that the darker sides of the shapes are lighter in the linear workflow.

However, the difference when using other components — like environment will be much more noticeable. For that reason, I think it's probably better to start with this change off by default after all. Once we've gotten more testing maybe it can become opt-out, instead.

[ngokevin]

Thanks for the rundown! So I think maybe it's ok to choose whatever is the more sensible default (linear?). Apps upgrading to 0.9.0 have to option to specify gamma. We can update the environment component for 0.9.0 and document it well in changelog.

[donmccurdy]

I think I'd rather stick a toe in the water first on this one, with an opt-in for 0.9. 😅

Linear is a better choice in the long-run, but it will affect existing components badly if they instantiate THREE.Color or THREE.Texture themselves. I'm hoping that we can test this out and get feedback to influence what three.js does in the future. If the same changes I'm making here could be done in three.js instead — treating colors as sRGB by default — there would be no need for components to add convertGammaToLinear(2.2) throughout their code, and we'd have a more seamless API here.

[ngokevin]

OK. Last thing then I guess is document the new properties/defaults in this table also: https://github.com/aframevr/aframe/pull/3757/files#diff-601edc1958f8dda05aaef2c8adf3cf0cL24

Then looks good!

[ngokevin]

Do we want to uncomment this now?

[donmccurdy]

Done. ✅

[donmccurdy]

Ok, refactored this a bit. Also added some helpers so e.g. obj-model and (temporarily) collada-model will adjust to the linear workflow.

[donmccurdy]

Getting a single error in Chrome, doesn't seem obviously related:

[ngokevin]

Looking again, looks scary it will affect existing components badly if they instantiate THREE.Color or THREE.Texture. I think that is an extremely frequent operation, would they need/want to call the color correction util each time? Or only need to worry if they are third party components? What would a three.js change look like that does all automatically? Do we override THREE.Color?

[dmarcos]

Term workflow seems generic word. Does colorManagment make more sense?

[dmarcos]

Or maybe colorSpace?

[ngokevin]

To note: prior explanation:

And is "workflow" a term used? Or does it describe "color space"?

I think either term could be used. C4D has a "Linear Workflow" checkbox. Unity calls it a workflow in their docs, but the checkbox says "Color Space: Linear". Maya requires you to set a bunch of colorspace and color profile settings, all of which together are documented as a "workflow".

The important thing to know is that this is the colorspace in which renderer calculations are done, and not the color space textures and colors are input as (which is pretty much always sRGB), or the color space output from the renderer. Because that's easy to misunderstand, I think that workflow is a bit clearer than colorSpace.

[donmccurdy]

^Per above I think "colorSpace" is too confusing. But colorManagement: true might be easier to understand, I do like that...

[donmccurdy]

Do you think is sensible to monkey patch THREE COLOR / TEXTURE or introduce temporary changes...

Maybe. I'm not sure how large the change will be in threejs. I can take a look at that but also don't want to hold up 0.9.0... will try to get a rough estimation of the work involved tomorrow.

[dmarcos]

@donmcurdy This is almost
readt. Should we change the property name to colorManagment?

[dmarcos]

Maybe simpler:

  if (this.data.workflow !== 'linear' || !colorOrTexture ||
      (!colorOrTexture.isColor && !colorOrTexture.isTexture) { return; }
   if (colorOrTexture.isColor) {
      colorOrTexture.convertSRGBToLinear();
    } else {
      colorOrTexture.encoding = THREE.sRGBEncoding;
   }

[donmccurdy]

tbh this seems harder to read to me 😇

[dmarcos]

glTF assumes sRGB (gamma) per standard. More and more experiences include glTF models. It makes sense to default to gamma instead of linear?

[donmccurdy]

glTF assumes sRGB (gamma) per standard. More and more experiences include glTF models. It makes sense to default to gamma instead of linear?

Note that the workflows are named for the space in which the renderer works, and 'linear' is actually what we want even for sRGB or gamma inputs. That's definitely confusing, so I think renaming to colorManagement sounds like a good idea.

As for enabling this change by default, I'd prefer to wait on that until we have a way to automatically upgrade third-party components creating THREE.Color and THREE.Texture instances themselves. Otherwise we'll have two non-trivial breaking changes, instead of one minor one. I think it's fairly likely that can be handled within threejs itself, but I haven't reached a good solution there yet. 😕

Are we OK with just adding the colorManagement option for now, off by default?

[dmarcos]

I'm good with colorManagement and adjust later if needed.

[donmccurdy]

Ok, sounds good. I've replaced workflow: linear with colorManagement: true.