Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2132113002:
SkLS accepts nullptr for pointer args, handles alpha accurately, has new GM (Closed)
DescriptionSkLS now accepting nullptr for diffuse shader and normal source, now accurately handling alpha
This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002
BUG=skia:5502, skia:5517
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002
Committed: https://skia.googlesource.com/skia/+/c526da94e4f2dc0c8521099dad2118c5d6b8da4a
Patch Set 1 #Patch Set 2 : comment I forgot about diffuseshader nullptr #
Total comments: 44
Patch Set 3 : Addressed patch 2 comments #Patch Set 4 : Fixed alpha handling, fixed problem with new GM #Patch Set 5 : Overridden paint now gets destructed, clearer comments, GM now tests texture + transparent paint, a… #
Total comments: 10
Patch Set 6 : Fixed line length #Patch Set 7 : Addressed patch 5 comments #Patch Set 8 : PremulInputFP now using correct invariant output effect #
Total comments: 4
Patch Set 9 : Addressed patch 8 comments #Patch Set 10 : Changed position coordinate types #
Total comments: 12
Patch Set 11 : Addressed patch 10 comments #
Total comments: 2
Patch Set 12 : Addressed patch 11 comment #
Total comments: 2
Patch Set 13 : Fixed multiplatform warnings, addressed patch 12 comment #Patch Set 14 : Fixed more windows warnings #
Messages
Total messages: 54 (18 generated)
Description was changed from ========== SkLS now accepting nullptr for diffuse shader and normal source BUG=skia:5502 ========== to ========== SkLS now accepting nullptr for diffuse shader and normal source BUG=skia:5502 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ==========
Description was changed from ========== SkLS now accepting nullptr for diffuse shader and normal source BUG=skia:5502 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ========== to ========== SkLS now accepting nullptr for diffuse shader and normal source This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002 BUG=skia:5502 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com, robertphillips@google.com
https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:1: /* 2016 https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:27: // This GM exercises lighting shaders. // more ... // what does it do besides what the original GM does ? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:35: builder.add(SkLights::Light(SkColor3f::Make(1.0f, 1.0f, 1.0f), normalize this so the reader has a better expectation of where it is? Maybe make a named const version (e.g., kLightFromUpperRight) ? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:42: protected: extra spaces ? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:53: void onOnceBeforeDraw() override { Can we create the lights in here too ? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:62: // ... ? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:69: Hmmm, there is a lot of duplicate code here. Can we add useDiffuseMap and useNormapMap parameters and have only one draw function ? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:71: SkScalar rotate) { this-> https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:134: void onDraw(SkCanvas* canvas) override { one line ? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:138: canvas->save(); SK_ScalarSqrt2 ? https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:271: sk_sp<GrFragmentProcessor> fpPipeline[] = { this seems over tabbed https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:291: bool SkLightingShaderImpl::isOpaque() const { Hmmm, what happens if the paint's color is transparent ? https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:469: buf.writeFlattenable(fNormalSource.get()); Normally this would require a bump in the .skp version number. I think we're okay though since no one should be saving these out yet. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:333: SkNormalSource::Provider* asProvider(const SkShader::ContextRec& rec, void* storage) const Hmm, can this be differently indented to not orphan the "override" keyword ? https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:365: ?? https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:392: GrProcessorKeyBuilder* b) { Is the class ID automatically added ? https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:446: void NormalFlatSourceImpl::Provider::fillScanLine(int x, int y, SkPoint3 output[], int count) odd identing
https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:283: return sk_make_sp<LightingFP>(std::move(normalFP), fLights); I think we still need to do the premul of alpha after this right? As in do the same MulOutputByInputAlpha as above https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:393: result[i] = convert(accum, SkColorGetA(diffColor)); Add comment that convert also switches the result back to premul. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:392: GrProcessorKeyBuilder* b) { On 2016/07/11 14:02:04, robertphillips wrote: > Is the class ID automatically added ? yes.
https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:1: /* On 2016/07/11 14:02:04, robertphillips wrote: > 2016 Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:27: // This GM exercises lighting shaders. On 2016/07/11 14:02:04, robertphillips wrote: > // more ... > // what does it do besides what the original GM does ? Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:35: builder.add(SkLights::Light(SkColor3f::Make(1.0f, 1.0f, 1.0f), On 2016/07/11 14:02:04, robertphillips wrote: > normalize this so the reader has a better expectation of where it is? > Maybe make a named const version (e.g., kLightFromUpperRight) ? Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:42: protected: On 2016/07/11 14:02:04, robertphillips wrote: > extra spaces ? Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:53: void onOnceBeforeDraw() override { On 2016/07/11 14:02:04, robertphillips wrote: > Can we create the lights in here too ? Done. Why? https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:62: On 2016/07/11 14:02:04, robertphillips wrote: > // ... ? Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:69: On 2016/07/11 14:02:03, robertphillips wrote: > Hmmm, there is a lot of duplicate code here. Can we add useDiffuseMap and > useNormapMap parameters and have only one draw function ? Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:71: SkScalar rotate) { On 2016/07/11 14:02:04, robertphillips wrote: > this-> Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:134: void onDraw(SkCanvas* canvas) override { On 2016/07/11 14:02:04, robertphillips wrote: > one line ? Done. https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:138: canvas->save(); On 2016/07/11 14:02:04, robertphillips wrote: > SK_ScalarSqrt2 ? Done. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:271: sk_sp<GrFragmentProcessor> fpPipeline[] = { On 2016/07/11 14:02:04, robertphillips wrote: > this seems over tabbed Done. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:283: return sk_make_sp<LightingFP>(std::move(normalFP), fLights); On 2016/07/11 18:00:59, egdaniel wrote: > I think we still need to do the premul of alpha after this right? As in do the > same MulOutputByInputAlpha as above The alpha of the input color gets passed down to the output color (see SkLightingShader.cpp:182). In the diffuseShader==nullptr case, this input color is the paint's color, so I believe this to be correct. In any case, the GPU side has never handled alpha correctly for some reason, so this need some looking into either way. I would leave it for later. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:291: bool SkLightingShaderImpl::isOpaque() const { On 2016/07/11 14:02:04, robertphillips wrote: > Hmmm, what happens if the paint's color is transparent ? I don't know :S, we don't know the paint at shader initalization time. I changed the result to false since I checked and this is used to verify overdraw. Please let me know if you think this is wrong because I'm not very sure about this. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:393: result[i] = convert(accum, SkColorGetA(diffColor)); On 2016/07/11 18:00:59, egdaniel wrote: > Add comment that convert also switches the result back to premul. Done. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:469: buf.writeFlattenable(fNormalSource.get()); On 2016/07/11 14:02:04, robertphillips wrote: > Normally this would require a bump in the .skp version number. I think we're > okay though since no one should be saving these out yet. Acknowledged. It's changed quite a few times by now so that's good, hahaha. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:333: SkNormalSource::Provider* asProvider(const SkShader::ContextRec& rec, void* storage) const On 2016/07/11 14:02:04, robertphillips wrote: > Hmm, can this be differently indented to not orphan the "override" keyword ? Done. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:365: On 2016/07/11 14:02:04, robertphillips wrote: > ?? I left the includes there but guarded out so that I remember to move them if I end up refactoring NormalFlatFP out of this source file. Should I delete this? https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:392: GrProcessorKeyBuilder* b) { On 2016/07/11 18:00:59, egdaniel wrote: > On 2016/07/11 14:02:04, robertphillips wrote: > > Is the class ID automatically added ? > > yes. Acknowledged. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:446: void NormalFlatSourceImpl::Provider::fillScanLine(int x, int y, SkPoint3 output[], int count) On 2016/07/11 14:02:04, robertphillips wrote: > odd identing Done?
https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:53: void onOnceBeforeDraw() override { On 2016/07/11 19:37:28, dvonbeck wrote: > On 2016/07/11 14:02:04, robertphillips wrote: > > Can we create the lights in here too ? > > Done. Why? The problem with doing stuff in the constructor is that the tooling creates all the GMs up front. If you want to put a break point in the SkLights class and then run an individual GM you're likely to get a lot of irrelevant breaks for GMs that do stuff in their constructors. In general we try to do as little as possible in the constructor.
https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:291: bool SkLightingShaderImpl::isOpaque() const { On 2016/07/11 19:37:29, dvonbeck wrote: > On 2016/07/11 14:02:04, robertphillips wrote: > > Hmmm, what happens if the paint's color is transparent ? > > I don't know :S, we don't know the paint at shader initalization time. I changed > the result to false since I checked and this is used to verify overdraw. Please > let me know if you think this is wrong because I'm not very sure about this. We can't pull the alpha out of 'fPaintColor' ? https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:365: On 2016/07/11 19:37:29, dvonbeck wrote: > On 2016/07/11 14:02:04, robertphillips wrote: > > ?? > > I left the includes there but guarded out so that I remember to move them if I > end up refactoring NormalFlatFP out of this source file. Should I delete this? I think so. You can always find the include list elsewhere.
On 2016/07/11 20:51:42, robertphillips wrote: > https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... > File src/core/SkLightingShader.cpp (right): > > https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... > src/core/SkLightingShader.cpp:291: bool SkLightingShaderImpl::isOpaque() const { > On 2016/07/11 19:37:29, dvonbeck wrote: > > On 2016/07/11 14:02:04, robertphillips wrote: > > > Hmmm, what happens if the paint's color is transparent ? > > > > I don't know :S, we don't know the paint at shader initalization time. I > changed > > the result to false since I checked and this is used to verify overdraw. > Please > > let me know if you think this is wrong because I'm not very sure about this. > > We can't pull the alpha out of 'fPaintColor' ? > > https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... > File src/core/SkNormalSource.cpp (right): > > https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... > src/core/SkNormalSource.cpp:365: > On 2016/07/11 19:37:29, dvonbeck wrote: > > On 2016/07/11 14:02:04, robertphillips wrote: > > > ?? > > > > I left the includes there but guarded out so that I remember to move them if I > > end up refactoring NormalFlatFP out of this source file. Should I delete this? > > I think so. You can always find the include list elsewhere. I addition to the alpha fixes we discussed yesterday, can you add a comment to the Lighting fragment processor that it expects its input to be a premultiplied color?
Addressed all comments, fixed alpha, fixed new GM. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:291: bool SkLightingShaderImpl::isOpaque() const { On 2016/07/11 20:51:42, robertphillips wrote: > On 2016/07/11 19:37:29, dvonbeck wrote: > > On 2016/07/11 14:02:04, robertphillips wrote: > > > Hmmm, what happens if the paint's color is transparent ? > > > > I don't know :S, we don't know the paint at shader initalization time. I > changed > > the result to false since I checked and this is used to verify overdraw. > Please > > let me know if you think this is wrong because I'm not very sure about this. > > We can't pull the alpha out of 'fPaintColor' ? No, fPaintColor is a field of LightingShaderContext. LightingShaderContext gets created at draw time so it has access to the paint. SkLightingShaderImpl does not. https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2132113002/diff/20001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:365: On 2016/07/11 20:51:42, robertphillips wrote: > On 2016/07/11 19:37:29, dvonbeck wrote: > > On 2016/07/11 14:02:04, robertphillips wrote: > > > ?? > > > > I left the includes there but guarded out so that I remember to move them if I > > end up refactoring NormalFlatFP out of this source file. Should I delete this? > > I think so. You can always find the include list elsewhere. Done.
Description was changed from ========== SkLS now accepting nullptr for diffuse shader and normal source This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002 BUG=skia:5502 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ========== to ========== SkLS now accepting nullptr for diffuse shader and normal source This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002 BUG=skia:5502,skia:5517 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ==========
Description was changed from ========== SkLS now accepting nullptr for diffuse shader and normal source This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002 BUG=skia:5502,skia:5517 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ========== to ========== SkLS now accepting nullptr for diffuse shader and normal source, now accurately handling alpha This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002 BUG=skia:5502,skia:5517 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ==========
https://codereview.chromium.org/2132113002/diff/80001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/80001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:71: SkScalar rotate, bool useNormalSource = true, bool useDiffuseShader = true, not sure there is much advantage to the defaults here. Might as well explicitly write out everything in the draw to make it clear to read in that function. https://codereview.chromium.org/2132113002/diff/80001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:114: this->drawRect(canvas, r, 1.0f, 1.0f, 45.f, true, true, true); Would there be value to drawing all these combinations inside some nested for loop? Also seems like the scale is always uniform. I'd say either use just one scale param or test some non uniform scale if you want the two. https://codereview.chromium.org/2132113002/diff/80001/include/gpu/GrFragmentP... File include/gpu/GrFragmentProcessor.h (right): https://codereview.chromium.org/2132113002/diff/80001/include/gpu/GrFragmentP... include/gpu/GrFragmentProcessor.h:51: * Returns a fragment processor that runs 2 children in series, first a FP that premuls the I think just saying something like "Returns a fragment processor that premuls the input before calling the passed in fragment processor" is sufficient. There is no need to get into the details of children or running in series. https://codereview.chromium.org/2132113002/diff/80001/src/gpu/GrFragmentProce... File src/gpu/GrFragmentProcessor.cpp (right): https://codereview.chromium.org/2132113002/diff/80001/src/gpu/GrFragmentProce... src/gpu/GrFragmentProcessor.cpp:164: fragBuilder->codeAppendf("%s.rgb *= %s.a;", args.fOutputColor, args.fInputColor); 100 chars https://codereview.chromium.org/2132113002/diff/80001/src/gpu/GrFragmentProce... src/gpu/GrFragmentProcessor.cpp:175: inout->mulByUnknownFourComponents(); This doesn't seem right. I feel like it should be able to tell some info if some of the values are known. For example if we know the alpha is opaque then we should do nothing to the inout. Knowing that the color stays opaque can definitely be useful to us in our optimizations later on.
https://codereview.chromium.org/2132113002/diff/80001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/80001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:71: SkScalar rotate, bool useNormalSource = true, bool useDiffuseShader = true, On 2016/07/14 03:22:47, egdaniel wrote: > not sure there is much advantage to the defaults here. Might as well explicitly > write out everything in the draw to make it clear to read in that function. Done. https://codereview.chromium.org/2132113002/diff/80001/gm/lightingshader2.cpp#... gm/lightingshader2.cpp:114: this->drawRect(canvas, r, 1.0f, 1.0f, 45.f, true, true, true); On 2016/07/14 03:22:47, egdaniel wrote: > Would there be value to drawing all these combinations inside some nested for > loop? > > Also seems like the scale is always uniform. I'd say either use just one scale > param or test some non uniform scale if you want the two. I don't think so, since I am varying different parameters for every rectangle. I made the function take only one scale param, since an-isotropic scaling is unsupported. https://codereview.chromium.org/2132113002/diff/80001/include/gpu/GrFragmentP... File include/gpu/GrFragmentProcessor.h (right): https://codereview.chromium.org/2132113002/diff/80001/include/gpu/GrFragmentP... include/gpu/GrFragmentProcessor.h:51: * Returns a fragment processor that runs 2 children in series, first a FP that premuls the On 2016/07/14 03:22:47, egdaniel wrote: > I think just saying something like "Returns a fragment processor that premuls > the input before calling the passed in fragment processor" is sufficient. There > is no need to get into the details of children or running in series. Done. https://codereview.chromium.org/2132113002/diff/80001/src/gpu/GrFragmentProce... File src/gpu/GrFragmentProcessor.cpp (right): https://codereview.chromium.org/2132113002/diff/80001/src/gpu/GrFragmentProce... src/gpu/GrFragmentProcessor.cpp:164: fragBuilder->codeAppendf("%s.rgb *= %s.a;", args.fOutputColor, args.fInputColor); On 2016/07/14 03:22:47, egdaniel wrote: > 100 chars This was fixed in patch set 6 https://codereview.chromium.org/2132113002/diff/80001/src/gpu/GrFragmentProce... src/gpu/GrFragmentProcessor.cpp:175: inout->mulByUnknownFourComponents(); On 2016/07/14 03:22:47, egdaniel wrote: > This doesn't seem right. I feel like it should be able to tell some info if some > of the values are known. For example if we know the alpha is opaque then we > should do nothing to the inout. Knowing that the color stays opaque can > definitely be useful to us in our optimizations later on. Adding appropriate invariant option in separate CL.
PremulInputFP now using correct invariant output effect
lgtm https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:100: if (useTransparentPaint) { Let's also add a case for when we have a diffuse shader with is transparent. https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:110: SkRect r = SkRect::MakeWH(SkIntToScalar(kTexSize), SkIntToScalar(kTexSize)); So I think what would give us the best coverage would be first to draw the cross product of useDiffuseShader, DiffuseAlpha, paintAlpha. Then we can add a few special cases like rotated (I assume this is just to check the normals), various scales, and lack of normal shader.
On 2016/07/14 17:10:35, egdaniel wrote: > lgtm > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp > File gm/lightingshader2.cpp (right): > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... > gm/lightingshader2.cpp:100: if (useTransparentPaint) { > Let's also add a case for when we have a diffuse shader with is transparent. > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... > gm/lightingshader2.cpp:110: SkRect r = SkRect::MakeWH(SkIntToScalar(kTexSize), > SkIntToScalar(kTexSize)); > So I think what would give us the best coverage would be first to draw the cross > product of useDiffuseShader, DiffuseAlpha, paintAlpha. Then we can add a few > special cases like rotated (I assume this is just to check the normals), various > scales, and lack of normal shader. nolgtm
On 2016/07/14 17:10:50, egdaniel wrote: > On 2016/07/14 17:10:35, egdaniel wrote: > > lgtm > > > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp > > File gm/lightingshader2.cpp (right): > > > > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... > > gm/lightingshader2.cpp:100: if (useTransparentPaint) { > > Let's also add a case for when we have a diffuse shader with is transparent. > > > > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... > > gm/lightingshader2.cpp:110: SkRect r = SkRect::MakeWH(SkIntToScalar(kTexSize), > > SkIntToScalar(kTexSize)); > > So I think what would give us the best coverage would be first to draw the > cross > > product of useDiffuseShader, DiffuseAlpha, paintAlpha. Then we can add a few > > special cases like rotated (I assume this is just to check the normals), > various > > scales, and lack of normal shader. > > nolgtm no-lgtm. Sorry hit wrong botton, meant to hit publish.
On 2016/07/14 17:11:32, egdaniel wrote: > On 2016/07/14 17:10:50, egdaniel wrote: > > On 2016/07/14 17:10:35, egdaniel wrote: > > > lgtm > > > > > > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp > > > File gm/lightingshader2.cpp (right): > > > > > > > > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... > > > gm/lightingshader2.cpp:100: if (useTransparentPaint) { > > > Let's also add a case for when we have a diffuse shader with is transparent. > > > > > > > > > https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... > > > gm/lightingshader2.cpp:110: SkRect r = > SkRect::MakeWH(SkIntToScalar(kTexSize), > > > SkIntToScalar(kTexSize)); > > > So I think what would give us the best coverage would be first to draw the > > cross > > > product of useDiffuseShader, DiffuseAlpha, paintAlpha. Then we can add a few > > > special cases like rotated (I assume this is just to check the normals), > > various > > > scales, and lack of normal shader. > > > > nolgtm > > no-lgtm. Sorry hit wrong botton, meant to hit publish. not lgtm
I fixed up lighitingshader2! A screenshot: https://screenshot.googleplex.com/wvH5zFj3SVg
On 2016/07/19 19:54:27, dvonbeck wrote: > I fixed up lighitingshader2! A screenshot: > https://screenshot.googleplex.com/wvH5zFj3SVg That looks like a really nice gm! Since you've added text, make sure you are using sk_tool_utils::create_portable_typeface(...) so that it looks the same on various platforms. Upload the new version and I'll take a look.
Oops, I thought I had uploaded the code already. Here it is.
https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:100: if (useTransparentPaint) { On 2016/07/14 17:10:35, egdaniel wrote: > Let's also add a case for when we have a diffuse shader with is transparent. Done. https://codereview.chromium.org/2132113002/diff/140001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:110: SkRect r = SkRect::MakeWH(SkIntToScalar(kTexSize), SkIntToScalar(kTexSize)); On 2016/07/14 17:10:35, egdaniel wrote: > So I think what would give us the best coverage would be first to draw the cross > product of useDiffuseShader, DiffuseAlpha, paintAlpha. Then we can add a few > special cases like rotated (I assume this is just to check the normals), various > scales, and lack of normal shader. Done.
https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:10: #include <cmath> if we use simpler nested for loops shouldn't need this https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:107: sk_sp <SkShader> normalMap = SkMakeBitmapShader(fNormalMap, SkShader::kClamp_TileMode, can we premake this bitmapshader in onceBeforeDraw? https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:116: diffuseShader = SkMakeBitmapShader(fOpaqueDiffuse, SkShader::kClamp_TileMode, Here as well, premake? https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:119: diffuseShader = SkMakeBitmapShader(fTranslucentDiffuse, SkShader::kClamp_TileMode, Again here. https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:154: for (; gridNum < NUM_PARAM_COMBINATIONS; gridNum++) { Why not just use 4 nested for loops here, seems like it would be a lot more readable. Looking at your sample image it could just be. for (tanslucent shader) { for (transparent paint) { reset x for (diffuse shader) { for (normal source) { draw adjust x } } ajust y } adjust y } https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:172: sprintf(label, "%s: %d", BOOL_PARAM_LABELS[j], params[j]); its not common to use sprintf in Skia. Lets just change this to SkString instead
https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:10: #include <cmath> On 2016/07/19 22:26:15, egdaniel wrote: > if we use simpler nested for loops shouldn't need this Done. https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:107: sk_sp <SkShader> normalMap = SkMakeBitmapShader(fNormalMap, SkShader::kClamp_TileMode, On 2016/07/19 22:26:15, egdaniel wrote: > can we premake this bitmapshader in onceBeforeDraw? Done. https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:116: diffuseShader = SkMakeBitmapShader(fOpaqueDiffuse, SkShader::kClamp_TileMode, On 2016/07/19 22:26:15, egdaniel wrote: > Here as well, premake? Done. https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:119: diffuseShader = SkMakeBitmapShader(fTranslucentDiffuse, SkShader::kClamp_TileMode, On 2016/07/19 22:26:15, egdaniel wrote: > Again here. Done. https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:154: for (; gridNum < NUM_PARAM_COMBINATIONS; gridNum++) { On 2016/07/19 22:26:15, egdaniel wrote: > Why not just use 4 nested for loops here, seems like it would be a lot more > readable. > > Looking at your sample image it could just be. > > for (tanslucent shader) { > for (transparent paint) { > reset x > for (diffuse shader) { > for (normal source) { > draw > adjust x > } > } > ajust y > } > adjust y > } Done. I left the x,y adjustment as is because I feel like it's more flexible and easy to extend this way, and it makes the for loops cleaner. LMK if you still disagree. https://codereview.chromium.org/2132113002/diff/180001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:172: sprintf(label, "%s: %d", BOOL_PARAM_LABELS[j], params[j]); On 2016/07/19 22:26:15, egdaniel wrote: > its not common to use sprintf in Skia. Lets just change this to SkString instead Done.
lgtm once you fix the for loops https://codereview.chromium.org/2132113002/diff/200001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/200001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:138: for (bool useNormalSource : {true, false}) skia style requires this all to tabbed in from eachother with their own {}
https://codereview.chromium.org/2132113002/diff/200001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/200001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:138: for (bool useNormalSource : {true, false}) On 2016/07/20 15:46:19, egdaniel wrote: > skia style requires this all to tabbed in from eachother with their own {} Done.
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2132113002/#ps220001 (title: "Addressed patch 11 comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
https://codereview.chromium.org/2132113002/diff/220001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/220001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:138: for (bool useNormalSource : {true, false}) { you don't need the extra spaces here anymore since the for's aren't lined up.
https://codereview.chromium.org/2132113002/diff/220001/gm/lightingshader2.cpp File gm/lightingshader2.cpp (right): https://codereview.chromium.org/2132113002/diff/220001/gm/lightingshader2.cpp... gm/lightingshader2.cpp:138: for (bool useNormalSource : {true, false}) { On 2016/07/20 16:48:00, egdaniel wrote: > you don't need the extra spaces here anymore since the for's aren't lined up. Oops! Done.
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2132113002/#ps240001 (title: "Fixed multiplatform warnings, addressed patch 12 comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2132113002/#ps260001 (title: "Fixed more windows warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
dvonbeck@google.com changed reviewers: + bsalomon@google.com - egdaniel@google.com, robertphillips@google.com
dvonbeck@google.com changed reviewers: + egdaniel@google.com
On 2016/07/20 18:03:12, dvonbeck wrote: > https://codereview.chromium.org/2132113002/diff/260001/include/gpu/GrFragment... > > Thanks! api lgtm
The CQ bit was checked by dvonbeck@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkLS now accepting nullptr for diffuse shader and normal source, now accurately handling alpha This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002 BUG=skia:5502,skia:5517 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 ========== to ========== SkLS now accepting nullptr for diffuse shader and normal source, now accurately handling alpha This CL's base is the CL for taking in a diffuse shader into SkLS on the API side: https://codereview.chromium.org/2064153002 BUG=skia:5502,skia:5517 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132113002 Committed: https://skia.googlesource.com/skia/+/c526da94e4f2dc0c8521099dad2118c5d6b8da4a ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://skia.googlesource.com/skia/+/c526da94e4f2dc0c8521099dad2118c5d6b8da4a |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
