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

Add CIE Clear Sky model to procedural environment #7134

Merged
merged 12 commits into from
Oct 22, 2018
Merged

Add CIE Clear Sky model to procedural environment #7134

merged 12 commits into from
Oct 22, 2018

Conversation

OmarShehata
Copy link
Contributor

@OmarShehata OmarShehata commented Oct 10, 2018

This is the same as #7123 but re-branched off the base branch.

You should be able to actually test this out here @bagnell .

I'm going to experiment with multiplying by the atmosphere color to get an orange-ish tint as the sun sets relative to the object. Will bump this PR with some pictures once that's in.

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

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

🌍 🌎 🌏

bagnell and others added 2 commits October 11, 2018 16:50
@OmarShehata
Copy link
Contributor Author

I've tried adding the atmosphere color using the GroundAtmosphere shader, and it does look nice as it goes from yellow to red as the sun sets:

sunset

However, if you look real close, there are these noisy artifacts, I'm not sure if that's just because the ground atmosphere wasn't intended to be used at this scale @bagnell ?

look_close

The color also seems to be determined by the camera direction, but it seems to me that it should be purely based on the angle from the sun to the horizon.

I think it would be simpler and would look better simply to use that computed angle to add a reddish tint for small values of that angle. I'm still tweaking it to find a model that looks good yet subtle.

Add Trilinear Filtering to Octahedral Maps
…specular maps. Remove diffuse maps in favor of SH.
Add Trilinear Filtering to Octahedral Maps
@OmarShehata OmarShehata changed the base branch from ibl to master October 15, 2018 18:54
@OmarShehata OmarShehata changed the base branch from master to ibl October 15, 2018 18:54
@OmarShehata
Copy link
Contributor Author

@bagnell I think this should be ready to merge. I couldn't find an easy way to make a reddish tint at sunrise/sunset look good.

// Angle between zenith and current pixel
fragmentShader += ' float theta = acos(clamp(dot(normalize(czm_inverseViewRotation * n), normalize(v_positionWC * -1.0)), 0.001, 1.0));\n';
// Angle between sun and current pixel
fragmentShader += ' float gamma = acos(NdotL);\n';
Copy link
Contributor

@bagnell bagnell Oct 15, 2018

Choose a reason for hiding this comment

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

For S, theta, and gamma, you don't need to use acos just to call cos on it again. For example, in the luminance below, there is pow(cos(gamma), 2.0) which is pow(cos(acos(NdotL)), 2.0) which can be simplified to just NdotL * NdotL. You can do that for all three.

// Angle between sun and current pixel
fragmentShader += ' float gamma = acos(NdotL);\n';
fragmentShader += ' float luminanceAtZenith = 10.0;\n';
fragmentShader += ' float luminance = luminanceAtZenith * (((0.91 + 10.0 * exp(-3.0 * gamma) + 0.45 * pow(cos(gamma), 2.0)) * (1.0 - exp(-0.32 / cos(theta)))) / (0.91 + 10.0 * exp(-3.0 * S) + 0.45 * pow(cos(S),2.0)) * (1.0 - exp(-0.32)));\n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split this up for readability? Perhaps, numerator and denominator and luminance = luminanceAtZenith * (numerator/ denominator);

fragmentShader += ' float theta = acos(clamp(dot(normalize(czm_inverseViewRotation * n), normalize(v_positionWC * -1.0)), 0.001, 1.0));\n';
// Angle between sun and current pixel
fragmentShader += ' float gamma = acos(NdotL);\n';
fragmentShader += ' float luminanceAtZenith = 10.0;\n';
Copy link
Contributor

@bagnell bagnell Oct 15, 2018

Choose a reason for hiding this comment

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

Where did you get this value? Should it be configurable as a uniform?

@bagnell
Copy link
Contributor

bagnell commented Oct 15, 2018

I think it looks good. What do you think @emackey?
With the luminance:
image
Without the luminance:
image
EDIT: I should note this is a branch off of the HDR branch.

@OmarShehata
Copy link
Contributor Author

@bagnell good suggestion about splitting it into numerator and denominator, I caught a missing paren. Subsequently, the new luminanceAtZenith value that looks good is 1.0 instead of 10.0. I changed it to a uniform and exposed it as a property on the model.

I also used the dot product directly where appropriate instead of cos(acos(..)). Should be good now.

@OmarShehata OmarShehata mentioned this pull request Oct 17, 2018
7 tasks
@lilleyse
Copy link
Contributor

@OmarShehata @bagnell can this be merged into ibl?

@OmarShehata
Copy link
Contributor Author

It's good on my end @lilleyse. The only thing Dan was wondering about was whether the default luminance of 10 was too much since I only tested it on the spheres, but I just tried with other models and I think it's a good default (and I imagine it would look a lot better in the HDR branch). It's easy to tweak the default later in any case.

@lilleyse
Copy link
Contributor

Ok, it should be fine to merge now then and review everything together in #7172.

@lilleyse lilleyse merged commit 79bc2c7 into CesiumGS:ibl Oct 22, 2018
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.

4 participants