Move GrTextureParams from GrSamplerState to GrTextureAccess

[bsalomon, 2012-09-17 21:47:53]

Currently the GrTextureParams are set on GrSamplerState. This is wrong for at
least reasons:

1) GrSamplerState was basically a description of each stage and is going away as
its functionality moves to GrCustomStage

2) In the future stage's can have more than one texture and thus need to have
multiple GrTextureParams.

I had hoped to combine GrTextureParams and GrTextureAccess, but GrTextureParams
is used to perform cache lookups when NPOT tiling may not be available. At
cache-lookup time we don't know either the texture or swizzle that will be used
with it (which are required for GrTextureAccess) :(

[TomH, 2012-09-17 21:57:32]

Why not combine and have an "unknown" enum or an "incomplete" flag?
Eh, no, probably better to do it your way. Hard for me to know without
rereading all the code.


On Mon, Sep 17, 2012 at 10:47 PM, <bsalomon@google.com> wrote:

> Reviewers: robertphillips,
>
> Message:
> Currently the GrTextureParams are set on GrSamplerState. This is wrong
> for at least reasons:
>
> 1) GrSamplerState was basically a description of each stage and is going
> away as its functionality moves to GrCustomStage
>
> 2) In the future stage's can have more than one texture and thus need to
> have multiple GrTextureParams.
>
> I had hoped to combine GrTextureParams and GrTextureAccess, but
> GrTextureParams is used to perform cache lookups when NPOT tiling may
> not be available. At cache-lookup time we don't know either the texture
> or swizzle that will be used with it (which are required for
> GrTextureAccess) :(
>
>
>
>
> Please review this at
https://codereview.appspot.**com/6496135/<https://codereview.appspot.com/6496...
>
> Affected files:
>   M     include/gpu/GrSamplerState.h
>   M     include/gpu/GrTextureAccess.h
>   M     include/gpu/SkGpuDevice.h
>   M     src/effects/gradients/**SkGradientShader.cpp
>   M     src/effects/gradients/**SkGradientShaderPriv.h
>   M     src/effects/gradients/**SkLinearGradient.cpp
>   M     src/effects/gradients/**SkRadialGradient.cpp
>   M     src/effects/gradients/**SkSweepGradient.cpp
>   M     src/effects/gradients/**SkTwoPointConicalGradient.cpp
>   M     src/effects/gradients/**SkTwoPointRadialGradient.cpp
>   M     src/gpu/GrContext.cpp
>   M     src/gpu/GrCustomStage.cpp
>   M     src/gpu/GrDrawState.h
>   M     src/gpu/GrTextContext.cpp
>   M     src/gpu/GrTextureAccess.cpp
>   M     src/gpu/SkGpuDevice.cpp
>   M     src/gpu/effects/**GrSingleTextureEffect.cpp
>   M     src/gpu/effects/**GrSingleTextureEffect.h
>   M     src/gpu/effects/**GrTextureDomainEffect.cpp
>   M     src/gpu/effects/**GrTextureDomainEffect.h
>   M     src/gpu/gl/GrGpuGL.cpp
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "skia-review" group.
> To post to this group, send email to skia-review@googlegroups.com.
> To unsubscribe from this group, send email to skia-review+unsubscribe@**
> googlegroups.com <skia-review%2Bunsubscribe@googlegroups.com>.
> For more options, visit this group at http://groups.google.com/**
> group/skia-review?hl=en <http://groups.google.com/group/skia-review?hl=en>
> .
>
>

[robertphillips, 2012-09-18 12:41:00]

LGTM + some nits & questions

https://codereview.appspot.com/6496135/diff/2001/include/gpu/GrTextureAccess.h
File include/gpu/GrTextureAccess.h (right):

https://codereview.appspot.com/6496135/diff/2001/include/gpu/GrTextureAccess....
include/gpu/GrTextureAccess.h:19: * GrTextureAccess (defined below). Also, some
of the texture cache methods require knowledge about
about about

https://codereview.appspot.com/6496135/diff/2001/include/gpu/GrTextureAccess....
include/gpu/GrTextureAccess.h:126: * swizzle must be a string between one and
four (inclusive) characters containing only 'r',
I don't think the "Uses default GrTextureParams" is correct

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkGrad...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkGrad...
src/effects/gradients/SkGradientShader.cpp:739: 
Comment here about ModeY always being clamp and filtering always being on?

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkLine...
File src/effects/gradients/SkLinearGradient.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkLine...
src/effects/gradients/SkLinearGradient.cpp:563:
sampler->matrix()->preConcat(fPtsToUnit);
indentation?

https://codereview.appspot.com/6496135/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1294: }
Is there a ctor for this?

https://codereview.appspot.com/6496135/diff/2001/src/gpu/effects/GrTextureDom...
File src/gpu/effects/GrTextureDomainEffect.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/gpu/effects/GrTextureDom...
src/gpu/effects/GrTextureDomainEffect.cpp:99:
GrTextureDomainEffect::GrTextureDomainEffect(GrTexture* texture,
Why isn't domain a const& ?

https://codereview.appspot.com/6496135/diff/2001/src/gpu/effects/GrTextureDom...
File src/gpu/effects/GrTextureDomainEffect.h (right):

https://codereview.appspot.com/6496135/diff/2001/src/gpu/effects/GrTextureDom...
src/gpu/effects/GrTextureDomainEffect.h:23: /** Uses default texture params (no
filter, clamp) */
same question r.e. domain

[bsalomon, 2012-09-18 14:15:16]

Committed as r5582

https://codereview.appspot.com/6496135/diff/2001/include/gpu/GrTextureAccess.h
File include/gpu/GrTextureAccess.h (right):

https://codereview.appspot.com/6496135/diff/2001/include/gpu/GrTextureAccess....
include/gpu/GrTextureAccess.h:19: * GrTextureAccess (defined below). Also, some
of the texture cache methods require knowledge about
On 2012/09/18 12:41:00, robertphillips wrote:
> about about

Done.

https://codereview.appspot.com/6496135/diff/2001/include/gpu/GrTextureAccess....
include/gpu/GrTextureAccess.h:126: * swizzle must be a string between one and
four (inclusive) characters containing only 'r',
On 2012/09/18 12:41:00, robertphillips wrote:
> I don't think the "Uses default GrTextureParams" is correct

Done.

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkGrad...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkGrad...
src/effects/gradients/SkGradientShader.cpp:739: 
On 2012/09/18 12:41:00, robertphillips wrote:
> Comment here about ModeY always being clamp and filtering always being on?

Done.

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkLine...
File src/effects/gradients/SkLinearGradient.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/effects/gradients/SkLine...
src/effects/gradients/SkLinearGradient.cpp:563:
sampler->matrix()->preConcat(fPtsToUnit);
On 2012/09/18 12:41:00, robertphillips wrote:
> indentation?

Done.

https://codereview.appspot.com/6496135/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1294: }
On 2012/09/18 12:41:00, robertphillips wrote:
> Is there a ctor for this?

Currently the order of params between GrTextureAccess and GrTextureParams is
different:

(bilerp, tm) for params
(tm, bilerp) for access

I think the latter is better since we care about the filter in more places than
the tm. A coming change will make them consistent and can save a line here.

https://codereview.appspot.com/6496135/diff/2001/src/gpu/effects/GrTextureDom...
File src/gpu/effects/GrTextureDomainEffect.cpp (right):

https://codereview.appspot.com/6496135/diff/2001/src/gpu/effects/GrTextureDom...
src/gpu/effects/GrTextureDomainEffect.cpp:99:
GrTextureDomainEffect::GrTextureDomainEffect(GrTexture* texture,
On 2012/09/18 12:41:00, robertphillips wrote:
> Why isn't domain a const& ?

Good catch, changed.