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

Examples: Update FBXLoader examples to .outputEncoding = sRGBEncoding #25441

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

donmccurdy
Copy link
Collaborator

Related issue:

Added THREE.ColorManagement.legacyMode = false only where helpful to preserve the original look. In other cases I think the changes to the image are correct.

@epreston
Copy link
Contributor

epreston commented Feb 6, 2023

I've been wondering about that. Will both Color and ColorManagement continue to export SRGBToLinear ? Is it more future proof to import from ColorManagement ?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 6, 2023

Don't we need some changes in FBXLoader first before doing this, see #23283 (comment)?

@donmccurdy
Copy link
Collaborator Author

Will both Color and ColorManagement continue to export SRGBToLinear ? Is it more future proof to import from ColorManagement ?

Although ColorManagement.js exports SRGBToLinear and LinearToSRGB, we don't export those functions from the three.js top-level module. Userland code will have to access them through other methods, like the THREE.Color API. It's also an option to do this...

THREE.ColorManagement.convert( color, 'srgb', 'srgb-linear' );

... but the THREE.Color API is more stable at this point; there is no reason to switch. I would probably consider convert(...) to be an internal API at least for now.


Don't we need some changes in FBXLoader first before doing this, see #23283 (comment)?

I'd missed that, thanks! Currently FBXLoader color manages textures, but nothing else. So we're in a limbo where it's only half correct. I don't think that updating the examples to use .outputEncoding = sRGBEncoding makes things any worse, but we should definitely finish the changes to FBXLoader as well.

@donmccurdy
Copy link
Collaborator Author

Let's go ahead and do the changes described by #23283 (comment) too. I noticed that Blender has a choice when exporting FBX of whether the vertex colors are linear or sRGB. The default is sRGB, and I don't know that we can detect which was chosen, so I've assumed they're sRGB here.

@looeee as FYI, meant to preserve the look and appearance of FBX models under outputEncoding=sRGBEncoding.

Update screenshots

FBXLoader: Add sRGB -> Linear-sRGB conversion for material colors.

FBXLoader: Add sRGB -> Linear-sRGB conversion for vertex colors.

revert bvh screenshot
@donmccurdy donmccurdy force-pushed the examples-fbx-outputencoding branch from 6c5b772 to d446d1b Compare March 7, 2023 03:08
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 7, 2023

Since the color maps already set .encoding = sRGBEncoding;, this PR is complete now. Merging.

@Mugen87 Mugen87 merged commit d879635 into mrdoob:dev Mar 7, 2023
@Mugen87 Mugen87 added this to the r151 milestone Mar 7, 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.

3 participants