-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Update PBR lighting #6430
Conversation
Also, found a place where "baseColor" was used in place of diffuse.
@emackey, thanks for the pull request! Maintainers, we have a signed CLA from @emackey, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
This isn't working as well as I would like for non-PBR models. I'm going to make a few more tweaks here. |
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. 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. 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. 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. |
More screenshots. Using the model from KhronosGroup/COLLADA2GLTF#172: Cesium master: This branch: |
Ah so much better. Sorry for being quiet here, I should have a chance to review later in the week. |
…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.
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. |
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. |
@emackey Can you try it with the following models if possible? |
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. |
@emackey Opps, I did not notice there was a built module there. I tried it with the PR version. Also, although it is another problem, it is unknown what this PR is affecting, but it seems that Monster.gltf is not displayed properly. |
I bet the monster issue is related to #6447. |
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). |
I understood that rimlight is the specification of Cesium.js. I would like to learn more about Cesium.js😄 |
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. |
I think I finally kicked this problem. @cx20 I'm curious your impressions of the latest version here. |
@emackey It's a wonderful job. I think that it is reproducibility comparable to photographs. |
I think it looks great. Does anyone else have thoughts before we merge? |
👍 from me. |
Couple of lighting changes to the glTF 2.0 PBR shader here:
baseColor
instead ofdiffuseColor
in one part of the formula.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