Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 8, 2023

Related issue: -

Description

This PR enables color management in webgl_materials_lightmap. Next to the changes in the example code, it was necessary to update the JSON asset.

@donmccurdy I have noticed a small issue with MeshPhongMaterial.specular and its default 0x111111. This value was supposed to be in linear-srgb previously. But after enabling color management I think this value should be updated to 0x494949 to ensure visual consistency (the decimal version of this hex is now used in the JSON, btw).

@Mugen87 Mugen87 added this to the r153 milestone May 8, 2023
@Mugen87 Mugen87 marked this pull request as ready for review May 8, 2023 08:44
@donmccurdy
Copy link
Collaborator

This value was supposed to be in linear-srgb previously.

Related to #25872. What it was supposed to be depended on the output color space. Similar to the colors in helpers like BoxHelper, which we intentionally did not update. I think we are aiming for consistency with the assumption that colors were chosen before sRGB renderer output was used.

That said, yes, Phong's specular property seems to be affected by these changes in a way that changes visual results more than other material color properties. I am unsure what to do with its default.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 8, 2023

Since sRGB output is the default now I think material color values other than 0xffffff and 0x000000 should be updated. For materials classes itself but also for classes creating materials like helpers etc..

Assuming sRGB output was used before (which was a project recommendation afaik), there is now a visual difference with color management enabled and it should be fixed, imo.

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 8, 2023

When we chose 0x444444 for GridHelper, it was a hex code representing a color in sRGB color space. In r152, it still is supposed to be a hex code representing a color in sRGB color space. We never updated the color when sRGB output become available, even though that changed the assumed color space — are we sure we want to update it now?

Aside: Primaries and fully-saturated blends of primaries (0xFFFFFF, 0xFFFF00, 0xFF0000) will not change. Only colors where RGB components are between 0 and 1, exclusive.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 8, 2023

@mrdoob What kind of previous setup would you consider as a baseline? I'm asking since as @donmccurdy pointed out, things evolved over the years and color space workflows can be different from app to app.

I think we should pick something that we can use as a reference and I vote for setups that previously configured an sRGB output color space. Meaning renderer.outputEncoding = THREE.sRGBEncoding.

With color management enabled, color defaults should produce the same visual result like this setup. And that means the previously 0x111111 specular color value was supposed to be in linear-sRGB (assuming sRGB workflow) and should now be updated to its sRGB pendant like we did in the examples.

@mrdoob
Copy link
Owner

mrdoob commented May 9, 2023

It may not be a surprise to hear that numbers like 0x444444 were chosen mostly because they're easy to write and not because it's a specific shade of gray.

I personally didn't really notice when we changed color space mostly because the lines tend to be thin and hard to see the change.

So I wouldn't worry too much about updating them, the ease of write/read is more important in that case imho.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 9, 2023

Okay, and what do you think about the specular value of MeshPhongMaterial? Is that something that should be updated? If not, all phong materials using the default will render slightly different with color management enabled.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 14, 2023

Merging the PR but I won't change any other examples with an updated phong specular value. At this point, I think the conclusion is to not update the (default) colors of helpers and other material properties.

@Mugen87 Mugen87 merged commit 8981ff5 into mrdoob:dev May 14, 2023
@donmccurdy
Copy link
Collaborator

I would prefer not to update default colors of helpers, yes. I'm wondering if there is something a bit different going on with MeshPhongMaterial's default specular color, and will try to look more closely into that...

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