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 #3856

Merged
merged 310 commits into from
May 29, 2016
Merged

Shadows #3856

merged 310 commits into from
May 29, 2016

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Apr 15, 2016

For #2594

This PR adds support for different types of shadows including cascaded directional shadows, spot light shadows, and point light shadows. It also adds the concept of derived commands which are used for OIT, shadows, and later 3D Tiles.

By default the scene contains a shadow map whose light source is the sun. It is disabled by default, but can be enabled with scene.shadowMap.enabled = true.

Shadow default settings:

Type Casts Shadows Receives Shadows
Globe False True
Primitives False False
Models True True

Any of these can change by setting castShadows or receiveShadows.

TODO

  • Unit tests
  • Cast shadows for terrain tiles that are out of view: on the shadows-globe branch.
  • Figure out the best approach approach for shadows when the light source is below the horizon. Currently if the globe casts shadows, the surface will be completely shadowed and dark. Maybe day-night shading should be on if shadows are on.
  • Possible support for GroundPrimitive if needed.
  • Integration with higher level constructs like Viewer.
  • 2D and Columbus view support.
  • Screenshots for twitter
  • Tech blog post
  • Tutorial, including use cases
  • Fix IE11 and Edge - no glPolygonOffset support

Testing TODO

  • Review ShadowMap.js
  • Step through in debugger
  • FPS with shadows on/off to get a ballpark
  • Retest aircraft and satellite CZMLs
  • Test with lots of 3D models
  • Terrain

TODO now or later

  • Use a good LOD when selecting tiles only in the shadow map frustum, e.g., not SSE from the light's perspective
  • Do not request imagery for tiles only in the shadow map frustum

@@ -505,7 +505,8 @@ define([
}

function screenSpaceError(primitive, frameState, tile) {
if (frameState.mode === SceneMode.SCENE2D) {
// TODO: check for sseDenominator so shadow cameras with orthographic frustum works. check correctness.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the plan with this TODO? Roadmap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely relevant for the shadows-globe branch, but I can remove it from this.

* @default false
* @private
*/
dirty : {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could just be a regular property without get/set functions.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 28, 2016

I submitted a few trivial style tweaks.

This is so close!

@pjcozzi pjcozzi mentioned this pull request May 28, 2016
44 tasks
@lilleyse
Copy link
Contributor Author

[.CommandBufferContext]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION : glClear: framebuffer incomplete (clear)

Do you also see this error when running the tests?

@lilleyse
Copy link
Contributor Author

Using the development Sandcastle example and the Shadow Tester 2 model, I noticed that disabling/enabling the normal offset shifts the shadow's location (on terrain, but not the model/primitive self-shadow).

Unfortunately this is a side effect of that technique. In the demo you can switch between bias modes, so if you turned it off/on for primitives you would notice the same thing. At some point these values worked well, but I think I can safely reduce them a bit now.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 28, 2016

Do you also see this error when running the tests?

Yes, when running http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/Specs/SpecRunner.html?spec=Scene%2FShadowMap

I get 27 of

SpecRunner.html:1 [.CommandBufferContext.Offscreen-MainThread-0x7ff8bd881150]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION : glClear: framebuffer incomplete (clear)

@pjcozzi
Copy link
Contributor

pjcozzi commented May 28, 2016

At some point these values worked well, but I think I can safely reduce them a bit now.

Sounds good.

Also, if the shadow shift is still noticeable, please document it so users that need ultra precision shadows can tune it.

@lilleyse
Copy link
Contributor Author

Updated. I don't have my laptop with me so it's hard to investigate the OSX/Chrome error...

var polygonOffsetSupported = true;
if (FeatureDetection.isInternetExplorer) {
polygonOffsetSupported = false;
}
this._polygonOffsetSupported = polygonOffsetSupported;

// The normalOffset bias pushes the shadows forward slightly, and may be disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this in the reference doc so users will see it.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 28, 2016

@lilleyse what do you suggest? Wait to merge until we figure out the Mac issue? Or merge now to increase the chances of more people trying it out in master?

@lilleyse
Copy link
Contributor Author

Updated. I was able to test it out and fix the problem.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 29, 2016

The WebGL error is gone, but I have one new shadow map test failure:

Ran 34 of 7358 specs - run all
34 specs, 1 failure
Spec List | Failures
Scene/ShadowMap directional shadow map
Expected [ 72, 72, 72, 255 ] to equal [ 240, 240, 240, 255 ].
Error: Expected [ 72, 72, 72, 255 ] to equal [ 240, 240, 240, 255 ].
    at stack (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1475:17)
    at buildExpectationResult (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1445:14)
    at Spec.expectationResultFactory (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:584:18)
    at Spec.addExpectationResult (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:332:34)
    at Expectation.addExpectationResult (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:528:21)
    at Expectation.toEqual (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1399:12)
    at verifyShadows (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/Specs/Scene/ShadowMapSpec.js:373:26)
    at http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/Specs/Scene/ShadowMapSpec.js:623:9
    at Object.<anonymous> (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/Specs/spec-main.js:139:30)
    at attemptAsync (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)
Expected [ 72, 72, 72, 255 ] to equal [ 240, 240, 240, 255 ].
Error: Expected [ 72, 72, 72, 255 ] to equal [ 240, 240, 240, 255 ].
    at stack (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1475:17)
    at buildExpectationResult (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1445:14)
    at Spec.expectationResultFactory (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:584:18)
    at Spec.addExpectationResult (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:332:34)
    at Expectation.addExpectationResult (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:528:21)
    at Expectation.toEqual (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1399:12)
    at verifyShadows (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/Specs/Scene/ShadowMapSpec.js:385:26)
    at http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/Specs/Scene/ShadowMapSpec.js:623:9
    at Object.<anonymous> (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/Specs/spec-main.js:139:30)
    at attemptAsync (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/shadows/ThirdParty/jasmine-2.2.0/jasmine.js:1793:24)

The blinking also appears to be fixed, but I thought I saw it one time, but I can't reproduce it.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 29, 2016

Is #2594 completely up-to-date? There are still items above (#3856 (comment)) that are not included. Is this because you plan to do them before we merge this?

@lilleyse
Copy link
Contributor Author

The problem there is most likely a z-fighting issue. I made a small change but I can't verify if it fixes the test. A lot of new aliasing problems like this are caused by changes in the browsers lately. In Chrome 49 polygonOffset seemed to work fine, but now there is no effect when toggling it in the demo.

@lilleyse
Copy link
Contributor Author

Is #2594 completely up-to-date? There are still items above (#3856 (comment)) that are not included. Is this because you plan to do them before we merge this?

I moved the rest of the unfinished items here to the roadmap.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 29, 2016

All looks good now.

I saw the blink again, this time from a stationary view, perhaps due to new terrain tiles loading. If this is a common issue, we'll track it down.

@pjcozzi pjcozzi merged commit bd3ddef into master May 29, 2016
@pjcozzi pjcozzi deleted the shadows branch May 29, 2016 15:16
@pjcozzi
Copy link
Contributor

pjcozzi commented May 29, 2016

Also update CHANGES.md in master to include a link to the Sandcastle demo (using the future cesiumjs.org base path).

@lilleyse
Copy link
Contributor Author

Also update CHANGES.md in master to include a link to the Sandcastle demo (using the future cesiumjs.org base path).

Done.

@shunter shunter mentioned this pull request Jun 15, 2016
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants