Removing vertex attrib indices

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/249af15fb82833d2274850c589812b6e69df0033

[joshua.litt, 2014-09-07 14:30:50]

joshualitt@chromium.org changed reviewers:
+ bsalomon@google.com

[joshua.litt, 2014-09-07 14:31:28]

joshualitt@chromium.org changed reviewers:
+ egdaniel@google.com

[joshua.litt, 2014-09-07 17:38:56]

This is my first take at removing vertex indices in GrEffect stage/GrEffect.  I
also renamed GrVertexEffect to GrGeometryProcessor.

I didn't change drawstate too much, though the vertex validation in RODrawState
is changed.  Most of the logic is encapsulated in GrVertexShaderBuilder. 
Basically, I get the vertex attribs off the draw state, and loop through looking
for the Effect attribs.  I then bind them in the relative order they are found.

The only problem I ran into was in GlProgramsTest, because now VertexBuilder
depends on a valid vertexattrib array in GrDrawState.  I don't really have an
easy fix because I can't build up the array of vertex attribs at runtime for the
test, but you guys might have other ideas.

I still think we need to do a better job encapsulating all of the vertex
handling code into one place, but I will wait to make that change until the
optimized drawstate code lands.

[bsalomon, 2014-09-08 13:47:53]

For the test could we just read the attrs off the GP and make a consistent
vertex attribute array?

https://codereview.chromium.org/543623004/diff/100001/include/gpu/GrShaderVar.h
File include/gpu/GrShaderVar.h (right):

https://codereview.chromium.org/543623004/diff/100001/include/gpu/GrShaderVar...
include/gpu/GrShaderVar.h:18: * deprecated, but we still need to know whether a
Varying variable
Should this and precision go into the GL class?

[joshua.litt, 2014-09-08 19:48:37]

feedback incorporated

[bsalomon, 2014-09-08 21:17:50]

https://codereview.chromium.org/543623004/diff/140001/include/gpu/GrShaderVar.h
File include/gpu/GrShaderVar.h (right):

