Skip to content

Examples: Updates for color management (pt3) #25889

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 20, 2023
Merged

Examples: Updates for color management (pt3) #25889

merged 11 commits into from
Apr 20, 2023

Conversation

@donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Apr 20, 2023

donmccurdy
donmccurdy commented Apr 20, 2023
View reviewed changes
examples/screenshots/webgl_loader_mmd.jpg
Copy link
Collaborator Author

@donmccurdy donmccurdy Apr 20, 2023

Choose a reason for hiding this comment

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

@takahirox does this result seem acceptable for the Hatsune Miku character?

Mugen87
Mugen87 reviewed Apr 20, 2023
View reviewed changes
examples/webgl_loader_kmz.html
scene.add( camera );

const grid = new THREE.GridHelper( 50, 50, 0xffffff, 0x333333 );
const grid = new THREE.GridHelper( 50, 50, 0xffffff, 0x7b7b7b );
Copy link
Collaborator

@Mugen87 Mugen87 Apr 20, 2023

Choose a reason for hiding this comment

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

The default color values of GridHelper are 0x444444 and 0x888888 right now. Would it make sense to update these in the ctor of GridHelper as well?

Copy link
Collaborator Author

@donmccurdy donmccurdy Apr 20, 2023

Choose a reason for hiding this comment

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

Good question! Replied in main thread, since it is an important question and I don't want to lose the discussion.

@Mugen87 Mugen87 added this to the r152 milestone Apr 20, 2023
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 20, 2023 •
edited
Loading

@Mugen87 to your question about changing the BoxHelper default colors —

  1. Originally three.js used input colors "as given", did some rendering, and wrote output to display. No color conversions were involved, so I'd argue that input colors were sRGB (ok), rendering happened in sRGB (incorrect), and output to canvas was sRGB (ok).

  2. With the introduction of renderer.outputEncoding = sRGBEncoding, we began assuming input colors were Linear-sRGB, not sRGB, and expecting users to convert them accordingly. Input was Linear-sRGB (ok, though confusing to webdevs), rendering happened in Linear-sRGB (ok), and then output to canvas was sRGB (ok).

  3. Now with ColorManagement.enabled = true, we are back to assuming hex and CSS inputs are sRGB, and the rest of the workflow is handled correctly.

I believe these BoxHelper colors were chosen during stage (1) above, and were not updated with (2). And (1) is still the default workflow in three.js, at least until r152 is released. So, probably the colors used in helpers do not need to be updated, they are still sRGB hex codes.

Because of the change of input color assumptions, I'm trying to update the hex values in examples that use renderer.outputEncoding = sRGBEncoding, while leaving hex values from case (1) alone as much as possible.

Anyway, sorry for such a long answer to a short question! It has been a challenge to work out these decisions. 😅 Does this sound OK?

/cc @WestLangley

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 20, 2023

Yes, sounds good!

@donmccurdy donmccurdy merged commit 66c460e into mrdoob:dev Apr 20, 2023
@donmccurdy donmccurdy deleted the feat/colormanagement-examples-pt3 branch April 20, 2023 17:46
@Mugen87 Mugen87 mentioned this pull request Apr 22, 2023
Closed
25 tasks
emmanueljl pushed a commit to emmanueljl/three.js that referenced this pull request Apr 28, 2023
@donmccurdy @Mugen87 @emmanueljl
Examples: Updates for color management (pt3) (mrdoob#25889)
34d2799 
* LWOLoader: Updates for color management
* MMDLoader: Updates for color management
* Examples: Update more loader examples for color management

---------

Co-authored-by: Michael Herzog <michael.herzog@human-interactive.org>
Mugen87
Mugen87 reviewed May 25, 2023
View reviewed changes
examples/jsm/loaders/MMDLoader.js
params.emissive = new Color().fromArray( material.ambient );
params.transparent = params.opacity !== 1.0;

if ( ColorManagement.enabled === true ) {
Copy link
Collaborator

@Mugen87 Mugen87 May 25, 2023

Choose a reason for hiding this comment

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

I'm currently reviewing the remaining explicit usage of ColorManagement in the example code.

Do we need this check here? This is the only loader which conditionally performs a color space conversion based on ColorManagement.enabled. Can't we just always assume color management and sRGB output is enabled?

Copy link
Collaborator Author

@donmccurdy donmccurdy May 25, 2023

Choose a reason for hiding this comment

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

I think the check would also be fine to remove, yes. VOXLoader does something similar:

three.js/examples/jsm/loaders/VOXLoader.js

Line 199 in 673eca6

_color.setRGB( r, g, b, SRGBColorSpace );

@donmccurdy donmccurdy mentioned this pull request Aug 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mugen87 Mugen87 Mugen87 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

r152

Development

Successfully merging this pull request may close these issues.

2 participants

@donmccurdy @Mugen87