Remove stage masks

[bsalomon, 2012-07-19 14:13:35]

To go where we want to go with the shader generator / stages we need to get rid
of GrVertexLayout and his slightly less evil cousin GrDrawState::StageMask. This
change removes the stage masks.

StageMasks were commonly used to preconcat sampler matrices when the view matrix
changes. The sampler matrices have to be updated when they operate on vertex
positions. However, there are a couple places where we use explicit texture
coords. In that case we *don't* want to preconcat the sampler matrix. This
should get cleaner when knowledge of the matrix / explicit coords moves to the
stage. Rather than  having the drawstate client directly update the sampler
matrix we will have a notification mechanism invoked when the view matrix
changes. Stages will be responsible for updating their matrix if necessary.

[TomH, 2012-07-19 18:55:09]

Looking good so far.

http://codereview.appspot.com/6422047/diff/2001/include/gpu/GrPaint.h
File include/gpu/GrPaint.h (right):

http://codereview.appspot.com/6422047/diff/2001/include/gpu/GrPaint.h#newcode128
include/gpu/GrPaint.h:128: * one enabled stage) then false is returned.
Do we have this circumlocution just to avoid inverting the matrix when there are
no active stages? Is that at all common?

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

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode634
src/gpu/GrContext.cpp:634: p = tmpPaint.get();
This two-step is an odd idiom, but I see it being used multiple times. Why?
Would it be clearer with a named helper function? Or is scoping too ugly then?

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode800
src/gpu/GrContext.cpp:800: GrDrawTarget::AutoReleaseGeometry geo(target, 0,
worstCaseVertCount, 0);
Could we get rid of the second argument now?

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode842
src/gpu/GrContext.cpp:842: target->setVertexSourceToBuffer(0, sqVB);
First argument obsolete???

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode894
src/gpu/GrContext.cpp:894: drawState->preConcatSamplerMatrices(m);
So previously we only preconcatted the sampler matrices with stages after the
first. Now we're also operating on the first stage. Behavior change? Or lying
code?

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1065: GrDrawTarget::AutoDeviceCoordDraw adcd(target);
Despite my sheltered upbringing, I want to read your variable name as acdc.

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrDrawState.h#newcode300
src/gpu/GrDrawState.h:300: bool preConcatSamplerMatricesWithInverse(const
GrMatrix& matrix) {
We have two very similar functions - thanks to levelization?

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

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrDrawTarget.cpp#oldc...
src/gpu/GrDrawTarget.cpp:1172: // sad trombone sound
I regret the timely demise of this comment.

[bsalomon, 2012-07-19 19:55:02]

http://codereview.appspot.com/6422047/diff/2001/include/gpu/GrPaint.h
File include/gpu/GrPaint.h (right):

http://codereview.appspot.com/6422047/diff/2001/include/gpu/GrPaint.h#newcode128
include/gpu/GrPaint.h:128: * one enabled stage) then false is returned.
On 2012/07/19 18:55:09, TomH wrote:
> Do we have this circumlocution just to avoid inverting the matrix when there
are
> no active stages? Is that at all common?

yes, yes quite common.

Done.

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

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode634
src/gpu/GrContext.cpp:634: p = tmpPaint.get();
On 2012/07/19 18:55:09, TomH wrote:
> This two-step is an odd idiom, but I see it being used multiple times. Why?
> Would it be clearer with a named helper function? Or is scoping too ugly then?

I think most other occurrences are in Mike's code. 

Are you thinking something like this?

class SkTLazyReplace {
public:
   SkTLazyReplace(T* initialItem) {fItem = initialItem;}
   void replace() {
      fReplacement.set(*fItem);
      fItem = fReplacemet.get();
   }
   T* get() { return fItem;}

   // maybe * and -> operators
private:
   T* fItem;
   SkTLazy<T> fReplacement;
}

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode800
src/gpu/GrContext.cpp:800: GrDrawTarget::AutoReleaseGeometry geo(target, 0,
worstCaseVertCount, 0);
On 2012/07/19 18:55:09, TomH wrote:
> Could we get rid of the second argument now?

not quite yet... We need to formulate a plan for vertex layout removal.

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode842
src/gpu/GrContext.cpp:842: target->setVertexSourceToBuffer(0, sqVB);
On 2012/07/19 18:55:09, TomH wrote:
> First argument obsolete???

not yet

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode894
src/gpu/GrContext.cpp:894: drawState->preConcatSamplerMatrices(m);
On 2012/07/19 18:55:09, TomH wrote:
> So previously we only preconcatted the sampler matrices with stages after the
> first. Now we're also operating on the first stage. Behavior change? Or lying
> code?
Nice catch, I actually had found and fixed this bug but forgot to upload after
doing so.

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1065: GrDrawTarget::AutoDeviceCoordDraw adcd(target);
On 2012/07/19 18:55:09, TomH wrote:
> Despite my sheltered upbringing, I want to read your variable name as acdc.
haha, I keep having the same thought.

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrDrawState.h#newcode300
src/gpu/GrDrawState.h:300: bool preConcatSamplerMatricesWithInverse(const
GrMatrix& matrix) {
On 2012/07/19 18:55:09, TomH wrote:
> We have two very similar functions - thanks to levelization?


Yes, I think it'll get better when the GrPaint as a facade happens.

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

http://codereview.appspot.com/6422047/diff/2001/src/gpu/GrDrawTarget.cpp#oldc...
src/gpu/GrDrawTarget.cpp:1172: // sad trombone sound
On 2012/07/19 18:55:09, TomH wrote:
> I regret the timely demise of this comment.

Revived!

[TomH, 2012-07-19 19:55:54]

LGTM

[bsalomon, 2012-07-19 19:57:09]

I forgot to mention that I uploaded an updated patch.

[bsalomon, 2012-07-20 17:49:12]

Closed with r4688.