Removing vertex attrib indices

src/gpu/effects/GrGeometryProcessor.h

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
-#ifndef GrVertexEffect_DEFINED
-#define GrVertexEffect_DEFINED
+#ifndef GrGeometryProcessor_DEFINED
+#define GrGeometryProcessor_DEFINED
 
 #include "GrEffect.h"
 
 /**
  * If an effect needs specialized vertex shader code, then it must inherit from this class.
  * Otherwise it won't be able to add vertex attribs, and it might be given a vertexless shader
  * program in emitCode.
  */
-class GrVertexEffect : public GrEffect {
+class GrGeometryProcessor : public GrEffect {
 public:
-    GrVertexEffect() { fRequiresVertexShader = true; }
+    GrGeometryProcessor() { fRequiresVertexShader = true; }
 
 protected:
     /**
      * Subclasses call this from their constructor to register vertex attributes (at most
      * kMaxVertexAttribs). This must only be called from the constructor because GrEffects are
      * immutable.
+     *
+     * We return a reference to the added var so that derived classes can name it nicely and use it
+     * in shader code.
      */
-    void addVertexAttrib(GrSLType type) {
-        SkASSERT(fVertexAttribTypes.count() < kMaxVertexAttribs);
-        fVertexAttribTypes.push_back(type);
+    GrShaderVar& addVertexAttrib(const GrShaderVar& var) {

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

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]

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]

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:

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

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:28]

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:

[egdaniel, 2014/09/12 02:09:18]

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:

+        SkASSERT(fVertexAttribs.count() < kMaxVertexAttribs);
+        return fVertexAttribs.push_back(var);
     }
 
 private:
     typedef GrEffect INHERITED;
 };
 
 #endif