Repurpose GrGLCoordTransform as GrGLEffectArray

Repurpose GrGLCoordTransform as GrGLProgramEffects

Creates a GrGLProgramEffects class that the GrGLProgram uses to manage
an array of effects. This gives us enough abstraction for the program
to cleanly handle different types of coord transforms.

Committed: http://code.google.com/p/skia/source/detail?r=11588

[Chris Dalton, 2013-10-02 19:57:32]

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
File include/gpu/GrTBackendEffectFactory.h (right):

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
include/gpu/GrTBackendEffectFactory.h:39: EffectKey attribKey =
GLEffect::ArrayClass::GenAttribKey(drawEffect);
I'm not sure I'm a huge fan of this 'ArrayClass' approach, but I couldn't think
of anything better.

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:389: fCoverageEffects->initSamplers(fUniformManager,
&texUnitIdx);
I considered making these null when empty, instead of 0-length arrays, but in
the end decided to stick with this approach.

[bsalomon, 2013-10-02 20:17:06]

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
File include/gpu/GrTBackendEffectFactory.h (right):

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
include/gpu/GrTBackendEffectFactory.h:39: EffectKey attribKey =
GLEffect::ArrayClass::GenAttribKey(drawEffect);
On 2013/10/02 19:57:32, Chris Dalton wrote:
> I'm not sure I'm a huge fan of this 'ArrayClass' approach, but I couldn't
think
> of anything better.

Why do we need the typedef and nested scoping anyway? (I see it was like that
before, I just can't remember why).

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLEffectArray.h
File src/gpu/gl/GrGLEffectArray.h (right):

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLEffectArray.h#...
src/gpu/gl/GrGLEffectArray.h:25: class GrGLEffectArray {
I feel like the name should indicate that this is the collection of effects for
a program. And I'm not sure Array needs to be in the name. How about
GrGLProgramEffects?

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLProgram.cpp#ne...
src/gpu/gl/GrGLProgram.cpp:235:
fColorEffects.reset(builder.emitEffects(colorStages,
this is nice

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:193: GrGLEffectArray* emitEffects(const
GrEffectStage* effectStages[],
emitEffectsAndCreateFoo()?

Can we update the comment to make it clear that the caller gets a newly minted
object that it is responsible for deleting.

Also, while you're at it, all the stuff about NULL and kNoEffectKey is stale and
can be deleted.

And the language about glEffects and effectSamplerHandles can be removed.

[Chris Dalton, 2013-10-02 20:24:09]

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
File include/gpu/GrTBackendEffectFactory.h (right):

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
include/gpu/GrTBackendEffectFactory.h:39: EffectKey attribKey =
GLEffect::ArrayClass::GenAttribKey(drawEffect);
On 2013/10/02 20:17:06, bsalomon wrote:
> On 2013/10/02 19:57:32, Chris Dalton wrote:
> > I'm not sure I'm a huge fan of this 'ArrayClass' approach, but I couldn't
> think
> > of anything better.
> 
> Why do we need the typedef and nested scoping anyway? (I see it was like that
> before, I just can't remember why).

This class is a template factory for any Effect. (So GLEffect is like,
GrGLSimpleTextureEffect, or GrGLConical2Gradient.

I thought it was weird to include GrGLEffectArray since this class exists at the
Gr level, but I didn't realize the method we'r in is called 'glEffectKey'. I'm
leaning toward that now -- using GrGLEffectArray directly. Does that sound good
to you?

[bsalomon, 2013-10-02 20:45:47]

On 2013/10/02 20:24:09, Chris Dalton wrote:
>
https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
> File include/gpu/GrTBackendEffectFactory.h (right):
> 
>
https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
> include/gpu/GrTBackendEffectFactory.h:39: EffectKey attribKey =
> GLEffect::ArrayClass::GenAttribKey(drawEffect);
> On 2013/10/02 20:17:06, bsalomon wrote:
> > On 2013/10/02 19:57:32, Chris Dalton wrote:
> > > I'm not sure I'm a huge fan of this 'ArrayClass' approach, but I couldn't
> > think
> > > of anything better.
> > 
> > Why do we need the typedef and nested scoping anyway? (I see it was like
that
> > before, I just can't remember why).
> 
> This class is a template factory for any Effect. (So GLEffect is like,
> GrGLSimpleTextureEffect, or GrGLConical2Gradient.
> 
> I thought it was weird to include GrGLEffectArray since this class exists at
the
> Gr level, but I didn't realize the method we'r in is called 'glEffectKey'. I'm
> leaning toward that now -- using GrGLEffectArray directly. Does that sound
good
> to you?

sgtm

[Chris Dalton, 2013-10-02 21:25:43]

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
File include/gpu/GrTBackendEffectFactory.h (right):

https://codereview.chromium.org/25605008/diff/1/include/gpu/GrTBackendEffectF...
include/gpu/GrTBackendEffectFactory.h:39: EffectKey attribKey =
GLEffect::ArrayClass::GenAttribKey(drawEffect);
On 2013/10/02 20:24:09, Chris Dalton wrote:
> On 2013/10/02 20:17:06, bsalomon wrote:
> > On 2013/10/02 19:57:32, Chris Dalton wrote:
> > > I'm not sure I'm a huge fan of this 'ArrayClass' approach, but I couldn't
> > think
> > > of anything better.
> > 
> > Why do we need the typedef and nested scoping anyway? (I see it was like
that
> > before, I just can't remember why).
> 
> This class is a template factory for any Effect. (So GLEffect is like,
> GrGLSimpleTextureEffect, or GrGLConical2Gradient.
> 
> I thought it was weird to include GrGLEffectArray since this class exists at
the
> Gr level, but I didn't realize the method we'r in is called 'glEffectKey'. I'm
> leaning toward that now -- using GrGLEffectArray directly. Does that sound
good
> to you?

Done.

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLEffectArray.h
File src/gpu/gl/GrGLEffectArray.h (right):

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLEffectArray.h#...
src/gpu/gl/GrGLEffectArray.h:25: class GrGLEffectArray {
On 2013/10/02 20:17:06, bsalomon wrote:
> I feel like the name should indicate that this is the collection of effects
for
> a program. And I'm not sure Array needs to be in the name. How about
> GrGLProgramEffects?

Done.

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/25605008/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:193: GrGLEffectArray* emitEffects(const
GrEffectStage* effectStages[],
On 2013/10/02 20:17:06, bsalomon wrote:
> emitEffectsAndCreateFoo()?
> 
> Can we update the comment to make it clear that the caller gets a newly minted
> object that it is responsible for deleting.
> 
> Also, while you're at it, all the stuff about NULL and kNoEffectKey is stale
and
> can be deleted.
> 
> And the language about glEffects and effectSamplerHandles can be removed.

Done.

[bsalomon, 2013-10-03 14:53:59]

lgtm

[commit-bot: I haz the power, 2013-10-03 14:54:05]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/25605008/44001

[commit-bot: I haz the power, 2013-10-03 15:18:01]

Change committed as 11588