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

Sun lighting option for Models and 3D Tilesets #6160

Closed
wants to merge 5 commits into from

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jan 26, 2018

If a model uses the KHR_materials_common extension or is a 2.0 PBR model, the shaders we generate use the Cesium sun as the light source.

This PR adds an enableLighting option to Cesium3DTileset, Model, and ModelInstanceCollection that controls whether models are shaded based on the sun direction (the default) or based on the camera direction. The latter is better when the sun is on the opposite side of the globe and the model just appears black.

This options also controls lighting for point clouds that contain normals.

The reason enableLighting can't be changed dynamically is we still don't have a good way of recompiling shaders for models. Until then this will need to stay as a constructor parameter.

TODO

  • Add to entity API

gltf-pipeline PR: CesiumGS/gltf-pipeline#346

For https://groups.google.com/forum/#!topic/cesium-dev/vIMLLYxYx4g

@cesium-concierge
Copy link

Signed CLA is on file.

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, 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.

🌍 🌎 🌏

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2018

Other than the point cloud lighting, I'm not 100% sure that we want this change - for "directionless" lighting I would rather go with AO + irradiance environment maps. A light on the camera doesn't always produce good results for edge on geometry.

@lilleyse
Copy link
Contributor Author

Would it be okay to go with this low tech solution until then?

At least for PBR models the edge lighting looks pretty good because we still have environment lighting.
helmet

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2018

I suppose - it is easy enough to deprecate. Consider renaming it to sunLighting, eyeLighting, etc.

@lilleyse
Copy link
Contributor Author

Updated.

Originally I went with enableLighting to match the Globe.enableLighting, but sunLighting is a better name.

@lilleyse lilleyse mentioned this pull request May 2, 2018
@cesium-concierge
Copy link

Thanks again for the pull request!

Looks like this pull request hasn't been updated in 30 days since I last commented.

To keep things tidy should this be closed? Perhaps keep the branch and submit an issue?

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

🌍 🌎 🌏

@lilleyse
Copy link
Contributor Author

Closing for now - we can restart this later if there is more need, but it will probably be a scene wide option for #6553.

@lilleyse lilleyse closed this Jun 22, 2018
@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/vIMLLYxYx4g

If this issue affects any of these threads, please post a comment like the following:

The issue at #6160 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.


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

🌍 🌎 🌏

@lilleyse lilleyse deleted the enable-lighting branch June 22, 2018 16:05
@lilleyse lilleyse restored the enable-lighting branch June 22, 2018 16:06
@lilleyse lilleyse deleted the enable-lighting branch June 22, 2018 16:08
@lilleyse
Copy link
Contributor Author

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 22, 2018

@lilleyse if you haven't already, probably a good idea to link to that branch (and all parked branches) from an issue.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 25, 2018

@lilleyse why did you park this on your fork instead of our repo? And yes, please open an issue in this repo if one doesn't already exist

@lilleyse
Copy link
Contributor Author

I didn't want to keep a stale branch around, but still wanted somewhere to look at the code later. Should I push the branch here instead?

This issue is being tracked in #6553

@hpinkos
Copy link
Contributor

hpinkos commented Jun 25, 2018

@lilleyse yes, I think that's preferred so it would be easy for anyone (not necessarily just you) to pick it back up and start working on it if there's an interest. So yeah, I would make a branch in this repo and add a link to it in #6553 so anyone interested in seeing the work on it would be able to find it.

@lilleyse lilleyse restored the enable-lighting branch June 25, 2018 18:14
@lilleyse
Copy link
Contributor Author

Ok, I restored the branch and updated the link in #6553.

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