https://codereview.chromium.org/543623004/diff/140001/include/gpu/GrShaderVar...
include/gpu/GrShaderVar.h:25: enum TypeModifier {
Maybe longer we should get rid of this enum and move the declaration code to
somewhere in the builder that is context-aware. It's weird that have things like
arrays of uniform vars where each var has individually recorded that it is a
uniform.

https://codereview.chromium.org/543623004/diff/140001/src/gpu/GrAAConvexPathR...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.chromium.org/543623004/diff/140001/src/gpu/GrAAConvexPathR...
src/gpu/GrAAConvexPathRenderer.cpp:507: const GrShaderVar
kAttrQuadEdge("aQuadEdge",
Chrome doesn't allow us to have statically initialized objects.

Maybe we could have the base class track these and return a GrShaderVar*? e.g.

class GrGP {
    GrShaderVar* addAttribute(name, type);
}

GrMyGP::GrMyGP() {
   fRadius = this->addAtribute("radius", kFloat);
}

[joshua.litt, 2014-09-09 02:54:14]

feedback incorporated

[bsalomon, 2014-09-12 00:34:17]

Adding Rob and Jim to the mix as well. Since we will all want to write GPs in
the future I want to make sure everyone is happy with the interface.

https://codereview.chromium.org/543623004/diff/220001/src/gpu/GrAAConvexPathR...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/GrAAConvexPathR...
src/gpu/GrAAConvexPathRenderer.cpp:524: 
I find the 'a' prefix naming a little bit weird. We don't have any similar
conventions that I'm aware of for method names. I'm perfectly happy to be
overruled, but I might be more comfortable with "getQuadEdgeAttr()", and similar
for other attrs.

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrBezie...
File src/gpu/effects/GrBezierEffect.cpp (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrBezie...
src/gpu/effects/GrBezierEffect.cpp:138: : GrGeometryProcessor()
We usually use the INHERITED typedef for the base class. Also, does it work to
omit the explicit base class default cons?

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrBezie...
src/gpu/effects/GrBezierEffect.cpp:211: const GrShaderVar& aHairQuadEdge =
drawEffect.castEffect<GrQuadEffect>().aHairQuadEdge();
similar for the local vars, hairQuadAttr? I don't think we use 'u' for
GrGLShaderVars that are uniforms, 'v' for varyings, etc. Ok to adopt a new
pattern but would rather be consistent.

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
File src/gpu/effects/GrGeometryProcessor.h (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
src/gpu/effects/GrGeometryProcessor.h:31: GrShaderVar& addVertexAttrib(const
GrShaderVar& var) {
I think this should just take a type and a name (and perhaps optional
precision). By allowing the caller to construct a GrShaderVar are we giving them
anything other than an opportunity to mess it up (e.g. specify a type modifier
other than attribute).

Also, does it make sense for it to return a ref, esp a non-const one? We don't
generally use non-const refs. Maybe it can just return by value? Or should it
give a handle (index) that can be used to retrieve the var object from the base
class?

[joshua.litt, 2014-09-12 00:46:07]

https://codereview.chromium.org/543623004/diff/220001/src/gpu/GrAAConvexPathR...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/GrAAConvexPathR...
src/gpu/GrAAConvexPathRenderer.cpp:524: 
On 2014/09/12 00:34:17, bsalomon wrote:
> I find the 'a' prefix naming a little bit weird. We don't have any similar
> conventions that I'm aware of for method names. I'm perfectly happy to be
> overruled, but I might be more comfortable with "getQuadEdgeAttr()", and
similar
> for other attrs.

I was trying to follow the convention we use in the shader code itself, but if
it seems too out of place then I'll change it.  There are places in our code
where we use const getters without the 'get' prefix ala name() instead of
getName(), and I find the former convention much more readable.  How does
quadEdgeAttr() sound?  I can do getQuadEdgeAttr() if we are trying to move
towards the convention of always using get*

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrBezie...
File src/gpu/effects/GrBezierEffect.cpp (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrBezie...
src/gpu/effects/GrBezierEffect.cpp:138: : GrGeometryProcessor()
On 2014/09/12 00:34:17, bsalomon wrote:
> We usually use the INHERITED typedef for the base class. Also, does it work to
> omit the explicit base class default cons?

Acknowledged.

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrBezie...
src/gpu/effects/GrBezierEffect.cpp:211: const GrShaderVar& aHairQuadEdge =
drawEffect.castEffect<GrQuadEffect>().aHairQuadEdge();
On 2014/09/12 00:34:17, bsalomon wrote:
> similar for the local vars, hairQuadAttr? I don't think we use 'u' for
> GrGLShaderVars that are uniforms, 'v' for varyings, etc. Ok to adopt a new
> pattern but would rather be consistent.

Yea, I probably should have discussed this with you guys.  I like the convention
of using the same variable names for shader vars in both GL code and C++.  I
think this makes it very easy to track when moving between shader code and C++. 
I am happy to change this if the team disagrees.

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
File src/gpu/effects/GrGeometryProcessor.h (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
src/gpu/effects/GrGeometryProcessor.h:31: GrShaderVar& addVertexAttrib(const
GrShaderVar& var) {
On 2014/09/12 00:34:17, bsalomon wrote:
> I think this should just take a type and a name (and perhaps optional
> precision). By allowing the caller to construct a GrShaderVar are we giving
them
> anything other than an opportunity to mess it up (e.g. specify a type modifier
> other than attribute).
> 
> Also, does it make sense for it to return a ref, esp a non-const one? We don't
> generally use non-const refs. Maybe it can just return by value? Or should it
> give a handle (index) that can be used to retrieve the var object from the
base
> class?

Non const ref was a mistake, I meant to return a const ref.  I can also just
return a handle.  My intent is to remove typemodifiers from GrShaderVar, so I'd
rather use it here and clean up GrShaderVar a few cls down the road.  Is that
okay?

[egdaniel, 2014-09-12 01:12:27]

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
File src/gpu/effects/GrGeometryProcessor.h (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
src/gpu/effects/GrGeometryProcessor.h:31: GrShaderVar& addVertexAttrib(const
GrShaderVar& var) {
Do we want to be dealing precision issues at this level? The VS should always be
defaulted to high so no need and currently (as in the bezier effect) we add the
precision level of attributes in FS when emitting the code. I can see an
argument for either, but part of me feels it is easier to deal with the
precision when it is with the rest of the shader code in emit.

Or is the idea to remove the need to manually add the addVarrying(...) calls in
the shaders?

On 2014/09/12 00:34:17, bsalomon wrote:
> I think this should just take a type and a name (and perhaps optional
> precision). By allowing the caller to construct a GrShaderVar are we giving
them
> anything other than an opportunity to mess it up (e.g. specify a type modifier
> other than attribute).
> 
> Also, does it make sense for it to return a ref, esp a non-const one? We don't
> generally use non-const refs. Maybe it can just return by value? Or should it
> give a handle (index) that can be used to retrieve the var object from the
base
> class?

[bsalomon, 2014-09-12 01:17:47]

https://codereview.chromium.org/543623004/diff/220001/src/gpu/GrAAConvexPathR...
File src/gpu/GrAAConvexPathRenderer.cpp (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/GrAAConvexPathR...
src/gpu/GrAAConvexPathRenderer.cpp:524: 
On 2014/09/12 00:46:06, joshua.litt wrote:
> On 2014/09/12 00:34:17, bsalomon wrote:
> > I find the 'a' prefix naming a little bit weird. We don't have any similar
> > conventions that I'm aware of for method names. I'm perfectly happy to be
> > overruled, but I might be more comfortable with "getQuadEdgeAttr()", and
> similar
> > for other attrs.
> 
> I was trying to follow the convention we use in the shader code itself, but if
> it seems too out of place then I'll change it.  There are places in our code
> where we use const getters without the 'get' prefix ala name() instead of
> getName(), and I find the former convention much more readable.  How does
> quadEdgeAttr() sound?  I can do getQuadEdgeAttr() if we are trying to move
> towards the convention of always using get*


I don't have a strong opinion about w/ or w/o "get". I agree that we're not
consistent.

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
File src/gpu/effects/GrGeometryProcessor.h (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
src/gpu/effects/GrGeometryProcessor.h:31: GrShaderVar& addVertexAttrib(const
GrShaderVar& var) {
On 2014/09/12 01:12:27, egdaniel wrote:
> Do we want to be dealing precision issues at this level? The VS should always
be
> defaulted to high so no need and currently (as in the bezier effect) we add
the
> precision level of attributes in FS when emitting the code. I can see an
> argument for either, but part of me feels it is easier to deal with the
> precision when it is with the rest of the shader code in emit.
> 
> Or is the idea to remove the need to manually add the addVarrying(...) calls
in
> the shaders?
> 

I think you're right that we don't need precision here. I was confusing adding
attribs with adding varyings. That's good because we really only want to worry
about the precision in backend-specific code (GrGL not Gr).

[joshua.litt, 2014-09-12 01:35:29]

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
File src/gpu/effects/GrGeometryProcessor.h (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
src/gpu/effects/GrGeometryProcessor.h:31: GrShaderVar& addVertexAttrib(const
GrShaderVar& var) {
I can remove precision from GrShaderVar along with type modifier.  I'd really
rather not just pass around a name and type simply because I hope to build more
intelligence into code emitting some day, ie array indexing simply by
overloading '[]', type checking and swizzling, etc.

Would everyone be happy with using GrShaderVar if the ref was const and there
was no precision / type modifier on GrShaderVar?

If GrShaderVar is just a name and type, would that make everyone happy?

On 2014/09/12 01:17:47, bsalomon wrote:
> On 2014/09/12 01:12:27, egdaniel wrote:
> > Do we want to be dealing precision issues at this level? The VS should
always
> be
> > defaulted to high so no need and currently (as in the bezier effect) we add
> the
> > precision level of attributes in FS when emitting the code. I can see an
> > argument for either, but part of me feels it is easier to deal with the
> > precision when it is with the rest of the shader code in emit.
> > 
> > Or is the idea to remove the need to manually add the addVarrying(...) calls
> in
> > the shaders?
> > 
> 
> I think you're right that we don't need precision here. I was confusing adding
> attribs with adding varyings. That's good because we really only want to worry
> about the precision in backend-specific code (GrGL not Gr).

[joshua.litt, 2014-09-12 01:38:24]

joshualitt@chromium.org changed reviewers:
+ jvanverth@google.com, robertphillips@google.com

[egdaniel, 2014-09-12 02:09:19]

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
File src/gpu/effects/GrGeometryProcessor.h (right):

https://codereview.chromium.org/543623004/diff/220001/src/gpu/effects/GrGeome...
src/gpu/effects/GrGeometryProcessor.h:31: GrShaderVar& addVertexAttrib(const
GrShaderVar& var) {
I was suggesting removing precision from GrShaderVar, I was just wondering if we
really want it to be a parameter in addVertexAttrib and if so where we would use
it exactly.

On 2014/09/12 01:35:28, joshua.litt wrote:
> I can remove precision from GrShaderVar along with type modifier.  I'd really
> rather not just pass around a name and type simply because I hope to build
more
> intelligence into code emitting some day, ie array indexing simply by
> overloading '[]', type checking and swizzling, etc.
> 
> Would everyone be happy with using GrShaderVar if the ref was const and there
> was no precision / type modifier on GrShaderVar?
> 
> If GrShaderVar is just a name and type, would that make everyone happy?
> 
> On 2014/09/12 01:17:47, bsalomon wrote:
> > On 2014/09/12 01:12:27, egdaniel wrote:
> > > Do we want to be dealing precision issues at this level? The VS should
> always
> > be
> > > defaulted to high so no need and currently (as in the bezier effect) we
add
> > the
> > > precision level of attributes in FS when emitting the code. I can see an
> > > argument for either, but part of me feels it is easier to deal with the
> > > precision when it is with the rest of the shader code in emit.
> > > 
> > > Or is the idea to remove the need to manually add the addVarrying(...)
calls
> > in
> > > the shaders?
> > > 
> > 
> > I think you're right that we don't need precision here. I was confusing
adding
> > attribs with adding varyings. That's good because we really only want to
worry
> > about the precision in backend-specific code (GrGL not Gr).
>

[joshua.litt, 2014-09-15 16:56:04]

feedback incorporated

[bsalomon, 2014-09-15 17:46:25]

lgtm

https://codereview.chromium.org/543623004/diff/280001/src/gpu/GrAARectRendere...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/543623004/diff/280001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:64: vsBuilder->codeAppendf("\t%s = %s;\n",
vsRectName, inRect.c_str());
happy about the result of our discussion earlier today, this will look nice
nicer as copying inRect to outRect.

[joshua.litt, 2014-09-15 18:08:43]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-15 18:09:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543623004/280001

[commit-bot: I haz the power, 2014-09-15 18:18:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 18:18:39]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[joshua.litt, 2014-09-15 18:19:48]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-15 18:20:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543623004/300001

[commit-bot: I haz the power, 2014-09-15 18:29:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 18:29:11]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[joshua.litt, 2014-09-15 18:33:20]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-15 18:34:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543623004/320001

[commit-bot: I haz the power, 2014-09-15 18:41:18]

Committed patchset #17 (id:320001) as 249af15fb82833d2274850c589812b6e69df0033