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 point light #3711

Merged
merged 19 commits into from
Mar 18, 2016
Merged

Shadows point light #3711

merged 19 commits into from
Mar 18, 2016

Conversation

lilleyse
Copy link
Contributor

For #2594

I'll finish up #3691 first.

Some of the changes here will help with the terrain artifacts in that PR. v_normalEC wasn't getting to the receive-shadows shader correctly because terrain lighting is off by default. This caused back faces to not be shadowed. Now those varyings are outputted if the globe receives shadows. There are still some other artifacts to deal with though.

First I organized ShadowMap to handle point lights. It now uses the concept of passes - point light shadows have 6 passes, cascaded shadows have 4 passes, single shadow has one pass. Each pass has a PassState to set the viewport, and a camera. From scene's perspective it doesn't care what type of shadowing is done.

The point light shadow texture is basically a cube map that is flattened into a 2d texture. I'm a little worried about the amount of shader operations to convert from a cube-map direction to a 2d uv, but overall the benefits are that we can still use a depth texture, avoid binding different faces to the framebuffer for each pass, and overall cleaner integration with the existing code.

@lilleyse
Copy link
Contributor Author

point
point2

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

No OSX luck here either, but your screenshot looks awesome!

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

point light shadows have 6 passes, cascaded shadows have 4 passes, single shadow has one pass.

So, as of now, the cube map faces are not cascaded? I am go with that to start as we discussed offline. I expect that we will need it at some point though.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

The point light shadow texture is basically a cube map that is flattened into a 2d texture.

Was this useful to you: http://webglinsights.github.io/downloads/WebGL-Insights-Chapter-16.pdf

@@ -0,0 +1,270 @@
{
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 make this entire model? Do we need to credit anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this test model.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

In the future, we will also be interested in generating cube maps for relfection and refraction - and also rendering for picture-in-picture, multiple monitors, etc.

Is there anything you think we should do to the shadow map passes now in anticipation of this? Or just rework when we actually need to?

this._passStates[2].viewport = new BoundingRectangle(0, size, size, size);
this._passStates[3].viewport = new BoundingRectangle(size, size, size, size);
} else if (numberOfPasses === 6) {
// +----+----+----+
Copy link
Contributor

Choose a reason for hiding this comment

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

For our default shadow map size, any concern with this exceeding the typically WebGL limit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, as we discussed offline, we are going to have cases like spot lights where we know we won't need all 6 faces. At the least, we should be able to optimize the number of passes, but if we think it is worth it, I suppose we can also optimize the texture size (but not for dynamic field of views).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The texture size should be ok for now. The min texture size on most systems is 4096, so 1024*3 is fine.

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 spotlights are the next type I'll support, it should be just one pass. Even point lights could remove one pass if we know their top face is facing the sky, but may not be worth handling.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

I'm a little worried about the amount of shader operations to convert from a cube-map direction to a 2d...

I agree this is a lot of instructions, but they are all cheap.

How much did you look into optimizing this (most likely involves changing the layout as in Jeff's WebGL chapter)? Could be a TODO for later...

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 15, 2016

That's all my comments. Looks really cool. Can't wait to run this.

@lilleyse
Copy link
Contributor Author

How much did you look into optimizing this (most likely involves changing the layout as in Jeff's WebGL chapter)? Could be a TODO for later...

I tried to keep optimization in mind. There are two other direction-to-uv functions I found (that use branching), so I'll check those out. I may also just get it working with cube maps and see how it compares.

I took a look at the chapter, but since it's using octahedral projection it doesn't apply as well. I think the idea is the same though, and looks like one of those functions I found.

@lilleyse
Copy link
Contributor Author

So, as of now, the cube map faces are not cascaded? I am go with that to start as we discussed offline. I expect that we will need it at some point though.

No they don't use cascades. The results seems pretty good though even with low resolutions.

@lilleyse
Copy link
Contributor Author

Updated to fix the Chrome OSX problem.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 17, 2016

OS X fix looks good.

Does this PR include everything in #3691? Should we close that?

It doesn't need to be in this PR, but do you plan to add an option to load different test models in the Sandcastle demo and perhaps disable parts of the UI when they don't work (maybe that's overkill); right now, a lot of options don't apply to point lights?

Otherwise, this is all good with me. @bagnell can you merge this (and #3691?) when you have reviewed.

@lilleyse
Copy link
Contributor Author

Yes this includes all the comments in #3691, so it can be closed.

All those UI suggestions sound good to me. One thing I've wanted to fix for a while is rebuilding the terrain shaders when the shadow types change. Also fixing the models to turn shadowing on/off dynamically like primitives and terrain (using the derived commands from the 3d-tiles-styles branch).

@lilleyse
Copy link
Contributor Author

#3691 Has a couple commits that aren't in this PR, so you could merge that branch into shadows or I can cherry-pick them here.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 17, 2016

Cherry pick them here and close #3691.

@bagnell
Copy link
Contributor

bagnell commented Mar 17, 2016

Instead of enabling and disabling the UI, how about separate demos for each type of light source?

@@ -791,7 +1055,20 @@ define([
}
};

ShadowMap.prototype.updatePass = function(context, pass) {
if (this._isPointLight && this._usesCubeMap) {
this._framebuffer._attachTexture(context, WebGLConstants.COLOR_ATTACHMENT0, this._passFaces[pass]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to have a framebuffer for each face rather than changing color attachments. Binding a framebuffer should be faster than having to re-validate the framebuffer after a new texture is attached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try that out and see. The one disadvantage is each framebuffer needs its own depth texture (in addition to the shadow texture).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that isn't true... it should be fine.

@bagnell
Copy link
Contributor

bagnell commented Mar 17, 2016

In the future, we will also be interested in generating cube maps for relfection and refraction - and also rendering for picture-in-picture, multiple monitors, etc.
Is there anything you think we should do to the shadow map passes now in anticipation of this? Or just rework when we actually need to?

I'm also interested in what you think about this. You may want to keep in mind that we will want to have multiple light sources. I see why we would have a shadow map dedicated to the Sun, but does czm_sunShadowMapTextureCube make sense?

@bagnell
Copy link
Contributor

bagnell commented Mar 17, 2016

Other that those two comments, I think this is good to go.

@lilleyse
Copy link
Contributor Author

This is updated. While changing the point light uniform names I figured I should just change the others as well since the shadow map is not necessarily tied to the sun.

if (shadowMap._usesDepthTexture && (shadowMap._passFramebuffers[0].status !== WebGLConstants.FRAMEBUFFER_COMPLETE)) {
shadowMap._usesDepthTexture = false;
var colorMask = shadowMap._renderState.colorMask;
colorMask.red = colorMask.green = colorMask.blue = colorMask.alpha = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably not documented well, but RenderStates are read only. This allows us to cache them and make minimal render state changes between draw calls.

@lilleyse
Copy link
Contributor Author

Updated to use immutable render states and to fix some issues with destroying the cube-map fbo's.

bagnell added a commit that referenced this pull request Mar 18, 2016
@bagnell bagnell merged commit a471f00 into shadows Mar 18, 2016
@bagnell bagnell deleted the shadows-point-light branch March 18, 2016 19:22
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.

3 participants