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

Fix base color and reflectivity calculations for PBR materials #12043

Merged
merged 5 commits into from
Jun 25, 2024
Merged

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Jun 20, 2024

Description

Our calculation of the base diffuse color for a material was scaled by the transmission coefficient (1.0 - f0) (where f0 is the specular reflection coefficient at normal incidence):

material.diffuse = material.diffuse * (1.0 - f0) * (1.0 - metalness);

This intuitively seems to make sense at normal incidence. The incident light that actually interacts with the microfacets of the surface (in the diffuse model) will be reduced by the amount of light already reflected specularly.

However, this simple 1D scaling is an oversimplification of something that is already accounted for in the BRDF, so including the scaling results in PBR materials being rendered slightly too dim.

This PR instead computes the base diffuse color using a formula more similar to the glTF Sample Viewer:

material.diffuse = mix(material.baseColor.rgb, vec3(0), metalness);

Note the new baseColor property on the czm_modelMaterial struct. This helps avoid confusion from overwriting the diffuse property too many times, and fixes #12041, where the KHR_materials_specular extension was using the diffuse property as if it were the raw base color.

Issue number and link

Resolves #12041

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • [ ] I have added or updated unit tests to ensure consistent code coverage
  • [ ] I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jjhembd!

✅ We can confirm we have a CLA on file for you.

@jjhembd jjhembd changed the title Fix f0 calculation for KHR_materials_specular extension Fix base color and reflectivity calculations for PBR materials Jun 24, 2024
@jjhembd jjhembd marked this pull request as ready for review June 24, 2024 19:29
@jjhembd jjhembd requested a review from ggetz June 24, 2024 19:29
CHANGES.md Outdated
@@ -14,6 +14,7 @@

##### Fixes :wrench:

- Fixed diffuse color calculation for PBR materials. Many models will now appear slightly brighter. This also fixes an [issue affecting the KHR_materials_specular extension](https://github.com/CesiumGS/cesium/issues/12041). [#12043](https://github.com/CesiumGS/cesium/pull/12043)
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though they were fixed in one PR, I would suggest listing this as two different items. The former (Fixed diffuse color calculation for PBR materials) can link to the PR. The later (fixes an issue affecting the KHR_materials_specular extension) can link to the issue.


material.specular = f0;

// diffuse only applies to dielectrics.
material.diffuse = material.diffuse * (1.0 - f0) * (1.0 - metalness);
material.diffuse = mix(material.baseColor.rgb, vec3(0), metalness);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be?

Suggested change
material.diffuse = mix(material.baseColor.rgb, vec3(0), metalness);
material.diffuse = mix(material.baseColor.rgb, vec3(0.0), metalness);

@jjhembd
Copy link
Contributor Author

jjhembd commented Jun 24, 2024

Thanks @ggetz! I addressed both of those comments.

@ggetz
Copy link
Contributor

ggetz commented Jun 25, 2024

All looks good! Thanks @jjhembd!

@ggetz ggetz merged commit 9d7b980 into main Jun 25, 2024
9 checks passed
@ggetz ggetz deleted the color-factor branch June 25, 2024 13:17
@jjhembd
Copy link
Contributor Author

jjhembd commented Jun 27, 2024

For the record: the extra (1 - f0) factor has appeared in other implementations as well. See KhronosGroup/glTF#1945 and KhronosGroup/glTF#2022 for discussion about how it was caught and corrected in the glTF spec implementation notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression for rendering between 1.117 and 1.118
2 participants