o3d-webgl: convolution shader and error texture.

Implemented error texture support in client and ParamArray class. Fixed bug in effect.js that broke checkerboard texture if running using CG shaders.
TODO: Still a bug with render surface sets; will file issue and tackle problem in separate CL.

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

[luchen, 2010-06-21 21:48:37]

http://codereview.chromium.org/2803007/diff/31001/32005
File samples/o3d-webgl/effect.js (left):

http://codereview.chromium.org/2803007/diff/31001/32005#oldcode557
samples/o3d-webgl/effect.js:557: 
The diff shows the empty lines at the bottom being removed, but my working copy
has one empty line at bottom (it doesn't seem to be preserved when uploading
CL). Is this a property/setting I need to enable?

http://codereview.chromium.org/2803007/diff/31001/32007
File samples/o3d-webgl/param_array.js (right):

http://codereview.chromium.org/2803007/diff/31001/32007#newcode1
samples/o3d-webgl/param_array.js:1: /*
I'm not sure what this svn:eol-style +LF thing is: good, bad?

[petersont, 2010-07-12 18:47:57]

LGTM, save one tiny thing.

http://codereview.chromium.org/2803007/diff/31001/32005
File samples/o3d-webgl/effect.js (left):

http://codereview.chromium.org/2803007/diff/31001/32005#oldcode557
samples/o3d-webgl/effect.js:557: 
Shrug.

On 2010/06/21 21:48:37, luchen wrote:
> The diff shows the empty lines at the bottom being removed, but my working
copy
> has one empty line at bottom (it doesn't seem to be preserved when uploading
> CL). Is this a property/setting I need to enable?

http://codereview.chromium.org/2803007/diff/31001/32007
File samples/o3d-webgl/param_array.js (right):

http://codereview.chromium.org/2803007/diff/31001/32007#newcode1
samples/o3d-webgl/param_array.js:1: /*
On 2010/06/21 21:48:37, luchen wrote:
> I'm not sure what this svn:eol-style +LF thing is: good, bad?

Done.

http://codereview.chromium.org/2803007/diff/43001/44001
File samples/o3d-webgl-samples/convolution.html (right):

http://codereview.chromium.org/2803007/diff/43001/44001#newcode193
samples/o3d-webgl-samples/convolution.html:193: loadScene(g_pack,
'../assets/teapot/scene.json', g_teapotRoot, teapotViewInfo);
80 chars

http://codereview.chromium.org/2803007/diff/43001/44005
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/43001/44005#newcode371
samples/o3d-webgl/effect.js:371: o3d.Effect.paramTypes_[-1] = 'ParamParamArray';
Recommend defining a constant instead of using -1, just because then, if you
search for all the occurrences of the constant you can see better how the hack
works.

[luchen, 2010-07-12 20:02:49]

http://codereview.chromium.org/2803007/diff/43001/44001
File samples/o3d-webgl-samples/convolution.html (right):

http://codereview.chromium.org/2803007/diff/43001/44001#newcode193
samples/o3d-webgl-samples/convolution.html:193: loadScene(g_pack,
'../assets/teapot/scene.json', g_teapotRoot, teapotViewInfo);
On 2010/07/12 18:47:58, petersont wrote:
> 80 chars

Done.

http://codereview.chromium.org/2803007/diff/43001/44005
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/43001/44005#newcode371
samples/o3d-webgl/effect.js:371: o3d.Effect.paramTypes_[-1] = 'ParamParamArray';
On 2010/07/12 18:47:58, petersont wrote:
> Recommend defining a constant instead of using -1, just because then, if you
> search for all the occurrences of the constant you can see better how the hack
> works.

Done.

[petersont, 2010-07-13 01:00:11]

I have an idea how to make it feel a little less like a hack.  What do you
think?

http://codereview.chromium.org/2803007/diff/62001/63005
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/62001/63005#newcode311
samples/o3d-webgl/effect.js:311: location: locations /* This is an _array_ of
locations! */
So, I've rethought this a little.  What if you refactored in this way: get rid
of o3d.Effect.PARAM_ARRAY and instead, add a field to the object literal here
that says whether the uniform represented is an array or a single element, you
could do that with an enum.  Something like:

o3d.Effect.ELEMENT = 0;  // Or a less overused word than Element if you can
think of one.
o3d.Effect.ARRAY = 1;
o3d.Effect.STRUCT = 2;  // Maybe someday.

Then, here, you do this:

this.uniforms_[baseName] = {type: o3d.Effect.ARRAY, locations: locations}

