o3djs: Multiple simple lights, and a new demo.

Multiple simple lights, and a new demo.


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

[Patrick Horn, 2010-09-09 08:19:42]

The gist of this change that if a user turns on multiple lights, the light
parameters become arrays, and the code which does light computation runs in a
for loop, so that the lights add together.

To do this, I had to add plumbing for a 'opt_options' argument into the shader
generator to keep compatibility, which can possibly be used for other changes as
well, so much of the changelist is that.

This is just a request for comments right now--I haven't tested yet with the O3D
plugin, but there's no technical reason it wouldn't work there.

[petersont, 2010-09-14 18:55:44]

I think this is a great idea.  A few comments, also let's make sure it outputs
good cg as well as glsl.

http://codereview.chromium.org/3358020/diff/2001/3002
File o3djs/effect.js (right):

http://codereview.chromium.org/3358020/diff/2001/3002#newcode1038
o3djs/effect.js:1038: function beginLightLoop() {
Should make beginLightLoop and endLightLoop vars and comment them.

http://codereview.chromium.org/3358020/diff/2001/3002#newcode1282
o3djs/effect.js:1282: * Builds the surface to light code for the vertex shader.
Change comment.

http://codereview.chromium.org/3358020/diff/2001/3002#newcode1388
o3djs/effect.js:1388: * @param {object=} opt_options Extra options for
buildStandardShaderString
stray =

http://codereview.chromium.org/3358020/diff/2001/3003
File o3djs/material.js (right):

http://codereview.chromium.org/3358020/diff/2001/3003#newcode116
o3djs/material.js:116: * @param {object=} opt_options Extra options for
effect.getStandardShader
Stray =, also should capitalize Object, and it'd be nice to add to the comment
to say what the possible fields of opt_options are.  On second thought, why not
add that comment in effect.getStandardShader and reference it here.

[Patrick Horn, 2010-09-17 04:23:49]

Daniel got things working in the plugin by using createParam on the arrays.

http://codereview.chromium.org/3358020/diff/2001/3002
File o3djs/effect.js (right):

http://codereview.chromium.org/3358020/diff/2001/3002#newcode1038
o3djs/effect.js:1038: function beginLightLoop() {
On 2010/09/14 18:55:45, petersont wrote:
> Should make beginLightLoop and endLightLoop vars and comment them.

Done.

http://codereview.chromium.org/3358020/diff/2001/3002#newcode1282
o3djs/effect.js:1282: * Builds the surface to light code for the vertex shader.
On 2010/09/14 18:55:45, petersont wrote:
> Change comment.

Done.

http://codereview.chromium.org/3358020/diff/2001/3002#newcode1388
o3djs/effect.js:1388: * @param {object=} opt_options Extra options for
buildStandardShaderString
On 2010/09/14 18:55:45, petersont wrote:
> stray =

Done in other places. Here I used the format {key: valuetype}, in the same way
as the
 @return {{description: string, shader: string}}
in buildShaderString above, so I could document the specific keys.

http://codereview.chromium.org/3358020/diff/2001/3003
File o3djs/material.js (right):

http://codereview.chromium.org/3358020/diff/2001/3003#newcode116
o3djs/material.js:116: * @param {object=} opt_options Extra options for
effect.getStandardShader
On 2010/09/14 18:55:45, petersont wrote:
> Stray =, also should capitalize Object, and it'd be nice to add to the comment
> to say what the possible fields of opt_options are.  On second thought, why
not
> add that comment in effect.getStandardShader and reference it here.

Made all the comments consistent, and document the possible value in
getStandardShader.

[petersont, 2010-09-28 18:01:58]

One little thing, otherwise, LGTM.

http://codereview.chromium.org/3358020/diff/13002/21003
File o3djs/effect.js (right):

http://codereview.chromium.org/3358020/diff/13002/21003#newcode1016
o3djs/effect.js:1016: beginLightLoop = function() {
I think you'll need the var declaration or beginLightLoop and endLightLoop will
wind up in global scope.