-
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
Custom light source #8493
Custom light source #8493
Conversation
Thanks for the pull request @lilleyse!
Reviewers, don't forget to make sure that:
|
14ad0b8
to
54f5a65
Compare
54f5a65
to
d7eba1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty thorough. Not having sun be the only light source is good.
My main API confusion is Globe.dynamicAtmosphereLightingFromSun
, since the light source is the sun in most cases anyway, and so users might think they must set this property too. I can't think of a better name though. This isn't a big deal as it might be solved in the future by giving the atmosphere system its own light source, and then we can remove this custom behavior for the sun.
Does anyone else want to review this before we merge? Mainly looking for feedback on the API. From CHANGES.md:
|
@lilleyse latest version looks good to me. Can't wait to use this, the flashlight is something we've wanted in Cesium since before it was even called Cesium. The sooner we can get this in the better since it should get as much testing as possible before the next release. I say we merge today and we can always tweak the API based on feedback before we release on Feb. @IanLilleyT any objections? Is there an updated issue with ideas for expanding this into an array of lights (and I assume adding different types of lights as well)? It would be could to capture your thoughts while still fresh in your head. Thanks again @lilleyse! |
Everything looks good! |
🔥 🔥 🔥 |
Fixes #6553
This PR adds more customizability to Cesium's lighting system. There is now a property
scene.light
that lets you swap the default sun light with a directional light of your choosing. For example:What this PR doesn't add is multiple light sources. That will need to be a future PR.
Some more notes about this PR:
light.color
is aColor
instance andlight.intensity
is a scalar multiplier which can greater than 1.0 to support HDR lighting. This seems to be similar to what ThreeJS, BabylonJS, and Blender do. Note that only glTF 2.0 PBR models actually use the HDR color, everything else uses an equivalent SDR color for their lighting calculations.phong.glsl
and other areas. This concept doesn't fit in the new design. The default material shininess is pretty rough so the specular contribution was not that noticeable anyways. In the future this would be implemented with two light sources.scene.sunColor
is deprecated and will be removed in 1.69. The recommended approach now is to setscene.light.color
. This property only affected glTF 2.0 PBR models whereasscene.light
affects all systems.KHR_techniques_webgl
models. Since the shaders are hardcoded there's no good fix except to upgrade to glTF 2.0 (Upgrade all SampleData models to gltf 2.0 #7802). I upgraded the Cesium Balloon in this PR and @sanjeetsuhag will upgrade the rest as part of onramping. The Bentley BIM tileset uses custom shaders heavily and actually hardcodesczm_sunDirectionWC
so we'd have to go through and change that toczm_lightDirectionWC
.globe.dynamicAtmosphereLighting
andglobe.dynamicAtmosphereLightingFromSun
to add more atmosphere and fog customization. This made the flashlight and moonlight demos look a lot better.Some screenshots from the Sandcastle example:
Fixed lighting
Flashlight
Moonlight
Sunlight
Custom color
To do:
scene.sunColor
in 1.69.