Skip to content

Conversation

mrxz
Copy link
Contributor

@mrxz mrxz commented Jan 10, 2023

This PR introduces .applyColorCorrection for ground textures and dressing material color to avoid washed out colors when enabling colorManagement. See #83 for more details and caveats.

Resolves #83

@dmarcos
Copy link
Member

dmarcos commented Apr 22, 2023

Super thanks!

@vincentfretin
Copy link
Contributor

From my understanding, those changes are needed for aframe 1.4.2 (r147) with colorManagement:true (and I have this combination in my project), but not needed for aframe master (r152), right?
It may be good to still merge this and include it in a new release.

@mrxz
Copy link
Contributor Author

mrxz commented Jun 11, 2023

From my understanding, those changes are needed for aframe 1.4.2 (r147) with colorManagement:true (and I have this combination in my project), but not needed for aframe master (r152), right?

In aframe master it's no longer needed to call rendererSystem.applyColorCorrection on colours, but it's still very much needed for textures, as there is no way for Three.js to infer which colour space they are in. This means that this PR is still needed for the groundTexture and gridTexture. Nevertheless it's probably best to merge this PR with the applyColorCorrection on colours as well, since that way it will work on both 1.4.2 and master. On master those will effectively become no-ops.

@mrxz mrxz force-pushed the color-management branch from f1b14be to 858b17f Compare June 11, 2023 10:05
@mrxz
Copy link
Contributor Author

mrxz commented Jun 11, 2023

Seems that on master some more changes will be needed, investigating it

@mrxz
Copy link
Contributor Author

mrxz commented Jun 11, 2023

Added a commit that fixes it for aframe-master. With the newer Three.js all Color instances were automatically converted, but the code does use instances of THREE.Color for drawing on a canvas for the ground and grid textures. I've made sure that these are now correctly performed in "NoColorSpace" and only converted to the working color space when needed.

It's done in such a way that it works for both 1.4.2 and master. With colorManagment: false the results should still be identical, and with colorManagement: true they are very close (obviously small differences due to lighting taking places in linearSRGBColorSpace in the shader). Biggest difference is still as noted in #83 when an emissiveMap is used. Sadly there is nothing that can be done about it without changing the shader.

@mrxz mrxz force-pushed the color-management branch from 92e0c11 to b9ddcd7 Compare June 13, 2023 08:13
@dmarcos
Copy link
Member

dmarcos commented Jun 14, 2023

Thank you very much!

@dmarcos dmarcos merged commit 13829ee into supermedium:master Jun 14, 2023
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

Successfully merging this pull request may close these issues.

Supporting colorManagement=true
3 participants