Shadow Map Sample

Added a new sample which implements basic shadow mapping.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21000

[apatrick, 2009-07-11 00:54:20]

You need to add it to the MANIFEST file.

You could add it to the list of samples in selenium and add a reference image.

It might be useful for some people if there was a link to an article describing
the algorithm since it's quite complicated.

LGTM

http://codereview.chromium.org/155401/diff/1002/1003
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/1002/1003#newcode5
Line 5: Author: Vangelis Kokkevis (vangelis@google.com)
change comment header

http://codereview.chromium.org/155401/diff/1002/1003#newcode638
Line 638: Use A, S, D, W, I and O to move the light.  Press spacebar to see the
shadow map.
80 chars

[gman, 2009-07-11 02:52:26]

this is awesome!!!

Just a few comments.

http://codereview.chromium.org/155401/diff/1002/1003
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/1002/1003#newcode157
Line 157: g_shadowViewInfo = o3djs.rendergraph.createBasicView(
We should probably make some more o3djs.rendergraph functions but just fyi. It's
inefficient to have 2 views because that creates 2 TreeTraversal objects which
will walk the entire transform graph once each.

Instead, if you create a 
DrawList
DrawPass
DrawContext

Then put the DrawPass under your RenderSurfaceSet and register your DrawList
with the TreeTraversal in g_viewInfo.treeTraversal then you'll only have 1
TreeTraversal and the transform graph will only need to be walked once.

I'm not saying you have to make that change now. But I am saying we (me?) should
probably look into adding some thing like

ViewInfo.createRenderPass that does those steps for you and sets things up the
most efficient way.

What do you think? After you check this in, if I write those functions should I
change this to use them or is it more clear the way it is?

http://codereview.chromium.org/155401/diff/1002/1003#newcode344
Line 344: function createFrustumVertices(matrix) {
If this is just a unit cube in positive z then you can do this with

cubeShape = o3djs.debug.createLineCube(
    g_pack,
    createColorMaterial([1, 0, 0, 1]),
    1,
    g_math.matrix4.translation([0, 0, 0.5]));

http://codereview.chromium.org/155401/diff/1002/1003#newcode370
Line 370: function createColorMaterial(baseColor) {
Creating a constant color material seems common enough we should probably put it
in material.js.  What do you think? Here's the code to do it if you don't mind
basing it off what's there.

/**
 * This function creates a constant material. 
 *
 * @param {!o3d.Pack} pack Pack to manage created objects.
 * @param {!o3djs.rendergraph.ViewInfo} viewInfo as returned from
 *     o3djs.rendergraph.createBasicView.
 * @param {(!o3djs.math.Vector4|!o3d.Texture)} colorOrTexture Either a color in
 *     the format [r, g, b, a] or an O3D texture.
 * @param {boolean} opt_transparent Whether or not the material is transparent.
 *     Defaults to false.
 * @return {!o3d.Material} The created material.
 */
o3djs.material.createConstantMaterial = function(pack,
                                                 viewInfo,
                                                 colorOrTexture,
                                                 opt_transparent) {
  var material = pack.createObject('Material');
  material.drawList = opt_transparent ? viewInfo.zOrderedDrawList :
                                        viewInfo.performanceDrawList;

  // If it has a length assume it's a color, otherwise assume it's a texture.
  if (colorOrTexture.length) {
    material.createParam('emissive', 'ParamFloat4').value = colorOrTexture;
  } else {
    var paramSampler = material.createParam('emissiveSampler', 'ParamSampler');
    var sampler = pack.createObject('Sampler');
    paramSampler.value = sampler;
    sampler.texture = colorOrTexture;
  }

  o3djs.material.attachStandardEffect(pack, material, viewInfo, 'constant');

  return material;
};

Then you'd change line 331 to

var shape = frustumVertices.createShape(
    g_pack, 
    o3djs.material.createConstantMaterial(
        g_pack, 
        g_viewInfo, 
        [1, 0, 0, 1]));

PS: If you think it's better to call it o3djs.material.createColorMaterial
that's fine too.

[vangelis, 2009-07-13 17:08:15]

Thanks for shaping this sample up.  It looks good!

I agree with Al's comment that we should add a more detailed description of how
the two passes are actually set up here and how the shadows are generated.  You
don't need to linger too much on the details of the shadowmaping technique (feel
free to add a pointer to someplace online that explains the technique) but you
should at least highlight portions of it that are specific to O3D.  I think a
general description of the technique at the top of the sample as well as some
more detailed comments in the code would go a long way explaining what's going
on.  

As a separate task I still think it would be quite valuable to write a chapter
in the docs that demonstrates the power of the rendergraph and this example
could be used as one of the sections.

http://codereview.chromium.org/155401/diff/1002/1003
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/1002/1003#newcode85
Line 85: // Add the shapes to the transform heirarchy.
typo: hierarchy

http://codereview.chromium.org/155401/diff/1002/1003#newcode344
Line 344: function createFrustumVertices(matrix) {
On 2009/07/11 02:52:26, gman wrote:
> If this is just a unit cube in positive z then you can do this with
> 
> cubeShape = o3djs.debug.createLineCube(
>     g_pack,
>     createColorMaterial([1, 0, 0, 1]),
>     1,
>     g_math.matrix4.translation([0, 0, 0.5]));
> 
The size along z is actually half of the size along x and y.  Having said that,
it might be worthwhile adding a 'frustum' type in the debug library as folks
experimenting with shadows would probably find it useful.

http://codereview.chromium.org/155401/diff/1002/1003#newcode369
Line 369: 
Please add descriptions to the following four functions as well.

http://codereview.chromium.org/155401/diff/1002/1003#newcode502
Line 502: 
It would be nice to have a short description of what each shader does as this is
at the heart of how the projected shadows work.

[gman, 2009-07-13 17:18:28]

http://codereview.chromium.org/155401/diff/1002/1003
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/1002/1003#newcode344
Line 344: function createFrustumVertices(matrix) {
On 2009/07/13 17:08:15, vangelis wrote:
> On 2009/07/11 02:52:26, gman wrote:
> > If this is just a unit cube in positive z then you can do this with
> > 
> > cubeShape = o3djs.debug.createLineCube(
> >     g_pack,
> >     createColorMaterial([1, 0, 0, 1]),
> >     1,
> >     g_math.matrix4.translation([0, 0, 0.5]));
> > 
> The size along z is actually half of the size along x and y.  Having said
that,
> it might be worthwhile adding a 'frustum' type in the debug library as folks
> experimenting with shadows would probably find it useful.
> 

Writing a o3djs.debug.createFrustum is fine. I'm just pointing out that it can
be based off of createLineCube

cubeShape = o3djs.debug.createLineCube(
    g_pack,
    createColorMaterial([1, 0, 0, 1]),
    1, 
    g_math.matrix4.mul(
      g_math.matrix4.translation([0, 0, 0.25]),
      g_math.matrix4.scale([1, 1, 0.5]))); 

Not sure of those last 2 lines need to be reversed or not, or you could just
type the matrix.

cubeShape = o3djs.debug.createLineCube(
    g_pack,
    createColorMaterial([1, 0, 0, 1]),
    1, 
    [[1, 0, 0, 0]
     [0, 1, 0, 0]
     [0, 0, 0.5, 0]
     [0, 0, 0.25, 0]]);
  
I think it's important to note that (a) it's a box and (b) the matrix is what is
bending it to look like a frustum.

[petersont, 2009-07-13 18:57:27]

http://codereview.chromium.org/155401/diff/1002/1003
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/1002/1003#newcode5
Line 5: Author: Vangelis Kokkevis (vangelis@google.com)
On 2009/07/11 00:54:20, apatrick wrote:
> change comment header

Done.

http://codereview.chromium.org/155401/diff/1002/1003#newcode85
Line 85: // Add the shapes to the transform heirarchy.
On 2009/07/13 17:08:15, vangelis wrote:
> typo: hierarchy

Done.

http://codereview.chromium.org/155401/diff/1002/1003#newcode157
Line 157: g_shadowViewInfo = o3djs.rendergraph.createBasicView(
> What do you think? After you check this in, if I write those functions should
I
> change this to use them or is it more clear the way it is?


It sounds like it might actually be more clear that way, because there really is
one view, and two passes.  I say go for it.  Sometime I'll get you to explain to
me why there only has to be one TreeTraversal.


On 2009/07/11 02:52:26, gman wrote:
> We should probably make some more o3djs.rendergraph functions but just fyi.
It's
> inefficient to have 2 views because that creates 2 TreeTraversal objects which
> will walk the entire transform graph once each.
> 
> Instead, if you create a 
> DrawList
> DrawPass
> DrawContext
> 
> Then put the DrawPass under your RenderSurfaceSet and register your DrawList
> with the TreeTraversal in g_viewInfo.treeTraversal then you'll only have 1
> TreeTraversal and the transform graph will only need to be walked once.
> 
> I'm not saying you have to make that change now. But I am saying we (me?)
should
> probably look into adding some thing like
> 
> ViewInfo.createRenderPass that does those steps for you and sets things up the
> most efficient way.
> 
> What do you think? After you check this in, if I write those functions should
I
> change this to use them or is it more clear the way it is?

http://codereview.chromium.org/155401/diff/1002/1003#newcode344
Line 344: function createFrustumVertices(matrix) {
On 2009/07/13 17:18:28, gman wrote:
> On 2009/07/13 17:08:15, vangelis wrote:
> > On 2009/07/11 02:52:26, gman wrote:
> > > If this is just a unit cube in positive z then you can do this with
> > > 
> > > cubeShape = o3djs.debug.createLineCube(
> > >     g_pack,
> > >     createColorMaterial([1, 0, 0, 1]),
> > >     1,
> > >     g_math.matrix4.translation([0, 0, 0.5]));
> > > 
> > The size along z is actually half of the size along x and y.  Having said
> that,
> > it might be worthwhile adding a 'frustum' type in the debug library as folks
> > experimenting with shadows would probably find it useful.
> > 
> 
> Writing a o3djs.debug.createFrustum is fine. I'm just pointing out that it can
> be based off of createLineCube
> 
> cubeShape = o3djs.debug.createLineCube(
>     g_pack,
>     createColorMaterial([1, 0, 0, 1]),
>     1, 
>     g_math.matrix4.mul(
>       g_math.matrix4.translation([0, 0, 0.25]),
>       g_math.matrix4.scale([1, 1, 0.5]))); 
> 
> Not sure of those last 2 lines need to be reversed or not, or you could just
> type the matrix.
> 
> cubeShape = o3djs.debug.createLineCube(
>     g_pack,
>     createColorMaterial([1, 0, 0, 1]),
>     1, 
>     [[1, 0, 0, 0]
>      [0, 1, 0, 0]
>      [0, 0, 0.5, 0]
>      [0, 0, 0.25, 0]]);
>   
> I think it's important to note that (a) it's a box and (b) the matrix is what
is
> bending it to look like a frustum.

Done.

http://codereview.chromium.org/155401/diff/1002/1003#newcode369
Line 369: 
On 2009/07/13 17:08:15, vangelis wrote:
> Please add descriptions to the following four functions as well.

Done.

http://codereview.chromium.org/155401/diff/1002/1003#newcode370
Line 370: function createColorMaterial(baseColor) {
On 2009/07/11 02:52:26, gman wrote:
> Creating a constant color material seems common enough we should probably put
it
> in material.js.  What do you think? Here's the code to do it if you don't mind
> basing it off what's there.
> 
> /**
>  * This function creates a constant material. 
>  *
>  * @param {!o3d.Pack} pack Pack to manage created objects.
>  * @param {!o3djs.rendergraph.ViewInfo} viewInfo as returned from
>  *     o3djs.rendergraph.createBasicView.
>  * @param {(!o3djs.math.Vector4|!o3d.Texture)} colorOrTexture Either a color
in
>  *     the format [r, g, b, a] or an O3D texture.
>  * @param {boolean} opt_transparent Whether or not the material is
transparent.
>  *     Defaults to false.
>  * @return {!o3d.Material} The created material.
>  */
> o3djs.material.createConstantMaterial = function(pack,
>                                                  viewInfo,
>                                                  colorOrTexture,
>                                                  opt_transparent) {
>   var material = pack.createObject('Material');
>   material.drawList = opt_transparent ? viewInfo.zOrderedDrawList :
>                                         viewInfo.performanceDrawList;
> 
>   // If it has a length assume it's a color, otherwise assume it's a texture.
>   if (colorOrTexture.length) {
>     material.createParam('emissive', 'ParamFloat4').value = colorOrTexture;
>   } else {
>     var paramSampler = material.createParam('emissiveSampler',
'ParamSampler');
>     var sampler = pack.createObject('Sampler');
>     paramSampler.value = sampler;
>     sampler.texture = colorOrTexture;
>   }
> 
>   o3djs.material.attachStandardEffect(pack, material, viewInfo, 'constant');
> 
>   return material;
> };
> 
> Then you'd change line 331 to
> 
> var shape = frustumVertices.createShape(
>     g_pack, 
>     o3djs.material.createConstantMaterial(
>         g_pack, 
>         g_viewInfo, 
>         [1, 0, 0, 1]));
> 
> PS: If you think it's better to call it o3djs.material.createColorMaterial
> that's fine too.

Done.

http://codereview.chromium.org/155401/diff/1002/1003#newcode502
Line 502: 
On 2009/07/13 17:08:15, vangelis wrote:
> It would be nice to have a short description of what each shader does as this
is
> at the heart of how the projected shadows work.

Done.

http://codereview.chromium.org/155401/diff/1002/1003#newcode638
Line 638: Use A, S, D, W, I and O to move the light.  Press spacebar to see the
shadow map.
On 2009/07/11 00:54:20, apatrick wrote:
> 80 chars

Done.

[vangelis, 2009-07-13 19:29:03]

Really like the shader explanations.  Short and to the point!  

I have a couple more comments below.  In general since this is supposed to be an
instructive sample and deals with a slightly complicated rendergraph setup I'd
like to see a few more comments in how everything is really put together.

http://codereview.chromium.org/155401/diff/1009/4
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/1009/4#newcode33
Line 33: This sample implements a basic shadow map.
I still feel that this sample requires a couple more lines of explanation up on
top so that folks can have an idea of what it demonstrates.  This is especially
important now that we have the drop shadows examples as well.

http://codereview.chromium.org/155401/diff/1009/4#newcode153
Line 153: */
I still think that this function could use a bit more details in the comments. 
Maybe some text to make it obvious that you're creating two branches in the
render tree, one that renders into a RenderSurface set which will use the shadow
shader and the other one that's the normal color pass that uses the results from
the first pass.

http://codereview.chromium.org/155401/diff/1009/4#newcode171
Line 171: g_colorPassRenderRoot.priority = 1;
Worth commenting on the fact that setting the priorities this way will force the
shadow pass to go first.

http://codereview.chromium.org/155401/diff/1009/4#newcode185
Line 185: g_renderSurfaceSet,
It might be more clear if we used the shadowPassRenderRoot here instead of
g_renderSurfaceSet so that it's consistent with how we set up the color pass.

http://codereview.chromium.org/155401/diff/1009/4#newcode189
Line 189: g_viewInfo = o3djs.rendergraph.createBasicView(
For consistency, g_viewInfo could be called g_colorViewInfo?

http://codereview.chromium.org/155401/diff/1009/4#newcode202
Line 202: g_shadowViewInfo.root.parent = g_renderSurfaceSet;
If you make the root change I suggested above, this should also change to point
to the shadowPassRenderRoot.

http://codereview.chromium.org/155401/diff/1009/4#newcode339
Line 339: transformTable[tt].shape.createDrawElements(g_pack, g_shadowMaterial);
This part here is where the magic happens that allows the shape to be drawn
twice with two different materials.  Can we highlight it more with a bit more
detailed comments?

[petersont, 2009-07-16 22:23:57]

Improved comments, added to the test list, please have another look.

http://codereview.chromium.org/155401/diff/1009/4
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/1009/4#newcode33
Line 33: This sample implements a basic shadow map.
On 2009/07/13 19:29:03, vangelis wrote:
> I still feel that this sample requires a couple more lines of explanation up
on
> top so that folks can have an idea of what it demonstrates.  This is
especially
> important now that we have the drop shadows examples as well. 

Done.

http://codereview.chromium.org/155401/diff/1009/4#newcode171
Line 171: g_colorPassRenderRoot.priority = 1;
On 2009/07/13 19:29:03, vangelis wrote:
> Worth commenting on the fact that setting the priorities this way will force
the
> shadow pass to go first. 

Done.

http://codereview.chromium.org/155401/diff/1009/4#newcode185
Line 185: g_renderSurfaceSet,
I don't understand what you mean.  If you put shadowPassRenderRoot there, it
doesn't work.

On 2009/07/13 19:29:03, vangelis wrote:
> It might be more clear if we used the shadowPassRenderRoot here instead of
> g_renderSurfaceSet so that it's consistent with how we set up the color pass.

http://codereview.chromium.org/155401/diff/1009/4#newcode189
Line 189: g_viewInfo = o3djs.rendergraph.createBasicView(
On 2009/07/13 19:29:03, vangelis wrote:
> For consistency, g_viewInfo could be called g_colorViewInfo?

Done.

http://codereview.chromium.org/155401/diff/1009/4#newcode202
Line 202: g_shadowViewInfo.root.parent = g_renderSurfaceSet;
Do you want the shadowPassRenderRoot to be under the surfaceset?

On 2009/07/13 19:29:03, vangelis wrote:
> If you make the root change I suggested above, this should also change to
point
> to the shadowPassRenderRoot.

[vangelis, 2009-07-16 22:59:47]

LGTM.  Don't forget to bump up the version for o3d-assets in the DEPS file to
include the reference screenshot.

http://codereview.chromium.org/155401/diff/21/1021
File core/cross/gl/renderer_gl.cc (right):

http://codereview.chromium.org/155401/diff/21/1021#newcode1571
Line 1571: // Note: glReadPixels caputres the alpha component of the frame
buffer as well
typo: captures

http://codereview.chromium.org/155401/diff/21/1021#newcode1572
Line 1572: // as the color components, the graphics pipeline usually ignores the
alpha
graphics pipeline usually -> browser

http://codereview.chromium.org/155401/diff/21/1018
File samples/shadow-map.html (right):

http://codereview.chromium.org/155401/diff/21/1018#newcode36
Line 36: The technique works by rendering the scene in two passes.  The first
pass renders the geometry in the scene with a shader that colors each pixel a
shade of gray representing how far the rendered point is from the light source. 
That image, the shadow map, is rendered to a texture, and then the second
(visible) render pass samples it to determine which points in the scene are in
shaodow.
80 char limit

http://codereview.chromium.org/155401/diff/21/1018#newcode154
Line 154: * point if view, arranges for the view from the light to be rendered
to a
if -> of

http://codereview.chromium.org/155401/diff/21/1018#newcode223
Line 223: * And loads effects for materials in the scene.  Other materials are
created
And -> and

http://codereview.chromium.org/155401/diff/21/1018#newcode369
Line 369: g_math.matrix4.compose(
worthwhile to add a comment here saying that you are essentially creating a box
with that extends from [-1, -1, 0] to [1, 1, 1]