and in the else clause below:

this.uniforms_[info.name] = {type: o3d.Effect.ELEMENT, info: info, location:
this.gl.getUniformLocation(this.program_, info.name)}

Then add the check for the type variable to searchForParmas_  (see the comment
in that function below...)

http://codereview.chromium.org/2803007/diff/62001/63005#newcode523
samples/o3d-webgl/effect.js:523: param.applyToLocation(this.gl,
uniformInfo.location);
... right, so what I'm thinking is: here is where you check for the type, and if
it's just a single element and not an array, do exactly what's written here, but
if it's an array, call the applyToLocation function you wrote, pass in locations
instead of of location... and maybe you could rename that function
applyToLocations or applyToLocationArray or something so that it's more apparent
that it takes an array as the second argument.

http://codereview.chromium.org/2803007/diff/62001/63006
File samples/o3d-webgl/param.js (right):

http://codereview.chromium.org/2803007/diff/62001/63006#newcode839
samples/o3d-webgl/param.js:839: * @param {WebGLUniformLocation} location The
location to which to apply.
I think this should be {!Array.<!WebGLUniformLocation>}

http://codereview.chromium.org/2803007/diff/62001/63006#newcode841
samples/o3d-webgl/param.js:841: o3d.ParamParamArray.prototype.applyToLocation =
function(gl, locationArr) {
I'd kinda prefer if locationArr were renamed either "locations" or
"locationArray".

http://codereview.chromium.org/2803007/diff/62001/63006#newcode843
samples/o3d-webgl/param.js:843: throw 'Invalid uniform param array: incorrect
number of elements.';
Should this appeal to the client's error message function?  I think gl has a
reference to it's client.

[petersont, 2010-07-13 01:00:22]

I have an idea how to make it feel a little less like a hack.  What do you
think?

[luchen, 2010-07-13 03:57:22]

Still feels hacky, but I think this is better. :)
ptal.

http://codereview.chromium.org/2803007/diff/62001/63005
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/62001/63005#newcode311
samples/o3d-webgl/effect.js:311: location: locations /* This is an _array_ of
locations! */
Did something along the lines of your suggestion. The object literal is a little
more complicated that you proposed; it keeps an 'info' field that points to the
original data fetched from the program (code elsewhere relies on this to get the
data size).

If the nested object is too confusing, another option would be to unfold the
properties of 'info' directly alongside 'kind' and 'locations', so you would do
this.uniforms_[paramName].size instead of this.uniforms_[paramName].info.size

On 2010/07/13 01:00:16, petersont wrote:
> So, I've rethought this a little.  What if you refactored in this way: get rid
> of o3d.Effect.PARAM_ARRAY and instead, add a field to the object literal here
> that says whether the uniform represented is an array or a single element, you
> could do that with an enum.  Something like:
> 
> o3d.Effect.ELEMENT = 0;  // Or a less overused word than Element if you can
> think of one.
> o3d.Effect.ARRAY = 1;
> o3d.Effect.STRUCT = 2;  // Maybe someday.
> 
> Then, here, you do this:
> 
> this.uniforms_[baseName] = {type: o3d.Effect.ARRAY, locations: locations}
> 
> and in the else clause below:
> 
> this.uniforms_[info.name] = {type: o3d.Effect.ELEMENT, info: info, location:
> this.gl.getUniformLocation(this.program_, info.name)}
> 
> Then add the check for the type variable to searchForParmas_  (see the comment
> in that function below...

http://codereview.chromium.org/2803007/diff/62001/63006
File samples/o3d-webgl/param.js (right):

http://codereview.chromium.org/2803007/diff/62001/63006#newcode839
samples/o3d-webgl/param.js:839: * @param {WebGLUniformLocation} location The
location to which to apply.
On 2010/07/13 01:00:16, petersont wrote:
> I think this should be {!Array.<!WebGLUniformLocation>}

Done.

http://codereview.chromium.org/2803007/diff/62001/63006#newcode841
samples/o3d-webgl/param.js:841: o3d.ParamParamArray.prototype.applyToLocation =
function(gl, locationArr) {
On 2010/07/13 01:00:16, petersont wrote:
> I'd kinda prefer if locationArr were renamed either "locations" or
> "locationArray".

Done.

http://codereview.chromium.org/2803007/diff/62001/63006#newcode843
samples/o3d-webgl/param.js:843: throw 'Invalid uniform param array: incorrect
number of elements.';
On 2010/07/13 01:00:16, petersont wrote:
> Should this appeal to the client's error message function?  I think gl has a
> reference to it's client.

Done.

[petersont, 2010-07-13 20:22:56]

LGTM.

[luchen, 2010-07-13 21:55:25]

Could you glance again at effect.js (patch 17 vs 18)? Fixed a bug that I
mistakenly introduced in getParameterInfo -- had it passing the wrong
numElements/className to EffectParameterInfo constructor.


On 2010/07/13 20:22:56, petersont wrote:
> LGTM.

[petersont, 2010-07-13 23:06:12]

http://codereview.chromium.org/2803007/diff/63011/81005
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/63011/81005#newcode308
samples/o3d-webgl/effect.js:308: info.name = baseName;
Unless I'm mistaken, info is a WebGLActiveInfo and all it's elements are
read-only.

http://codereview.chromium.org/2803007/diff/63011/81005#newcode487
samples/o3d-webgl/effect.js:487: name, dataType, numElements, semantic,
sasClassName));
So, now I'm looking at o3d.EffectParameterInfo, and I think the semantic is
supposed to be a string.  I must have made it a o3d.Stream.Semantic by
mistake... silly me.  I'm not sure why it's a string, that seems like the wrong
choice to me, but there is this pretense of copying the plugin right or wrong,
so  could you do me a favor and "fix" that while you're at it?

