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

Update encoding to colorSpace and add transparent image support #267

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

danrossi
Copy link

Three have changed the encoding property to colorSpace from r153. There is a requirement to add a colorspace to video textures but I have a ticket about that in terms of performance issues using colorSpace and a WebGLRenderer work around using a custom shader.

example usage https://github.com/danrossi/three-troika/blob/main/test/index.html#L125

I've added transparent support for images.

Copy link
Collaborator

@lojjic lojjic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely 👍 on the transparent image change.

The colorSpace change requires a new minimum Threejs version, right? I'd prefer to keep compatibility by using either outputEncoding or colorSpace depending on the current environment.

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Jul 11, 2023

Something we did in pmndrs was check whether colorSpace existed and hardcoded enums:

if ('colorSpace' in texture) texture.colorSpace = 'srgb'
else texture.encoding = 3001 // sRGBEncoding
//
if ('colorSpace' in texture) texture.colorSpace = 'srgb-linear'
else texture.encoding = 3000 // LinearEncoding

Same with outputColorSpace and outputEncoding.

@danrossi
Copy link
Author

I'll try that then using the hardcoded flags. Here is a project where it updates and builds in a reduced three bundle with the webgpu renderer for future support. There is alot of wierd references to React so had to work around those with custom bundles also.

https://github.com/danrossi/three-troika

…e required. Textures can set their own colorSpavce.
@danrossi
Copy link
Author

example test with the change. https://danrossi.github.io/three-troika/test/

Comment on lines 56 to 63
renderer.outputEncoding = this.outputEncoding || LinearEncoding

//backwards compatibility support for output encoding and color space
if ('outputColorSpace' in renderer) {
renderer.outputColorSpace = this.outputColorSpace || 'srgb';
} else {
renderer.outputEncoding = this.outputEncoding || 3000
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to still be a breaking change, but on Troika's public API side.

IDK how you'd want to handle this, but you can emulate previous behavior with:

get outputEncoding() {
  return this.outputColorSpace === 'srgb' ? 3001 : 3000
}
set outputEncoding(encoding) {
  this.outputColorSpace = encoding === 3001 ? 'srgb' : 'srgb-linear'
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, just a note in the docs about which prop to use with which Three version is sufficient.

Thanks @CodyJasonBennett for the assist on this! :)

@danrossi
Copy link
Author

The documentation links to a non existent property. It's documentation for the React element I don't use. I've added it into that element. The new api link is this with a reference use with Three v153+.

https://threejs.org/docs/#api/en/renderers/WebGLRenderer.outputColorSpace

@danrossi
Copy link
Author

It's doing a fallback internally for outputEncoding. So can pass through output encoding if set.

https://github.com/mrdoob/three.js/blob/a05dee742ccbbba8d8f8386bca2fc537d3d45727/src/renderers/WebGLRenderer.js#L2433

@danrossi
Copy link
Author

Ok it can allow to fallback to outputEncoding if originally set. It will print logs. Or set new outputColorSpace property.

@danrossi
Copy link
Author

danrossi commented Jul 11, 2023

Excuse the spam sorry. With this change there is a chrome performance issue with that colorspace setting and video textures, it sets a specific texture srgb internal format with an issue. Hence the temporary work around. May not need to be documented when Chrome fix it.

Texture colorSpace overrides the renderer one.

https://github.com/danrossi/three-troika/blob/main/test/index.html#L163

@michaelthatsit
Copy link

Any updates on this? We've only just noticed that our text is in an entirely different color space.

@lojjic
Copy link
Collaborator

lojjic commented Nov 18, 2023

Any updates on this? We've only just noticed that our text is in an entirely different color space.

@michaelthatsit This PR is not about troika-three-text. If you're having issues with text colors, can you please open a new issue with detailed info and examples of what you're seeing? Thanks.

@danrossi
Copy link
Author

I'll double check if the font texture needs a colorSpace configuration with text, I think it does when loading image textures. However I've been personally in the middle of trying to replicate the text shader into a node system to work with WebGPU. I already upgraded my bmfont fork. It's not easy or going well yet. My tests for that are here to show how the node system works. https://github.com/danrossi/three-webgpu-renderer/blob/master/tests/webgpu_nodes.html

@danrossi
Copy link
Author

danrossi commented Nov 25, 2023

I've created an option on text to set a colorSpace on it's texture. However without colorSpace I am noticing square outlines on some characters. When setting colorSpace the colors are different for some characters. I'm not sure if my text config is right but for some reason the text is fuzzy and not a flat color. Not clear.

https://danrossi.github.io/three-troika/test/troika_text.html
https://danrossi.github.io/three-troika/test/troika_text_colorspace.html

@lojjic
Copy link
Collaborator

lojjic commented Nov 25, 2023

@danrossi We definitely don't want to be applying color adjustments on the SDF texture, because that texture is not used for color, it's used as data, so shifting its values changes the data.

I could see the need for color space control on the material's color, or a (seldom used) map texture, but those should be in full control of the user already. So there must be some detail I'm missing, and am hoping @michaelthatsit can provide that for me with an example. (In a new issue, please! Should be done independently of this PR.)

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.

4 participants