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

Shadows for 3D Tiles #4002

Merged
merged 5 commits into from
Jun 7, 2016
Merged

Shadows for 3D Tiles #4002

merged 5 commits into from
Jun 7, 2016

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jun 6, 2016

For #2594

I still have to figure out why the translucent commands are not casting shadows. They seem to get to the step of casting into the shadow map but the shadow map is empty... I'll figure it out soon.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

Also for #3241.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

I still have to figure out why the translucent commands are not casting shadows

They are probably not writing depth. This is also an issue for picking. Update the TODO or fix it now if you want.

See https://github.com/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/Cesium3DTileBatchTableResources.js#L416

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 6, 2016

Ah.. that must be it. I'll try to fix the TODO as part of this PR.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

Do you get this test failure in this branch?

Scene/ShadowMap shadows are disabled during the pick pass
TypeError: Cannot read property 'receiveCommand' of undefined
TypeError: Cannot read property 'receiveCommand' of undefined
    at executeCommand (http://localhost:8080/Source/Scene/Scene.js:1495:44)
    at executeTranslucentCommandsSortedMRT (http://localhost:8080/Source/Scene/OIT.js:567:13)
    at OIT.executeCommands (http://localhost:8080/Source/Scene/OIT.js:575:13)
    at scene._executeOITFunction (http://localhost:8080/Source/Scene/Scene.js:1685:32)
    at executeCommands (http://localhost:8080/Source/Scene/Scene.js:1786:13)
    at executeCommandsInViewport (http://localhost:8080/Source/Scene/Scene.js:2104:9)
    at updateAndExecuteCommands (http://localhost:8080/Source/Scene/Scene.js:1963:17)
    at render (http://localhost:8080/Source/Scene/Scene.js:2369:9)
    at Scene.render (http://localhost:8080/Source/Scene/Scene.js:2407:13)
    at render (http://localhost:8080/Specs/Scene/ShadowMapSpec.js:352:15)

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

Do you get this in the 3D Tiles Sandcastle example? Clean build, cleared cache, forced reload.

image

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 6, 2016

I'm not seeing either issue on Chrome, even after hard reload. But I am seeing it on Firefox and it reminds me of what used to happen. I do have a fix for it, but it's more of a band-aid.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

I do have a fix for it, but it's more of a band-aid.

Push it and I'll see if I have any other ideas.

@@ -219,7 +219,7 @@
size : 1024,
modelOptions : ['Wood Tower', 'Cesium Air', 'Cesium Man', 'Transparent Box', 'Shadow Tester', 'Shadow Tester 2', 'Shadow Tester 3', 'Shadow Tester 4', 'Shadow Tester Point'],
model : 'Shadow Tester',
locationOptions : ['Exton', 'Everest', 'Pinnacle PA', 'Seneca Rocks', 'Half Dome'],
locationOptions : ['Exton', 'Everest', 'Pinnacle PA', 'Seneca Rocks', 'Half Dome', 'Seattle 3D Tiles', 'NYC 3D Tiles'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to commit these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because it helps to see real use cases inside development/Shadows.html. But I can see how this is a problem. I'll switch to using the test 3D Tiles sets we have and change to using Seattle/etc locally.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 6, 2016

Does the 3D Tileset primitive need any changes to its update function so the shadow map pass doesn't impact the load queue or cache?

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 6, 2016

Updated with the "fix".

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 6, 2016

Does the 3D Tileset primitive need any changes to its update function so the shadow map pass doesn't impact the load queue or cache?

I don't think so. It will just use whatever commands are pushed from the render pass.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 7, 2016

I fixed the issue with translucent buildings not casting into the shadow map. It was not related to the depth writing, instead it was caused by the pass == translucent checks in the batching shader. I needed to set the pass right before casting in order to fix that problem. I left the picking depth TODO in for now since it's unrelated.

Also in regards to the crashes we were seeing, the reason behind that is OIT commands, being derived from the original commands, do not have derived commands themselves, which is why we were seeing problems with derivedCommands.shadows being undefined. But since the OIT commands have shadows code built in, it all works out. The fix I made is actually fine.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 7, 2016

When this is ready I'm going to merge the Scene.js changes into master as well.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 7, 2016

Can you fix the JSHint warnings even if they are not related to this PR? https://travis-ci.org/AnalyticalGraphicsInc/cesium/builds/135932620

@@ -1491,7 +1491,7 @@ define([

if (scene.debugShowCommands || scene.debugShowFrustums) {
executeDebugCommand(command, scene, passState);
} else if (scene.frameState.shadowHints.shadowsEnabled && command.receiveShadows) {
} else if (scene.frameState.shadowHints.shadowsEnabled && command.receiveShadows && defined(command.derivedCommands.shadows)) {
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 add a comment here like the GiHub comment since the overall order of operations may be non-obvious to those new to the code?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 7, 2016

Merging, just make those other changes in the 3d-tiles branch.

@pjcozzi pjcozzi merged commit a75011e into 3d-tiles Jun 7, 2016
@pjcozzi pjcozzi deleted the 3d-tiles-shadows branch June 7, 2016 18:07
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.

2 participants