[luchen, 2010-07-13 23:34:28]

http://codereview.chromium.org/2803007/diff/63011/81005
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/63011/81005#newcode308
samples/o3d-webgl/effect.js:308: info.name = baseName;
On 2010/07/13 23:06:12, petersont wrote:
> Unless I'm mistaken, info is a WebGLActiveInfo and all it's elements are
> read-only.

Good call. Made a copy of object and overwrote name there instead.

http://codereview.chromium.org/2803007/diff/63011/81005#newcode487
samples/o3d-webgl/effect.js:487: name, dataType, numElements, semantic,
sasClassName));
On 2010/07/13 23:06:12, petersont wrote:
> So, now I'm looking at o3d.EffectParameterInfo, and I think the semantic is
> supposed to be a string.  I must have made it a o3d.Stream.Semantic by
> mistake... silly me.  I'm not sure why it's a string, that seems like the
wrong
> choice to me, but there is this pretense of copying the plugin right or wrong,
> so  could you do me a favor and "fix" that while you're at it?

Done. Is this correct: the string should be 'position', 'normal', etc if it is a
semantic?

[petersont, 2010-07-14 00:54:39]

http://codereview.chromium.org/2803007/diff/94001/90006
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/94001/90006#newcode65
samples/o3d-webgl/effect.js:65: * @type {string}
Well, I think it _is_ supposed to be upper case.

http://codereview.chromium.org/2803007/diff/94001/90006#newcode485
samples/o3d-webgl/effect.js:485: name, dataType, numElements, semantic,
sasClassName));
semantic.toUpperCase()  I guess.

[luchen, 2010-07-14 01:06:37]

http://codereview.chromium.org/2803007/diff/94001/90006
File samples/o3d-webgl/effect.js (right):

http://codereview.chromium.org/2803007/diff/94001/90006#newcode65
samples/o3d-webgl/effect.js:65: * @type {string}
On 2010/07/14 00:54:40, petersont wrote:
> Well, I think it _is_ supposed to be upper case.

Done.

http://codereview.chromium.org/2803007/diff/94001/90006#newcode485
samples/o3d-webgl/effect.js:485: name, dataType, numElements, semantic,
sasClassName));
On 2010/07/14 00:54:40, petersont wrote:
> semantic.toUpperCase()  I guess.

Done.

[petersont, 2010-07-14 01:10:10]

LGTM.

On 2010/07/14 01:06:37, luchen wrote:
> http://codereview.chromium.org/2803007/diff/94001/90006
> File samples/o3d-webgl/effect.js (right):
> 
> http://codereview.chromium.org/2803007/diff/94001/90006#newcode65
> samples/o3d-webgl/effect.js:65: * @type {string}
> On 2010/07/14 00:54:40, petersont wrote:
> > Well, I think it _is_ supposed to be upper case.
> 
> Done.
> 
> http://codereview.chromium.org/2803007/diff/94001/90006#newcode485
> samples/o3d-webgl/effect.js:485: name, dataType, numElements, semantic,
> sasClassName));
> On 2010/07/14 00:54:40, petersont wrote:
> > semantic.toUpperCase()  I guess.
> 
> Done.