Remove unused texture coordinate flags

Remove unused texture coordinate flags.

Currently we support 5 texture stages, each with 5 possible texture coordinate attributes. However, we only ever use one explicit texture coordinate. This change removes all but one (now named just "aTexCoord") of the possible explicit texture coordinates.

[bsalomon, 2013-02-12 21:24:11]

It's awesome to see some of this complexity go away.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode99
src/gpu/GrDrawState.cpp:99: 0x01,
Maybe we can get of the need for this global array entirely (and the generating
function) since it is now just (1 << index)?

I think all occurrences of gStageTexCoordMasks[index] could be replaced by
StageTexCoordVertexLayoutBit(index).

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode109
src/gpu/GrDrawState.cpp:109: int num_tex_coords(GrVertexLayout layout) {
return (gTexCoordMask & layout) ? 1 : 0;

or maybe make the return a bool and rename to has_tex_coords?

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h#newcode117
src/gpu/GrDrawState.h:117: * and per-vertex coverage. Each stage can use any
texture coordinates as
Stages that require texture coordinates can either use the explicit textures
coordinates or positions. (?)

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h#newcode124
src/gpu/GrDrawState.h:124: * The order in memory is always (position, texture
coords, color, coverage)
coverage, edge)

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h#newcode144
src/gpu/GrDrawState.h:144: static const int STAGE_BIT_CNT = kNumStages;
Should we put the stage-uses-explicit-coords bits last so that we don't need
this private constant?

[robertphillips, 2013-02-12 21:24:14]

