Default to using position for texture coordinates

Not nearly as bad as it looked, once I poked it.

There's a little more cleanup that *could* be done, but this is the bulk of it.

[bsalomon, 2012-06-23 00:54:14]

Great, I'm surprised pulling this thread didn't unravel a whole lot more of the
sweater. We haven't yet come fully undone.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrContext.cpp#newcode1575
src/gpu/GrContext.cpp:1575: GrVertexLayout layout = 0;
static const or just pass 0 directly to geo() below?

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (left):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#oldcode361
src/gpu/GrDrawTarget.cpp:361: gen_mask_arrays(stageTexCoordMasks, stageMasks,
texCoordMasks);
We should update gStageMasks (and not remove the below assert). Put

gen_globals();

here.

It will call gen_mask_arrays() and GrPrintf code that you copy/paste at line 77
of this file. It's kind of like a snake eating its tail.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#oldcode891
src/gpu/GrDrawTarget.cpp:891: GrVertexLayout default_blend_opts_vertex_layout()
{
This function is a source of evil but it can't quite be removed yet. 

Sometimes we check for blend optimizations before we have a vertex layout. This
is the "assume all stages could be active if they have a texture" layout that
was used in this scenario.

With your change we no longer need to know the vertex layout to know if a stage
is active. However, we do still need the layout to know about per-vertex colors,
aa-edge-eqs, and per-vertex coverage. We just assume those aren't present if the
blend opts are checked before we have a vtx layout.

Once those non-stagey things are in stages then this can die.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#newcode318
src/gpu/GrDrawTarget.cpp:318: GrAssert(false &&
We have a GrCrash("msg") macro.

However, I think we can just replace the loop contents with:

int tcIdx = VertexTexCoordsForStage(...);
texCoordOffsetsByStage[s] = tcIdx < 0 ? 0 : tcIdx;

We don't really need two tests.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode209
src/gpu/GrDrawTarget.h:209: static int StagePosAsTexCoordVertexLayoutBit(int
stage) {
Shouldn't we remove this?

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode219
src/gpu/GrDrawTarget.h:219: void addToVertexLayout(int flag) {
This should definitely be removed. It is a hack and is only used in one place
which you've removed.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode988
src/gpu/GrDrawTarget.h:988: return NULL != state.getTexture(stage);
Should we remove the layout param, or wait till a future CL?

http://codereview.appspot.com/6337050/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/gl/GrGLProgram.cpp#newco...
src/gpu/gl/GrGLProgram.cpp:595: if (!GrDrawTarget::StageUsesTexCoords(layout,
s)) {
Can we do the same switcheroo here and not call the new function. I.e.:

tcIdx = ...;
if (tcIdx < 0) {
} else {
}

http://codereview.appspot.com/6337050/diff/1/src/gpu/gl/GrGLProgram.cpp#newco...
src/gpu/gl/GrGLProgram.cpp:715: if (!GrDrawTarget::StageUsesTexCoords(layout,
s)) {
ditto

[TomH, 2012-06-25 14:53:11]

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrContext.cpp#newcode1575
src/gpu/GrContext.cpp:1575: GrVertexLayout layout = 0;
On 2012/06/23 00:54:14, bsalomon wrote:
> static const or just pass 0 directly to geo() below?

Done.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (left):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#oldcode361
src/gpu/GrDrawTarget.cpp:361: gen_mask_arrays(stageTexCoordMasks, stageMasks,
texCoordMasks);
On 2012/06/23 00:54:14, bsalomon wrote:
> We should update gStageMasks (and not remove the below assert). Put
> 
> gen_globals();
> 
> here.
> 
> It will call gen_mask_arrays() and GrPrintf code that you copy/paste at line
77
> of this file. It's kind of like a snake eating its tail.

Done.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#oldcode891
src/gpu/GrDrawTarget.cpp:891: GrVertexLayout default_blend_opts_vertex_layout()
{
On 2012/06/23 00:54:14, bsalomon wrote:
> This function is a source of evil but it can't quite be removed yet. 
> 
> Sometimes we check for blend optimizations before we have a vertex layout.
This
> is the "assume all stages could be active if they have a texture" layout that
> was used in this scenario.
> 
> With your change we no longer need to know the vertex layout to know if a
stage
> is active. However, we do still need the layout to know about per-vertex
colors,
> aa-edge-eqs, and per-vertex coverage. We just assume those aren't present if
the
> blend opts are checked before we have a vtx layout.
> 
> Once those non-stagey things are in stages then this can die.

Not sure what change you're asking for? Or is this just documenting future work?

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#newcode318
src/gpu/GrDrawTarget.cpp:318: GrAssert(false &&
On 2012/06/23 00:54:14, bsalomon wrote:
> We have a GrCrash("msg") macro.
> 
> However, I think we can just replace the loop contents with:
> 
> int tcIdx = VertexTexCoordsForStage(...);
> texCoordOffsetsByStage[s] = tcIdx < 0 ? 0 : tcIdx;
> 
> We don't really need two tests.
> 
> 

Needs to be :texCoordOffsetsByIdx[tcIdx], not :tcIdx, or we crash.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode209
src/gpu/GrDrawTarget.h:209: static int StagePosAsTexCoordVertexLayoutBit(int
stage) {
On 2012/06/23 00:54:14, bsalomon wrote:
> Shouldn't we remove this?

Done.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode219
src/gpu/GrDrawTarget.h:219: void addToVertexLayout(int flag) {
On 2012/06/23 00:54:14, bsalomon wrote:
> This should definitely be removed. It is a hack and is only used in one place
> which you've removed.

Done.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode988
src/gpu/GrDrawTarget.h:988: return NULL != state.getTexture(stage);
On 2012/06/23 00:54:14, bsalomon wrote:
> Should we remove the layout param, or wait till a future CL?

Done.

http://codereview.appspot.com/6337050/diff/1/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/gl/GrGLProgram.cpp#newco...
src/gpu/gl/GrGLProgram.cpp:595: if (!GrDrawTarget::StageUsesTexCoords(layout,
s)) {
On 2012/06/23 00:54:14, bsalomon wrote:
> Can we do the same switcheroo here and not call the new function. I.e.:
> 
> tcIdx = ...;
> if (tcIdx < 0) {
> } else {
> }

Done.

http://codereview.appspot.com/6337050/diff/1/src/gpu/gl/GrGLProgram.cpp#newco...
src/gpu/gl/GrGLProgram.cpp:715: if (!GrDrawTarget::StageUsesTexCoords(layout,
s)) {
On 2012/06/23 00:54:14, bsalomon wrote:
> ditto

Done.

[bsalomon, 2012-06-25 15:14:58]

LGTM

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (left):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#oldcode891
src/gpu/GrDrawTarget.cpp:891: GrVertexLayout default_blend_opts_vertex_layout()
{
On 2012/06/25 14:53:11, TomH wrote:
> On 2012/06/23 00:54:14, bsalomon wrote:
> > This function is a source of evil but it can't quite be removed yet. 
> > 
> > Sometimes we check for blend optimizations before we have a vertex layout.
> This
> > is the "assume all stages could be active if they have a texture" layout
that
> > was used in this scenario.
> > 
> > With your change we no longer need to know the vertex layout to know if a
> stage
> > is active. However, we do still need the layout to know about per-vertex
> colors,
> > aa-edge-eqs, and per-vertex coverage. We just assume those aren't present if
> the
> > blend opts are checked before we have a vtx layout.
> > 
> > Once those non-stagey things are in stages then this can die.
> 
> Not sure what change you're asking for? Or is this just documenting future
work?

It doesn't imply anything your change. Just explaining why this exists since I
suspect you'll encounter this again in future changes.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.cpp#newcode318
src/gpu/GrDrawTarget.cpp:318: GrAssert(false &&
On 2012/06/25 14:53:11, TomH wrote:
> On 2012/06/23 00:54:14, bsalomon wrote:
> > We have a GrCrash("msg") macro.
> > 
> > However, I think we can just replace the loop contents with:
> > 
> > int tcIdx = VertexTexCoordsForStage(...);
> > texCoordOffsetsByStage[s] = tcIdx < 0 ? 0 : tcIdx;
> > 
> > We don't really need two tests.
> > 
> > 
> 
> Needs to be :texCoordOffsetsByIdx[tcIdx], not :tcIdx, or we crash.
> 

oh, right.

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode988
src/gpu/GrDrawTarget.h:988: return NULL != state.getTexture(stage);
On 2012/06/25 14:53:11, TomH wrote:
> On 2012/06/23 00:54:14, bsalomon wrote:
> > Should we remove the layout param, or wait till a future CL?
> 
> Done.

Great, I suppose once you change the def of enabled stage in GrDrawState then we
can kill this function and isStageEnabled().

[TomH, 2012-06-25 15:22:27]

Closed with r4321.

[TomH, 2012-06-25 15:31:30]

On 2012/06/25 15:14:58, bsalomon wrote:
> http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h
> File src/gpu/GrDrawTarget.h (right):
> 
> http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode988
> src/gpu/GrDrawTarget.h:988: return NULL != state.getTexture(stage);
> On 2012/06/25 14:53:11, TomH wrote:
> > On 2012/06/23 00:54:14, bsalomon wrote:
> > > Should we remove the layout param, or wait till a future CL?
> > 
> > Done.
> 
> Great, I suppose once you change the def of enabled stage in GrDrawState then
we
> can kill this function and isStageEnabled().

I started doing that in 6306097, but leaving isStageEnabled() there gives us a
little bit of control of coupling: GrGpuGL doesn't need to explicitly get the
draw state and call stageActive() on it.

Apparently the orthodoxy on controlling coupling is to not access the interface
of a class several-pointer-references away, but that's a rule of thumb I've
never been completely happy with.

[bsalomon, 2012-06-25 15:34:59]

On 2012/06/25 15:31:30, TomH wrote:
> On 2012/06/25 15:14:58, bsalomon wrote:
> > http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h
> > File src/gpu/GrDrawTarget.h (right):
> > 
> >
http://codereview.appspot.com/6337050/diff/1/src/gpu/GrDrawTarget.h#newcode988
> > src/gpu/GrDrawTarget.h:988: return NULL != state.getTexture(stage);
> > On 2012/06/25 14:53:11, TomH wrote:
> > > On 2012/06/23 00:54:14, bsalomon wrote:
> > > > Should we remove the layout param, or wait till a future CL?
> > > 
> > > Done.
> > 
> > Great, I suppose once you change the def of enabled stage in GrDrawState
then
> we
> > can kill this function and isStageEnabled().
> 
> I started doing that in 6306097, but leaving isStageEnabled() there gives us a
> little bit of control of coupling: GrGpuGL doesn't need to explicitly get the
> draw state and call stageActive() on it.
> 
> Apparently the orthodoxy on controlling coupling is to not access the
interface
> of a class several-pointer-references away, but that's a rule of thumb I've
> never been completely happy with.

I suppose we could have an inline helper that just forwards the query on to the
current GrDrawState. It would be nice to get rid of the static function, though.