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 PBR lighting #6430

Merged
merged 12 commits into from
Apr 27, 2018
Merged

Update PBR lighting #6430

merged 12 commits into from
Apr 27, 2018

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Apr 5, 2018

Couple of lighting changes to the glTF 2.0 PBR shader here:

  • Updated the math to convert baseColor and emissiveColor from sRGB to linear, run the PBR calculations in linear space, and then convert back to sRGB on the way out. This matches a change that was made to the Khronos reference shader after Cesium had taken a copy of it.
  • We had mistakenly used baseColor instead of diffuseColor in one part of the formula.
  • Updated the "Earth colors" gradients that Cesium uses in place of IBL.

Together these changes make glTF 2.0 models look a fair bit brighter than they had before. I'll post a before/after screenshot pair soon.

Fixes #6412. /cc @abwood

@cesium-concierge
Copy link

Signed CLA is on file.

@emackey, thanks for the pull request! Maintainers, we have a signed CLA from @emackey, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

⚠️ I noticed that a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version. Once you do, please confirm by commenting on this pull request.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@emackey
Copy link
Contributor Author

emackey commented Apr 5, 2018

master

gltf-pbr-before

branch

gltf-pbr-after

@emackey
Copy link
Contributor Author

emackey commented Apr 5, 2018

Another before/after:

gltf-pbr2-before gltf-pbr2-after

@emackey
Copy link
Contributor Author

emackey commented Apr 9, 2018

This isn't working as well as I would like for non-PBR models. I'm going to make a few more tweaks here.

@emackey
Copy link
Contributor Author

emackey commented Apr 9, 2018

milktruck-gltf1

Above is the glTF 1.0 Milk Truck. It supplies its own shaders, so looks the same on master as on this branch. The model doesn't know where the Sun is in Cesium, and is lit by a light on the camera. This means the lighting moves around as you examine the model from different sides.

milktruck-before

Above is the glTF 2.0 Milk Truck in master. glTF 2.0 models rely on the PBR shader even if the model came from an older, non-PBR source. The shader is aware of where Cesium's Sun is, it's shining on the roof of the truck but not the driver's side. Still, the model looks dim and doesn't appear to be in direct sunlight.

milktruck-after

Above, the glTF 2.0 truck rendered on this branch, without shadows. The roof is clearly in direct sunlight. The sky contributes a "cool" (blue-ish) IBL color to the driver's side of the truck, and the Sun is hitting the lit sides with a "warm" (yellow-ish) color.

milktruck-shadow

Same but with shadows enabled.

These latest changes didn't affect the PBR models noticeably, so the earlier screenshots are still valid.

This is ready.

@emackey
Copy link
Contributor Author

emackey commented Apr 16, 2018

More screenshots. Using the model from KhronosGroup/COLLADA2GLTF#172:

Cesium master:

collada172-before

This branch:

collada172-after

@lilleyse
Copy link
Contributor

Ah so much better.

Sorry for being quiet here, I should have a chance to review later in the week.

emackey and others added 3 commits April 18, 2018 17:27
…position up to the surface. Without this, geometry positioned close to the center of the Earth will turn white.
Minor fix to procedural reflection map calculation.
@lilleyse
Copy link
Contributor

Looks great - my only critique is that purely reflective areas look a bit washed out, most noticeably in the "space" view.

Comparing this branch to master:

lighter
darker

@emackey
Copy link
Contributor Author

emackey commented Apr 23, 2018

Yes, this washing-out is fairly deliberate, so that models don't blend into the black space background too easily. We'll revisit this for HDR Image-based lighting.

@mramato
Copy link
Contributor

mramato commented Apr 23, 2018

I'm no expert and I agree that master is too dark, but I think a lot of these models look washed out in this branch, particularly the truck and mask. If this will go away with HDR IBL, great!, but could we maybe have a middle ground until then?

Also, I imagine we would really want to do these types of comparisons on a properly calibrated monitor.

But again, just my 2 cents.

@cx20
Copy link

cx20 commented Apr 23, 2018

@emackey Can you try it with the following models if possible?
playcanvas/playcanvas-gltf#13 (comment)

Three.js + shoe.glb result:
image

Babylon.js + shoe.glb result:
image

Cesium.js v1.44 + shoe.glb result:
image

@emackey
Copy link
Contributor Author

emackey commented Apr 23, 2018

Sounds like the consensus is that I should take one more pass at this.

@cx20 You can try this branch in your gltf-test project by expanding the "All checks have passed... show all checks" section at the bottom of the PR, and click on "zip file... Details" to download the built version of it.

@cx20
Copy link

cx20 commented Apr 23, 2018

@emackey Opps, I did not notice there was a built module there. I tried it with the PR version.
It looks good. However, I feel that the outline of the shoe is whitening slightly due to rimlight, but is this the specification of Cesium.js?
image

Also, although it is another problem, it is unknown what this PR is affecting, but it seems that Monster.gltf is not displayed properly.
monster_cesiumjs_pbr_lighting_20180424

@lilleyse
Copy link
Contributor

I bet the monster issue is related to #6447.

@emackey
Copy link
Contributor Author

emackey commented Apr 23, 2018

I feel that the outline of the shoe is whitening slightly due to rimlight, but is this the specification of Cesium.js?

It could be your light source is on the horizon. Cesium keeps track of actual time of day, and calculates where the Sun would really be for a given world location at that date & time. It's possible the Sun was setting or is even below the horizon (we have a separate issue to turn off the Sunlight after dusk).

@cx20
Copy link

cx20 commented Apr 23, 2018

I understood that rimlight is the specification of Cesium.js. I would like to learn more about Cesium.js😄

@emackey
Copy link
Contributor Author

emackey commented Apr 24, 2018

Cesium does have a "rim light" shader, but it's not in use here. I think you might be seeing the fresnel effects of the BRDF. Everything has fresnel in PBR.

I suspect this branch makes the fresnel effect stronger. Conversions between sRGB and Linear color space were added, but the BRDF LUT (Look-up Table) is computed in a shader, not stored in an image, so a corresponding sRGB-to-linear conversion was not added there. Since the conversions were added everywhere else, and the lighting calculations all happen in linear space now, the effect is amplified from what it was before. I posted a question about this to KhronosGroup/glTF-Sample-Viewer#64.

@emackey
Copy link
Contributor Author

emackey commented Apr 27, 2018

I think I finally kicked this problem. @cx20 I'm curious your impressions of the latest version here.

pbr_shoe

@emackey
Copy link
Contributor Author

emackey commented Apr 27, 2018

pbr_flighthelmet

@cx20
Copy link

cx20 commented Apr 27, 2018

@emackey It's a wonderful job. I think that it is reproducibility comparable to photographs.

@emackey
Copy link
Contributor Author

emackey commented Apr 27, 2018

Wow, thanks @cx20.

@lilleyse This is ready. The gltf-pipeline one is updated too.

@lilleyse
Copy link
Contributor

I think it looks great. Does anyone else have thoughts before we merge?

@mramato
Copy link
Contributor

mramato commented Apr 27, 2018

👍 from me.

@lilleyse lilleyse merged commit 125c398 into master Apr 27, 2018
@lilleyse lilleyse deleted the pbr-lighting branch April 27, 2018 14:04
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.

6 participants