LGTM + questions/suggestions

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode77
src/gpu/GrDrawState.cpp:77: 
Shouldn't this be turned off?

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode108
src/gpu/GrDrawState.cpp:108: 
Couldn't this be a one liner?

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode249
src/gpu/GrDrawState.cpp:249: int size = VertexSizeAndOffsets(vertexLayout,
Why not just always pass &texCoordOffset in?

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode267
src/gpu/GrDrawState.cpp:267: bool
GrDrawState::VertexUsesTexCoords(GrVertexLayout vertexLayout) {
ToBool?

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

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawTarget.h#newcode407
src/gpu/GrDrawTarget.h:407: * @param matrix    optional matrix applied to rect
(before viewMatrix)
srcRect

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrSWMaskHelper.cpp#newc...
src/gpu/GrSWMaskHelper.cpp:210: SK_Scalar1 * rect.fBottom);
can we pass in a "const GrRect&" now?

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

https://codereview.appspot.com/7308094/diff/1/src/gpu/gl/GrGLProgram.cpp#newc...
src/gpu/gl/GrGLProgram.cpp:581: // add texture coordinates that are used to the
list of vertex attr decls
Do we even need texCoordAttr?

[JimVV, 2013-02-12 21:28:37]

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode109
src/gpu/GrDrawState.cpp:109: int num_tex_coords(GrVertexLayout layout) {
Having it act like it's returning a number makes the code cleaner in a few
places, even if the number is 0 or 1. E.g. see the offset routines below.

On 2013/02/12 21:24:11, bsalomon wrote:
> return (gTexCoordMask & layout) ? 1 : 0;
> 
> or maybe make the return a bool and rename to has_tex_coords?

[bsalomon, 2013-02-12 21:29:16]

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode109
src/gpu/GrDrawState.cpp:109: int num_tex_coords(GrVertexLayout layout) {
On 2013/02/12 21:28:38, JimVV wrote:
> Having it act like it's returning a number makes the code cleaner in a few
> places, even if the number is 0 or 1. E.g. see the offset routines below.
> 
> On 2013/02/12 21:24:11, bsalomon wrote:
> > return (gTexCoordMask & layout) ? 1 : 0;
> > 
> > or maybe make the return a bool and rename to has_tex_coords?
> 

gotcha

[JimVV, 2013-02-13 19:17:25]

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode77
src/gpu/GrDrawState.cpp:77: 
On 2013/02/12 21:24:14, robertphillips wrote:
> Shouldn't this be turned off?

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode99
src/gpu/GrDrawState.cpp:99: 0x01,
On 2013/02/12 21:24:11, bsalomon wrote:
> Maybe we can get of the need for this global array entirely (and the
generating
> function) since it is now just (1 << index)?
> 
> I think all occurrences of gStageTexCoordMasks[index] could be replaced by
> StageTexCoordVertexLayoutBit(index).

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode108
src/gpu/GrDrawState.cpp:108: 
On 2013/02/12 21:24:14, robertphillips wrote:
> Couldn't this be a one liner?

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode109
src/gpu/GrDrawState.cpp:109: int num_tex_coords(GrVertexLayout layout) {
On 2013/02/12 21:24:11, bsalomon wrote:
> return (gTexCoordMask & layout) ? 1 : 0;
> 
> or maybe make the return a bool and rename to has_tex_coords?

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode249
src/gpu/GrDrawState.cpp:249: int size = VertexSizeAndOffsets(vertexLayout,
On 2013/02/12 21:24:14, robertphillips wrote:
> Why not just always pass &texCoordOffset in?

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.cpp#newcode267
src/gpu/GrDrawState.cpp:267: bool
GrDrawState::VertexUsesTexCoords(GrVertexLayout vertexLayout) {
On 2013/02/12 21:24:14, robertphillips wrote:
> ToBool?

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h#newcode117
src/gpu/GrDrawState.h:117: * and per-vertex coverage. Each stage can use any
texture coordinates as
On 2013/02/12 21:24:11, bsalomon wrote:
> Stages that require texture coordinates can either use the explicit textures
> coordinates or positions. (?)

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h#newcode124
src/gpu/GrDrawState.h:124: * The order in memory is always (position, texture
coords, color, coverage)
On 2013/02/12 21:24:11, bsalomon wrote:
> coverage, edge)

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h#newcode144
src/gpu/GrDrawState.h:144: static const int STAGE_BIT_CNT = kNumStages;
On 2013/02/12 21:24:11, bsalomon wrote:
> Should we put the stage-uses-explicit-coords bits last so that we don't need
> this private constant?

Then we'd need a private constant to indicate the shift for the explicit-coords
bits... I don't see the difference.

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

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawTarget.h#newcode407
src/gpu/GrDrawTarget.h:407: * @param matrix    optional matrix applied to rect
(before viewMatrix)
On 2013/02/12 21:24:14, robertphillips wrote:
> srcRect

Done.

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrSWMaskHelper.cpp#newc...
src/gpu/GrSWMaskHelper.cpp:210: SK_Scalar1 * rect.fBottom);
On 2013/02/12 21:24:14, robertphillips wrote:
> can we pass in a "const GrRect&" now?

The srcRect is still optional, so it has to be passed by pointer.

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

https://codereview.appspot.com/7308094/diff/1/src/gpu/gl/GrGLProgram.cpp#newc...
src/gpu/gl/GrGLProgram.cpp:581: // add texture coordinates that are used to the
list of vertex attr decls
On 2013/02/12 21:24:14, robertphillips wrote:
> Do we even need texCoordAttr?

Removed.

[TomH, 2013-02-13 19:19:13]

On 2013/02/12 21:24:11, bsalomon wrote:
> It's awesome to see some of this complexity go away.

Ditto!

[bsalomon, 2013-02-13 19:38:35]

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.appspot.com/7308094/diff/1/src/gpu/GrDrawState.h#newcode144
src/gpu/GrDrawState.h:144: static const int STAGE_BIT_CNT = kNumStages;
On 2013/02/13 19:17:25, JimVV wrote:
> On 2013/02/12 21:24:11, bsalomon wrote:
> > Should we put the stage-uses-explicit-coords bits last so that we don't need
> > this private constant?
> 
> Then we'd need a private constant to indicate the shift for the
explicit-coords
> bits... I don't see the difference.

Ok, I suppose we will get rid of this in time anyway.

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.cpp#newc...
src/gpu/GrDrawState.cpp:86: const GrVertexLayout gTexCoordMask = 0x1f;
Can we do:

static const kTexCoordMask = ((1 << GrDrawState::kNumStages) - 1);

and delete gen_globals?

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.h#newcod...
src/gpu/GrDrawState.h:118: * can either use the explicit coordinates or can use
positions. If no
The sentence "If no text..." no longer applies. There used to be a bit to
indicate that positions should be used and now using positions is the default if
the explicit coords bit isn't set. (It was wrong before your change but while
you're in here...). My request is to simply remove the sentence.

[JimVV, 2013-02-13 22:16:56]

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.cpp#newc...
src/gpu/GrDrawState.cpp:86: const GrVertexLayout gTexCoordMask = 0x1f;
On 2013/02/13 19:38:35, bsalomon wrote:
> Can we do:
> 
> static const kTexCoordMask = ((1 << GrDrawState::kNumStages) - 1);
> 
> and delete gen_globals?

Done.

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.h#newcod...
src/gpu/GrDrawState.h:118: * can either use the explicit coordinates or can use
positions. If no
On 2013/02/13 19:38:35, bsalomon wrote:
> The sentence "If no text..." no longer applies. There used to be a bit to
> indicate that positions should be used and now using positions is the default
if
> the explicit coords bit isn't set. (It was wrong before your change but while
> you're in here...). My request is to simply remove the sentence.

Done.

[bsalomon, 2013-02-14 00:31:31]

On 2013/02/13 22:16:56, JimVV wrote:
> https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.cpp
> File src/gpu/GrDrawState.cpp (right):
> 
>
https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.cpp#newc...
> src/gpu/GrDrawState.cpp:86: const GrVertexLayout gTexCoordMask = 0x1f;
> On 2013/02/13 19:38:35, bsalomon wrote:
> > Can we do:
> > 
> > static const kTexCoordMask = ((1 << GrDrawState::kNumStages) - 1);
> > 
> > and delete gen_globals?
> 
> Done.
> 
> https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.h
> File src/gpu/GrDrawState.h (right):
> 
>
https://codereview.appspot.com/7308094/diff/8001/src/gpu/GrDrawState.h#newcod...
> src/gpu/GrDrawState.h:118: * can either use the explicit coordinates or can
use
> positions. If no
> On 2013/02/13 19:38:35, bsalomon wrote:
> > The sentence "If no text..." no longer applies. There used to be a bit to
> > indicate that positions should be used and now using positions is the
default
> if
> > the explicit coords bit isn't set. (It was wrong before your change but
while
> > you're in here...). My request is to simply remove the sentence.
> 
> Done.

lgtm

[JimVV, 2013-02-14 15:26:53]

Committed as r7737.