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

Can't turn on shadows during viewer construction with Cesium OSM Buildings? #9137

Closed
OmarShehata opened this issue Sep 4, 2020 · 5 comments · Fixed by #9172
Closed

Can't turn on shadows during viewer construction with Cesium OSM Buildings? #9137

OmarShehata opened this issue Sep 4, 2020 · 5 comments · Fixed by #9172

Comments

@OmarShehata
Copy link
Contributor

OmarShehata commented Sep 4, 2020

Here's a Sandcastle with Cesium OSM Buildings. Click the button to toggle shadows, works fine.

If you enable shadows in the viewer constructor, you get:

An error occurred while rendering.  Rendering has stopped.
DeveloperError: normalized result is not a number
Error
    at new DeveloperError (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:83:13)
    at Function.Cartesian3.normalize (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:2040:13)
    at Ellipsoid.geodeticSurfaceNormal (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:3606:23)
    at Ellipsoid.cartesianToCartographic (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:3699:18)
    at fitShadowMapToScene (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:267333:40)
    at ShadowMap.update (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:267565:9)
    at updateShadowMaps (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:282939:17)
    at updateAndRenderPrimitives (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:282962:5)
    at executeCommandsInViewport (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:282734:7)
    at Scene.updateAndExecuteCommands (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:282477:7)
@mramato
Copy link
Contributor

mramato commented Sep 5, 2020

FYI, this only happens at certain times of the day (I just tried to reproduce and it didn't work). Basically you need shadows to actually be cast in the scene. I edited the original link to have the demo always crash.

@mramato
Copy link
Contributor

mramato commented Sep 5, 2020

This looks like a clear cut "something isn't initialized yet" bug which cascades into NaNs.

@jtorresfabra
Copy link
Contributor

jtorresfabra commented Sep 24, 2020

I've tracked this to be a bug in this condition https://github.com/CesiumGS/cesium/blob/master/Source/Scene/View.js#L322.
In fact is not very clear for me what the intention is, as the condition is difficult to read. The bug happens when commnadFar has a negative value.

Rewritting this condition to something like the below code fixes the crash:

(!(pass === Pass.GLOBE && commandNear < -100.0 && commandFar > 100.0) && commandFar > 0.0)

I would say the above fix is just a hack and I'm inclined to think the condition is not correct as it is, or at least it is difficult to understand what it does. However I don't feel very comfortable changing this code without more knowledge about what is the intention.

Maybe it could be rewritten in a friendlier and more understandable way if the current code is correct.

Probably @IanLilleyT could give more insights about it. (sorry for the too fast git blame)

cc: @lilleyse

@IanLilleyT
Copy link
Contributor

This is more for @lilleyse . I was the last one to modify this file, and may have even introduced a bug in the process (do we know when this bug started happening?), but I'm not familiar enough with the shadow mapping system to know if there are any downsides to your fix

@lilleyse
Copy link
Contributor

lilleyse commented Sep 26, 2020

Thanks for investigating this @jtorresfabra. That helped me narrow down the bug fairly quickly. I opened #9172.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants