Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118553002:
adding new GM to demostrate new shadows (Closed)
|
Created:
4 years, 5 months ago by vjiaoblack Modified:
4 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionadding new GM to demostrate new shadows
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2118553002
Committed: https://skia.googlesource.com/skia/+/53da5ba619553aa47dfe27c315f7deae389e6b07
Patch Set 1 #Patch Set 2 : Updated! #Patch Set 3 : fixed codez #
Total comments: 4
Patch Set 4 : Added the framework for having canvas / recorder / picture record depth sets. #Patch Set 5 : Basic version works; rebased off master #Patch Set 6 : Forgot to commit files. #
Total comments: 114
Patch Set 7 : Made requested changes #
Total comments: 73
Patch Set 8 : Made requested changes #Patch Set 9 : Made requested changes #
Total comments: 26
Patch Set 10 : Made req changes, and fixed CPU lighting. #
Total comments: 10
Patch Set 11 : made req changes #
Total comments: 6
Patch Set 12 : made changes #Patch Set 13 : Disabled shadow maps #
Total comments: 3
Patch Set 14 : Added variable size shadow maps and fixed soem bugs #Patch Set 15 : Lines too long #
Dependent Patchsets: Messages
Total messages: 48 (16 generated)
Description was changed from ========== adding new GM to demostrate new shadows BUG=skia: ========== to ========== adding new GM to demostrate new shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2118553002 ==========
vjiaoblack@google.com changed reviewers: + robertphillips@google.com
pls help look at my code on dev machine!
Description was changed from ========== adding new GM to demostrate new shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2118553002 ========== to ========== adding new GM to demostrate new shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2118553002 ==========
robertphillips@google.com changed reviewers: + jvanverth@google.com
https://codereview.chromium.org/2118553002/diff/40001/gm/victor-shadows.cpp File gm/victor-shadows.cpp (right): https://codereview.chromium.org/2118553002/diff/40001/gm/victor-shadows.cpp#n... gm/victor-shadows.cpp:52: // Maybe add the ID here along with the depth Note that you're also going to need to add a SkCanvas::getZ call that is akin to getTotalMatrix. https://codereview.chromium.org/2118553002/diff/40001/gm/victor-shadows.cpp#n... gm/victor-shadows.cpp:81: Thinking about this a bit more I think we should use the SkPaintFilterCanvas instead. This is b.c. the SkPaintFilterCanvas::onFilter method can look at the state of the canvas itself (i.e., the z-level). Additionally, I don't think we will need to alter all the operations in SkRecords.h. I think we can just have the current z-level stored in SkCanvas::fClipStack and access it from there. (Note that you will still need to add a new SetZ record type to SkRecords.h to hold the recorded value of the SkCanvas::setZ call). https://codereview.chromium.org/2118553002/diff/40001/gm/victor-shadows.cpp#n... gm/victor-shadows.cpp:145: // TODO: change to kIndex_8_SkColorType We could probably start off with this being kAlpha_8_SkColorType/kPremul_SkAlphaType.
lgtm https://codereview.chromium.org/2118553002/diff/40001/gm/victor-shadows.cpp File gm/victor-shadows.cpp (right): https://codereview.chromium.org/2118553002/diff/40001/gm/victor-shadows.cpp#n... gm/victor-shadows.cpp:81: On 2016/07/01 15:17:54, robertphillips wrote: > Thinking about this a bit more I think we should use the SkPaintFilterCanvas > instead. This is b.c. the SkPaintFilterCanvas::onFilter method can look at the > state of the canvas itself (i.e., the z-level). > > Additionally, I don't think we will need to alter all the operations in > SkRecords.h. I think we can just have the current z-level stored in > SkCanvas::fClipStack and access it from there. (Note that you will still need to > add a new SetZ record type to SkRecords.h to hold the recorded value of the > SkCanvas::setZ call). After talking to Robert about this, when we create the new paint in the SkPaintFilter canvas, we'll want to copy the patheffect and maskfilter, but not the imagefilter (because that would be too much to handle). The color in the new paint will be the index copied into each of the RGBA channels, i.e., RGBA = (index, index, index, index). And one of your tests should be a dashed line (which uses a patheffect) to make sure this works. And probably a blur maskfilter as well.
notlgtm
On 2016/07/01 15:31:35, jvanverth1 wrote: > notlgtm not lgtm (argh)
updated!!!!!!! u can see the depth values in the top left quadrant on top of the actual image :) would be gr9 if someone could maybe check over and make sure what I've done with canvas and Records (etc) is valid?
On 2016/07/07 17:24:03, vjiaoblack wrote: > updated!!!!!!! u can see the depth values in the top left quadrant on top of the > actual image > > :) > > would be gr9 if someone could maybe check over and make sure what I've done with > canvas and Records (etc) is valid? Ack. Broke with the current master somehow. Checking. Sorry
On 2016/07/07 17:46:06, vjiaoblack wrote: > On 2016/07/07 17:24:03, vjiaoblack wrote: > > updated!!!!!!! u can see the depth values in the top left quadrant on top of > the > > actual image > > > > :) > > > > would be gr9 if someone could maybe check over and make sure what I've done > with > > canvas and Records (etc) is valid? > > Ack. Broke with the current master somehow. Checking. Sorry Nvm. It was working; had some changes after the push and I freaked out
On 2016/07/07 17:50:13, vjiaoblack wrote: > On 2016/07/07 17:46:06, vjiaoblack wrote: > > On 2016/07/07 17:24:03, vjiaoblack wrote: > > > updated!!!!!!! u can see the depth values in the top left quadrant on top of > > the > > > actual image > > > > > > :) > > > > > > would be gr9 if someone could maybe check over and make sure what I've done > > with > > > canvas and Records (etc) is valid? > > > > Ack. Broke with the current master somehow. Checking. Sorry > > Nvm. It was working; had some changes after the push and I freaked out We don't want to commit the GM just yet -- I'd make a new patch with just your canvas and record changes and put that up for review.
vjiaoblack@google.com changed reviewers: + bsalomon@google.com
vjiaoblack@google.com changed reviewers: + reed@google.com - bsalomon@google.com, jvanverth@google.com, robertphillips@google.com
jvanverth@google.com changed reviewers: + bsalomon@google.com, jvanverth@google.com, robertphillips@google.com
My only general comment is that I think using the shadow mapping algorithm to generate a depth map for the view direction is fine for this GM, but we really need to find a way to pass the current Z value for the primitive down to the shader, either via a uniform or (ideally) via vertex attributes. See issue https://bugs.chromium.org/p/skia/issues/detail?id=5572 https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp File gm/victor-shadows.cpp (right): https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:1: /* I'd rename this to something more generic, like shadow-maps.cpp and ShadowMapsGM. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:24: // this is the 0, 0, 1 light It's unclear based on this comment what this means. And do you need to pass in the povDepth image and create a separate shader for this? Can't you just create a temporary image in the SkShadowShader? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:28: &matrix1); If the matrix is identity, you don't need to pass it in. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:38: sk_sp<SkPicture> make_picture(int width, int height, sk_sp<SkLights> lights) { This name seems a little generic. Maybe make_test_picture? Or include it as part of the ShadowsGM class. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:111: SkVector3 lightDir = this->fLights->light(0).dir(); Some sort of comment here that describes what you're doing would be helpful. And I thought you had multiple lights working? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:312: // TODO: change to kIndex_8_SkColorType Or use kAlpha_8_SkColorType? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:376: canvas->drawRect(SkRect::MakeIWH(400, 400), paint); kWidth, kHeight? https://codereview.chromium.org/2118553002/diff/100001/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.chromium.org/2118553002/diff/100001/gyp/common_variables.g... gyp/common_variables.gypi:254: 'skia_experimental_shadowing': 1, # for experimental shadow-drawing I don't think you want to commit this. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkPictureReco... File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/2118553002/diff/100001/src/core/SkPictureReco... src/core/SkPictureRecord.h:164: #else How did this compile? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:128: fDepthMapAccess[i].reset(((SkImage_Base*)lights->light(i).getShadowMap().get())-> You might want to break this line up -- it's a little confusing as to what's going on here. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:129: asTextureRef(GrContext::Create(kOpenGL_GrBackend, (GrBackendContext) nullptr, GrContextOptions()), Lines too long (must be <= 100 chars) https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:132: // // TODO: Call unref on the GrTexture somewhere. Is this still needed? Otherwise you have a memory leak. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:159: kVec3f_GrSLType, kDefault_GrSLPrecision, Line up indents next to parens. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:164: kVec3f_GrSLType, kDefault_GrSLPrecision, Lines too long. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:179: fragBuilder->codeAppendf("vec2 %s = vMatrixCoord_0_1_Stage0 + (vec2(%s) * povDepth.b * 255 / 400);", Line too long. Also, it's not clear to me what this math is doing. Why divide by 400? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:201: fragBuilder->codeAppendf("%s += dot(vec3(0,0,1), %s) * %s;", totalLightColor.c_str(), Line too long. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:318: localMatrix, filterQuality, Line too long.
https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp File gm/victor-shadows.cpp (right): https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:8: #include "gm.h" Do you still need SkDrawFilter.h ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:18: overlength line file static methods are make_shadow_shader formatted https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:34: std::move(lights) ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:37: If it is named this way it should be file static // add a comment https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:79: // Why does this class exist https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:82: Do you need to this entry point ? It isn't virtual in the base class. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:86: Can you fit this all on one line. Also no spaces in "( canvas )". https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:101: void onDrawPicture(const SkPicture* picture, const SkMatrix* matrix, const SkPaint* paint) { Maybe some words here about how the updating of the matrix works within a picture draw ? Particularly, nested pictures. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:102: SkTCopyOnFirstWrite<SkPaint> filteredPaint(paint); Calls to member functions are preceded with "this->" Why are we explicitly filtering here - won't the base class handle that for us? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:103: if (onFilter(&filteredPaint, kPicture_Type)) Need {}s around this line Not "this->INHERITED::onDrawPicture" ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:107: void updateMatrix() { this-> https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:109: Shouldn't we only ever have one light when we get here ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:118: void onDrawPaint(const SkPaint& paint) { this-> & all following instances too https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:120: this->INHERITED::onDrawPaint(paint); this-> & all following instances too https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:124: void onDrawPoints(PointMode mode, size_t count, const SkPoint pts[], align the "const SkPint& paint" somehow https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:275: this->setBGColor(sk_tool_utils::color_to_565(0xFFCCCCCC)); move the construction of the lights into onOnceBeforeDraw ? // Create a light set consisting of greenish infinite light pointing slightly to the right and a reddish infinite light pointing slightly down ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:289: SkString onShortName() override { Probably rename to shadowmap-shadows or something else https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:298: // This picture stores the picture of the scene. depth maps or shadow maps ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:301: Unless I'm missing something this isn't a cast. Do you mean: Wrap the input canvas in an ShadowPaintFilterCanvas ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:312: // TODO: change to kIndex_8_SkColorType On 2016/07/26 15:01:24, jvanverth1 wrote: > Or use kAlpha_8_SkColorType? We should probably avoid trying to render to Index_8. As Jim mentions, kAlpha_8 is probably the way to go. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:318: sk_sp<SkSurface> surf(SPFCanvas->makeSurface(info)); // Wrap the new canvas in a ShadowPaintFilterCanvas only with more semantic info ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:332: // TODO: plumb down each primitive's Z so we don't have to separately render the depth and color for the final pass ? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:347: sk_sp<SkSurface> surf(SPFCanvas->makeSurface(info)); // Wrap ... https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:351: // set the depth map canvas to have the light we're drawing. How does curLight differ from povLight ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:1: /* 2016 https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:14: #include "SkMathPriv.h" Do we need SkNormalSource.h ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:72: SK_TO_STRING_OVERRIDE() Tab this over ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:117: // TODO do we support 4 directional lights, or just 4 lights? Probably any number of ambient + 4 "real" ones. Maybe replace 4 everywhere with kMaxNumNonAmbientLights ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:119: This doesn't seem quite right. fNumLights needs to factor into account that all the ambient lights are collapsed together. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:120: fNumLights = lights->numLights(); // refers to directional lights. You probably can't have this assert but keep a separate counter for the number of non-ambient lights encountered and then just drop the extras on the floor (asserting if there are any extras) https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:222: We set fNumLights in the ctor. Is it correct to reset it here ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:227: for (int i = 0; i < fNumLights; i++) { Do we need the "1" suffixes? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:255: can this just be an int? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:283: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); Shouldn't we check if the number of lights is the same first ? Since we're comparing possible empty slots at the end, have we guaranteed that all the slots are initialized? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:294: line this guy up with the rest ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:312: I'm not in love with the D_FP* names https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:1: /* 2016 https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:10: Do we actually need these two includes or can we just predeclare them ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:13: Do we actually need the SkBitmap & SkMatrix predeclarations ? https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:23: static sk_sp<SkShader> Make(sk_sp<SkShader> povDepthShader, just diffuseShader (no 2) ?
https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp File gm/victor-shadows.cpp (right): https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:1: /* On 2016/07/26 15:01:24, jvanverth1 wrote: > I'd rename this to something more generic, like shadow-maps.cpp and > ShadowMapsGM. Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:8: #include "gm.h" On 2016/07/26 16:15:04, robertphillips wrote: > Do you still need SkDrawFilter.h ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:18: On 2016/07/26 16:15:04, robertphillips wrote: > overlength line > file static methods are make_shadow_shader formatted Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:24: // this is the 0, 0, 1 light Is that necessarily better? Asking because IDK. The shader that I copied seemed to prefer / have no problem with using shaders / normal sources. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:28: &matrix1); On 2016/07/26 15:01:24, jvanverth1 wrote: > If the matrix is identity, you don't need to pass it in. Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:34: On 2016/07/26 16:15:05, robertphillips wrote: > std::move(lights) ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:37: On 2016/07/26 16:15:05, robertphillips wrote: > If it is named this way it should be file static > // add a comment Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:38: sk_sp<SkPicture> make_picture(int width, int height, sk_sp<SkLights> lights) { On 2016/07/26 15:01:24, jvanverth1 wrote: > This name seems a little generic. Maybe make_test_picture? Or include it as part > of the ShadowsGM class. Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:79: On 2016/07/26 16:15:04, robertphillips wrote: > // Why does this class exist Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:82: On 2016/07/26 16:15:04, robertphillips wrote: > Do you need to this entry point ? > It isn't virtual in the base class. Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:86: On 2016/07/26 16:15:05, robertphillips wrote: > Can you fit this all on one line. Also no spaces in "( canvas )". Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:101: void onDrawPicture(const SkPicture* picture, const SkMatrix* matrix, const SkPaint* paint) { On 2016/07/26 16:15:04, robertphillips wrote: > Maybe some words here about how the updating of the matrix works within a > picture draw ? > Particularly, nested pictures. Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:102: SkTCopyOnFirstWrite<SkPaint> filteredPaint(paint); On 2016/07/26 16:15:05, robertphillips wrote: > Calls to member functions are preceded with "this->" > Why are we explicitly filtering here - won't the base class handle that for us? Filtering is necessary for calling onFilter, which is necessary for rendering depths https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:103: if (onFilter(&filteredPaint, kPicture_Type)) On 2016/07/26 16:15:04, robertphillips wrote: > Need {}s around this line > Not "this->INHERITED::onDrawPicture" ? Doing that didn't work. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:107: void updateMatrix() { On 2016/07/26 16:15:04, robertphillips wrote: > this-> Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:109: On 2016/07/26 16:15:04, robertphillips wrote: > Shouldn't we only ever have one light when we get here ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:111: SkVector3 lightDir = this->fLights->light(0).dir(); On 2016/07/26 15:01:24, jvanverth1 wrote: > Some sort of comment here that describes what you're doing would be helpful. And > I thought you had multiple lights working? I did! I can't really pass into these override-ed functions a value to tell it what light to render the shadow map for. Thus, I simply decided to draw the shadow map from the first light in its fLights (inherited from SkCanvas). We go through all the lights to render shadow maps for and take turns setting SPFCanvas's fLights equal to them. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:118: void onDrawPaint(const SkPaint& paint) { On 2016/07/26 16:15:05, robertphillips wrote: > this-> & all following instances too Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:120: this->INHERITED::onDrawPaint(paint); On 2016/07/26 16:15:04, robertphillips wrote: > this-> & all following instances too Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:124: void onDrawPoints(PointMode mode, size_t count, const SkPoint pts[], On 2016/07/26 16:15:05, robertphillips wrote: > align the "const SkPint& paint" somehow Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:275: this->setBGColor(sk_tool_utils::color_to_565(0xFFCCCCCC)); On 2016/07/26 16:15:04, robertphillips wrote: > move the construction of the lights into onOnceBeforeDraw ? > > // Create a light set consisting of greenish infinite light pointing slightly to > the right and a reddish infinite light pointing slightly down > > ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:289: SkString onShortName() override { On 2016/07/26 16:15:05, robertphillips wrote: > Probably rename to shadowmap-shadows or something else Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:298: // This picture stores the picture of the scene. On 2016/07/26 16:15:04, robertphillips wrote: > depth maps or shadow maps ? Technically, shadow maps === depth maps, right? (are equivalent by definition, not sure if thats the right symb) https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:301: On 2016/07/26 16:15:04, robertphillips wrote: > Unless I'm missing something this isn't a cast. Do you mean: Wrap the input > canvas in an ShadowPaintFilterCanvas ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:312: // TODO: change to kIndex_8_SkColorType On 2016/07/26 15:01:24, jvanverth1 wrote: > Or use kAlpha_8_SkColorType? I don't think this is actually possible with the current way we do this. Using the SPFCanvas means that I'm whacking the paint to get the right output into the field. Since SkPaint only has a fColor32 (right? I tried for variance maps to use a kRGBA_16F but it wouldn't hold due to this), any change in this doesn't really work well, right? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:312: // TODO: change to kIndex_8_SkColorType On 2016/07/26 16:15:04, robertphillips wrote: > On 2016/07/26 15:01:24, jvanverth1 wrote: > > Or use kAlpha_8_SkColorType? > > We should probably avoid trying to render to Index_8. As Jim mentions, kAlpha_8 > is probably the way to go. Is that possible through SkPaint? https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:318: sk_sp<SkSurface> surf(SPFCanvas->makeSurface(info)); On 2016/07/26 16:15:04, robertphillips wrote: > // Wrap the new canvas in a ShadowPaintFilterCanvas > > only with more semantic info ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:332: On 2016/07/26 16:15:05, robertphillips wrote: > // TODO: plumb down each primitive's Z so we don't have to separately render the > depth and color for the final pass > > ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:347: sk_sp<SkSurface> surf(SPFCanvas->makeSurface(info)); On 2016/07/26 16:15:04, robertphillips wrote: > // Wrap ... Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:351: // set the depth map canvas to have the light we're drawing. On 2016/07/26 16:15:05, robertphillips wrote: > How does curLight differ from povLight ? Done. https://codereview.chromium.org/2118553002/diff/100001/gm/victor-shadows.cpp#... gm/victor-shadows.cpp:376: canvas->drawRect(SkRect::MakeIWH(400, 400), paint); On 2016/07/26 15:01:24, jvanverth1 wrote: > kWidth, kHeight? Done. https://codereview.chromium.org/2118553002/diff/100001/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.chromium.org/2118553002/diff/100001/gyp/common_variables.g... gyp/common_variables.gypi:254: 'skia_experimental_shadowing': 1, # for experimental shadow-drawing On 2016/07/26 15:01:24, jvanverth1 wrote: > I don't think you want to commit this. Acknowledged. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkPictureReco... File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/2118553002/diff/100001/src/core/SkPictureReco... src/core/SkPictureRecord.h:164: #else On 2016/07/26 15:01:24, jvanverth1 wrote: > How did this compile? :P https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:1: /* On 2016/07/26 16:15:05, robertphillips wrote: > 2016 Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:14: #include "SkMathPriv.h" On 2016/07/26 16:15:05, robertphillips wrote: > Do we need SkNormalSource.h ? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:72: SK_TO_STRING_OVERRIDE() On 2016/07/26 16:15:05, robertphillips wrote: > Tab this over ? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:117: // TODO do we support 4 directional lights, or just 4 lights? On 2016/07/26 16:15:05, robertphillips wrote: > Probably any number of ambient + 4 "real" ones. > > Maybe replace 4 everywhere with kMaxNumNonAmbientLights ? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:119: On 2016/07/26 16:15:05, robertphillips wrote: > This doesn't seem quite right. fNumLights needs to factor into account that all > the ambient lights are collapsed together. I'll change fNumLights to fNumDirLights. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:120: fNumLights = lights->numLights(); // refers to directional lights. On 2016/07/26 16:15:05, robertphillips wrote: > You probably can't have this assert but keep a separate counter for the number > of non-ambient lights encountered and then just drop the extras on the floor > (asserting if there are any extras) Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:128: fDepthMapAccess[i].reset(((SkImage_Base*)lights->light(i).getShadowMap().get())-> On 2016/07/26 15:01:25, jvanverth1 wrote: > You might want to break this line up -- it's a little confusing as to what's > going on here. Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:129: asTextureRef(GrContext::Create(kOpenGL_GrBackend, (GrBackendContext) nullptr, GrContextOptions()), On 2016/07/26 15:01:24, jvanverth1 wrote: > Lines too long (must be <= 100 chars) Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:132: // // TODO: Call unref on the GrTexture somewhere. On 2016/07/26 15:01:25, jvanverth1 wrote: > Is this still needed? Otherwise you have a memory leak. Hm. I'm trying to fix this (and it seems like a pretty bad memory leak), but it might take a while - I'll continue on to fix the other comments and come back to this. edit: fixed! thanks for the tips https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:159: kVec3f_GrSLType, kDefault_GrSLPrecision, On 2016/07/26 15:01:25, jvanverth1 wrote: > Line up indents next to parens. Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:164: kVec3f_GrSLType, kDefault_GrSLPrecision, On 2016/07/26 15:01:24, jvanverth1 wrote: > Lines too long. Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:179: fragBuilder->codeAppendf("vec2 %s = vMatrixCoord_0_1_Stage0 + (vec2(%s) * povDepth.b * 255 / 400);", On 2016/07/26 15:01:25, jvanverth1 wrote: > Line too long. Also, it's not clear to me what this math is doing. Why divide by > 400? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:201: fragBuilder->codeAppendf("%s += dot(vec3(0,0,1), %s) * %s;", totalLightColor.c_str(), On 2016/07/26 15:01:24, jvanverth1 wrote: > Line too long. Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:222: On 2016/07/26 16:15:05, robertphillips wrote: > We set fNumLights in the ctor. Is it correct to reset it here ? We set fNumLights in the ShadowFP constructor. We are currently in the GLSLShadowFP, which isn't done with the same constructor. We need to pass in fNumLights in from the ShadowFP. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:227: for (int i = 0; i < fNumLights; i++) { On 2016/07/26 16:15:05, robertphillips wrote: > Do we need the "1" suffixes? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:255: On 2016/07/26 16:15:05, robertphillips wrote: > can this just be an int? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:283: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); On 2016/07/26 16:15:05, robertphillips wrote: > Shouldn't we check if the number of lights is the same first ? > Since we're comparing possible empty slots at the end, have we guaranteed that > all the slots are initialized? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:294: On 2016/07/26 16:15:05, robertphillips wrote: > line this guy up with the rest ? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:312: On 2016/07/26 16:15:05, robertphillips wrote: > I'm not in love with the D_FP* names Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:318: localMatrix, filterQuality, On 2016/07/26 15:01:25, jvanverth1 wrote: > Line too long. Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:1: /* On 2016/07/26 16:15:06, robertphillips wrote: > 2016 Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:10: On 2016/07/26 16:15:06, robertphillips wrote: > Do we actually need these two includes or can we just predeclare them ? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:13: On 2016/07/26 16:15:06, robertphillips wrote: > Do we actually need the SkBitmap & SkMatrix predeclarations ? Done. https://codereview.chromium.org/2118553002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.h:23: static sk_sp<SkShader> Make(sk_sp<SkShader> povDepthShader, On 2016/07/26 16:15:06, robertphillips wrote: > just diffuseShader (no 2) ? Done.
https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:116: fGrContext = context; Do you use this anywhere else? If not, you don't need to store it in the class. If so, we usually just use the name fContext. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:130: GrTextureParams::ClampNoFilter(), Still over 100 chars. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:160: kVec3f_GrSLType, kDefault_GrSLPrecision, Over 100 chars? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:165: kVec3f_GrSLType, kDefault_GrSLPrecision, Over 100 chars? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:330: sk_sp<GrFragmentProcessor> shadowfp = sk_make_sp<ShadowFP>(povDepthFP, diffuseFP, fLights, context); 100 chars
https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:8: #include "gm.h" Alphabetize the Sk* headers https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:14: #include "SkShadowShader.h" Do we need SkBitmapProcShader.h ? https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:80: space before the ':' use INHERITED rather than SkPaintFilterCanvas no ';' https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:86: SkColor color = 0xFF000000; // init color to opaque black assert that z will fit in 8 bits ? https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:93: Aren't all these onDraw* methods supposed to have an "override" suffix ? https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:94: void onDrawPicture(const SkPicture* picture, const SkMatrix* matrix, const SkPaint* paint) { It seems like SkPaintFilterCanvas::onDrawPicture does all this for you? Why do you need this? https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:95: SkTCopyOnFirstWrite<SkPaint> filteredPaint(paint); this->onFilter https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:96: if (onFilter(&filteredPaint, kPicture_Type)) Need enclosing brackets here I think you still need the this->SkCanvas::... https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:254: typedef SkPaintFilterCanvas INHERITED; extra '\n' here https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:295: Why do we wrap 'canvas' in an SPFC? AFAICT we never draw to it as an SPFC. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:306: // TODO: change to kIndex_8_SkColorType rm this last TODO or change to A8 https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:307: Maybe change kWidth & kHeight here to be kDefaultShadowMapWidth & kDefaultShadowMapHeight ? https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:311: // Create a new surface (that matches the backend of SPFCanvas) for each shadow map ? https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:346: // Create a new surface (that matches the backend of 'canvas') to create the povDepthMap ? https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:373: Make this 2 lines: SkPaint paint; paint.setShader(make_shadow_shader(povDepthMap, diffuseMap, fLights)); ? If not you should move the shadowShader into the paint. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:67: I think these two lines are supposed to be tabbed over https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:123: fNumDirLights++; I think you need to index by fNumDirLights (in the LHS) in the following assignments (the RHS remain being indexed by 'i'). This also means you need to defer the increment of fNumDirLights until the end of this if-block. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:128: // this sk_sp gets deleted when the ShadowFP is destroyed, and frees the GrTexture* IIUC, asTextureRef returns a "GrTexture*" so I think you just need: fTexture[i].reset(shadowMap->...); I'm surprised the std::move isn't complaining. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:181: // povDepth.b * 255 scales it to 0 - 255, bringing it to world space, Is the 400 coming from the fact that the GM has a width & height of 400? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:257: SkVector3 fLightDir[SK_MAX_NON_AMBIENT_LIGHTS]; Shouldn't these other 4s be SK_MAX_NON_AMBIENT_LIGHTS too? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:292: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); You could just say: if (fAmbientColor != shadowFP.fAmbientColor || fNumDirLights != shadowFP.fNumDirLights) { return false; } and skip creating & using the isEqual boolean. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:296: for (int i = 0; i < fNumDirLights && isEqual; i++) { Similarly, in here you can just return false if any of the individual lights aren't equal https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:305: SK_MAX_NON_AMBIENT_LIGHTS ? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:323: Can you keep the method name next to the '->' and then wrap the parameters? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:329: Seems like you want some std::moves for diffuseFP & povDepthFP https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:389: } rm the printf https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:426: It seems like pegging NdotL is going to short circuit all lighting. Why even compute NdotL? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:517: size_t heapRequired = povDepthShader->contextSize(rec) + extra space ? https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:531: I don't think we need the '2' suffix here https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:537: extra '\n' here ?
https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:8: #include "gm.h" On 2016/07/27 18:52:58, robertphillips wrote: > Alphabetize the Sk* headers Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:14: #include "SkShadowShader.h" On 2016/07/27 18:52:58, robertphillips wrote: > Do we need SkBitmapProcShader.h ? Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:80: On 2016/07/27 18:52:58, robertphillips wrote: > space before the ':' > use INHERITED rather than SkPaintFilterCanvas > no ';' Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:86: SkColor color = 0xFF000000; // init color to opaque black On 2016/07/27 18:52:58, robertphillips wrote: > assert that z will fit in 8 bits ? Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:93: On 2016/07/27 18:52:58, robertphillips wrote: > Aren't all these onDraw* methods supposed to have an "override" suffix ? Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:94: void onDrawPicture(const SkPicture* picture, const SkMatrix* matrix, const SkPaint* paint) { On 2016/07/27 18:52:58, robertphillips wrote: > It seems like SkPaintFilterCanvas::onDrawPicture does all this for you? Why do > you need this? It doesn't work. I think because SkPaintFilterCanvas's onDrawPicture calls INHERITED::onDrawPicture instead of SKCanvas::onDrawPicture https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:95: SkTCopyOnFirstWrite<SkPaint> filteredPaint(paint); On 2016/07/27 18:52:58, robertphillips wrote: > this->onFilter Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:96: if (onFilter(&filteredPaint, kPicture_Type)) On 2016/07/27 18:52:58, robertphillips wrote: > Need enclosing brackets here > I think you still need the this->SkCanvas::... Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:254: typedef SkPaintFilterCanvas INHERITED; On 2016/07/27 18:52:58, robertphillips wrote: > extra '\n' here Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:295: On 2016/07/27 18:52:58, robertphillips wrote: > Why do we wrap 'canvas' in an SPFC? AFAICT we never draw to it as an SPFC. Done. Seemed like SPFCanvas and Canvas had different coordinate spaces. Interesting. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:306: // TODO: change to kIndex_8_SkColorType On 2016/07/27 18:52:58, robertphillips wrote: > rm this last TODO or change to A8 Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:307: On 2016/07/27 18:52:58, robertphillips wrote: > Maybe change kWidth & kHeight here to be kDefaultShadowMapWidth & > kDefaultShadowMapHeight > > ? What do you mean? The shadow map size should be dependent on the picture size and the light direction and the deepest draw depth of the picture. I have the formulas down, I'll implement it soon. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:311: On 2016/07/27 18:52:58, robertphillips wrote: > // Create a new surface (that matches the backend of SPFCanvas) for each shadow > map > > ? Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:346: On 2016/07/27 18:52:58, robertphillips wrote: > // Create a new surface (that matches the backend of 'canvas') to create the > povDepthMap > > ? Done. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:373: On 2016/07/27 18:52:58, robertphillips wrote: > Make this 2 lines: > > SkPaint paint; > paint.setShader(make_shadow_shader(povDepthMap, diffuseMap, fLights)); > > ? > > If not you should move the shadowShader into the paint. Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:67: On 2016/07/27 18:52:59, robertphillips wrote: > I think these two lines are supposed to be tabbed over Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:116: fGrContext = context; On 2016/07/27 17:46:04, jvanverth1 wrote: > Do you use this anywhere else? If not, you don't need to store it in the class. > If so, we usually just use the name fContext. Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:123: fNumDirLights++; On 2016/07/27 18:52:59, robertphillips wrote: > I think you need to index by fNumDirLights (in the LHS) in the following > assignments (the RHS remain being indexed by 'i'). This also means you need to > defer the increment of fNumDirLights until the end of this if-block. Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:128: // this sk_sp gets deleted when the ShadowFP is destroyed, and frees the GrTexture* On 2016/07/27 18:52:59, robertphillips wrote: > IIUC, asTextureRef returns a "GrTexture*" so I think you just need: > > fTexture[i].reset(shadowMap->...); > > I'm surprised the std::move isn't complaining. Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:130: GrTextureParams::ClampNoFilter(), On 2016/07/27 17:46:04, jvanverth1 wrote: > Still over 100 chars. Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:160: kVec3f_GrSLType, kDefault_GrSLPrecision, On 2016/07/27 17:46:04, jvanverth1 wrote: > Over 100 chars? Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:165: kVec3f_GrSLType, kDefault_GrSLPrecision, On 2016/07/27 17:46:04, jvanverth1 wrote: > Over 100 chars? Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:181: // povDepth.b * 255 scales it to 0 - 255, bringing it to world space, On 2016/07/27 18:52:59, robertphillips wrote: > Is the 400 coming from the fact that the GM has a width & height of 400? Yes. I'll pass this stuff in when I migrate to a Sample, it'll be much better to test with. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:257: SkVector3 fLightDir[SK_MAX_NON_AMBIENT_LIGHTS]; On 2016/07/27 18:52:59, robertphillips wrote: > Shouldn't these other 4s be SK_MAX_NON_AMBIENT_LIGHTS too? Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:292: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); On 2016/07/27 18:52:59, robertphillips wrote: > You could just say: > > if (fAmbientColor != shadowFP.fAmbientColor || fNumDirLights != > shadowFP.fNumDirLights) { > return false; > } > > and skip creating & using the isEqual boolean. Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:296: for (int i = 0; i < fNumDirLights && isEqual; i++) { On 2016/07/27 18:52:59, robertphillips wrote: > Similarly, in here you can just return false if any of the individual lights > aren't equal Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:305: On 2016/07/27 18:52:59, robertphillips wrote: > SK_MAX_NON_AMBIENT_LIGHTS ? Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:323: On 2016/07/27 18:52:59, robertphillips wrote: > Can you keep the method name next to the '->' and then wrap the parameters? Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:329: On 2016/07/27 18:52:59, robertphillips wrote: > Seems like you want some std::moves for diffuseFP & povDepthFP Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:330: sk_sp<GrFragmentProcessor> shadowfp = sk_make_sp<ShadowFP>(povDepthFP, diffuseFP, fLights, context); On 2016/07/27 17:46:04, jvanverth1 wrote: > 100 chars Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:389: } On 2016/07/27 18:52:59, robertphillips wrote: > rm the printf Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:426: On 2016/07/27 18:52:59, robertphillips wrote: > It seems like pegging NdotL is going to short circuit all lighting. Why even > compute NdotL? Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:517: size_t heapRequired = povDepthShader->contextSize(rec) + On 2016/07/27 18:52:59, robertphillips wrote: > extra space ? Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:531: On 2016/07/27 18:52:59, robertphillips wrote: > I don't think we need the '2' suffix here Done. https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:537: On 2016/07/27 18:52:59, robertphillips wrote: > extra '\n' here ? Done.
https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:94: void onDrawPicture(const SkPicture* picture, const SkMatrix* matrix, const SkPaint* paint) { On 2016/07/28 17:17:51, vjiaoblack wrote: > On 2016/07/27 18:52:58, robertphillips wrote: > > It seems like SkPaintFilterCanvas::onDrawPicture does all this for you? Why do > > you need this? > > It doesn't work. I think because SkPaintFilterCanvas's onDrawPicture calls > INHERITED::onDrawPicture instead of SKCanvas::onDrawPicture That's odd, it should ultimately wind up calling SkCanvas::drawPicture. https://codereview.chromium.org/2118553002/diff/120001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:307: On 2016/07/28 17:17:50, vjiaoblack wrote: > On 2016/07/27 18:52:58, robertphillips wrote: > > Maybe change kWidth & kHeight here to be kDefaultShadowMapWidth & > > kDefaultShadowMapHeight > > > > ? > > What do you mean? > The shadow map size should be dependent on the picture size and the light > direction and the deepest draw depth of the picture. > > I have the formulas down, I'll implement it soon. Right, since they aren't necessarily the same you can indicate that here by using a different constant (that for now can be hardwired to the same value). https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:181: // povDepth.b * 255 scales it to 0 - 255, bringing it to world space, On 2016/07/28 17:17:51, vjiaoblack wrote: > On 2016/07/27 18:52:59, robertphillips wrote: > > Is the 400 coming from the fact that the GM has a width & height of 400? > > Yes. I'll pass this stuff in when I migrate to a Sample, it'll be much better to > test with. Well, isn't the 400 value going to have to vary with each shadow map. That is, do you need to be passing it up as a uniform? For now, add a TODO? https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:79: bool onFilter(SkTCopyOnFirstWrite<SkPaint>* paint, Type type) const override { So, what happens when paint is nullptr ? https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:80: if (*paint) { So what happens if paint has an imageFilter or colorFilter attached? I suspect you want to create a new paint on the stack, set its color to the "z-color", copy over any path effects and then just replace the passed in paint with that. https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:309: SPFCanvas -> canvas now ? https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:322: std::move(curLight) ? https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:354: // set the depth map canvas to have the light as the user's POV std::move(povLight) ? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:125: fNumDirLights++; extra '\n' here ? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:150: for (int i = 0; i < numLights; i++) { do we still need this init ? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:157: here too ? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:194: fragBuilder->appendTextureLookup(&depthMaps[i], args.fTexSamplers[i], extra '\n' ? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:206: // Implement: diffColor * (ambientLightTot + foreachDirLight(lightColor * (N . L))) ? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:299: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); Can this entire if test fit on 1 line ? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:345: bool SkShadowShaderImpl::isOpaque() const { Does the povDepthShader's opacity actually matter? Shouldn't it always be opaque? https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:423: Is this implementing the same lighting model as the gpu?
https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:79: bool onFilter(SkTCopyOnFirstWrite<SkPaint>* paint, Type type) const override { On 2016/07/28 19:47:25, robertphillips wrote: > So, what happens when paint is nullptr ? Then nothing is being drawn, and since our current model doesn't handle anything else more complicated, we don't draw anything. I'll put in a TODO to change this to a shader. https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:80: if (*paint) { On 2016/07/28 19:47:25, robertphillips wrote: > So what happens if paint has an imageFilter or colorFilter attached? > > I suspect you want to create a new paint on the stack, set its color to the > "z-color", copy over any path effects and then just replace the passed in paint > with that. Done. https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:309: On 2016/07/28 19:47:25, robertphillips wrote: > SPFCanvas -> canvas now ? Done. https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:322: On 2016/07/28 19:47:25, robertphillips wrote: > std::move(curLight) ? Done. https://codereview.chromium.org/2118553002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:354: // set the depth map canvas to have the light as the user's POV On 2016/07/28 19:47:25, robertphillips wrote: > std::move(povLight) ? Done. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:125: fNumDirLights++; On 2016/07/28 19:47:25, robertphillips wrote: > extra '\n' here ? Done. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:150: for (int i = 0; i < numLights; i++) { On 2016/07/28 19:47:25, robertphillips wrote: > do we still need this init ? Done. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:157: On 2016/07/28 19:47:25, robertphillips wrote: > here too ? Done. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:194: fragBuilder->appendTextureLookup(&depthMaps[i], args.fTexSamplers[i], On 2016/07/28 19:47:25, robertphillips wrote: > extra '\n' ? Done. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:206: On 2016/07/28 19:47:25, robertphillips wrote: > // Implement: diffColor * (ambientLightTot + foreachDirLight(lightColor * (N . > L))) > > ? Done. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:299: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); On 2016/07/28 19:47:25, robertphillips wrote: > Can this entire if test fit on 1 line ? Done. The next can't tho. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:345: bool SkShadowShaderImpl::isOpaque() const { On 2016/07/28 19:47:25, robertphillips wrote: > Does the povDepthShader's opacity actually matter? Shouldn't it always be > opaque? Done. https://codereview.chromium.org/2118553002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:423: On 2016/07/28 19:47:25, robertphillips wrote: > Is this implementing the same lighting model as the gpu? Fixing.
https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:12: #include "SkSurface.h" This is out of order https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:86: SkPaint newPaint; I don't think you need the 'writable' here for an access https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:101: if (this->onFilter(&filteredPaint, kPicture_Type)) { // Add a comment here about why we have to skip the base class in the call to onDrawPicture https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:382: SkPaint paint; std::moves on povDepthMap & diffuseMap https://codereview.chromium.org/2118553002/diff/180001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2118553002/diff/180001/src/core/SkShadowShade... src/core/SkShadowShader.h:15: public: Fix comment. // This shader combines the diffuse color in 'diffuseShader' with the shadows determined by the 'povDepthShader' and the shadow maps stored in each of the lights in 'lights' ? There isn't really a shape here. This is just a shader that you're applying to a rect.
https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:12: #include "SkSurface.h" On 2016/07/29 16:30:52, robertphillips wrote: > This is out of order Done. https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:86: SkPaint newPaint; On 2016/07/29 16:30:52, robertphillips wrote: > I don't think you need the 'writable' here for an access Done. https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:101: if (this->onFilter(&filteredPaint, kPicture_Type)) { On 2016/07/29 16:30:52, robertphillips wrote: > // Add a comment here about why we have to skip the base class in the call to > onDrawPicture Done. https://codereview.chromium.org/2118553002/diff/180001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:382: SkPaint paint; On 2016/07/29 16:30:52, robertphillips wrote: > std::moves on povDepthMap & diffuseMap Done. https://codereview.chromium.org/2118553002/diff/180001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2118553002/diff/180001/src/core/SkShadowShade... src/core/SkShadowShader.h:15: public: On 2016/07/29 16:30:52, robertphillips wrote: > Fix comment. > > // This shader combines the diffuse color in 'diffuseShader' with the shadows > determined by the 'povDepthShader' and the shadow maps stored in each of the > lights in 'lights' > > ? > > There isn't really a shape here. This is just a shader that you're applying to a > rect. Done.
lgtm + nits https://codereview.chromium.org/2118553002/diff/200001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/200001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:101: if (this->onFilter(&filteredPaint, kPicture_Type)) { the base class's -> SkCanvas's ? https://codereview.chromium.org/2118553002/diff/200001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:114: // the light they intend to use for the current depth rendering. Assert here that the first light is a directional light ? Maybe that there is at least one too? https://codereview.chromium.org/2118553002/diff/200001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:140: int32_t numLights = args.fFp.cast<ShadowFP>().fNumDirLights; Maybe assert here that numLights <= SK_MAX_NON_AMBIENT_LIGHTS ?
https://codereview.chromium.org/2118553002/diff/200001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2118553002/diff/200001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:101: if (this->onFilter(&filteredPaint, kPicture_Type)) { On 2016/07/29 17:58:12, robertphillips wrote: > the base class's -> SkCanvas's ? Done. https://codereview.chromium.org/2118553002/diff/200001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:114: // the light they intend to use for the current depth rendering. On 2016/07/29 17:58:12, robertphillips wrote: > Assert here that the first light is a directional light ? > Maybe that there is at least one too? Done. https://codereview.chromium.org/2118553002/diff/200001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:140: int32_t numLights = args.fFp.cast<ShadowFP>().fNumDirLights; On 2016/07/29 17:58:12, robertphillips wrote: > Maybe assert here that numLights <= SK_MAX_NON_AMBIENT_LIGHTS ? Done.
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2118553002/#ps220001 (title: "made changes")
The CQ bit was unchecked by vjiaoblack@google.com
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2118553002/#ps240001 (title: "Disabled shadow maps")
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
A disapproval has been posted by following reviewers: jvanverth@google.com. Please make sure to get positive review before another CQ attempt.
lgtm
https://codereview.chromium.org/2118553002/diff/240001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2118553002/diff/240001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:30: : povDepthShader(std::move(povDepthShader)) fPovDepthShader https://codereview.chromium.org/2118553002/diff/240001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:303: fLightColor[i] == shadowFP.fLightColor[i]) { This should be a != ... fixing. https://codereview.chromium.org/2118553002/diff/240001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:449: sk_sp<SkFlattenable> SkShadowShaderImpl::CreateProc(SkReadBuffer& buf) { Add shadow maps into buf
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2118553002/#ps280001 (title: "Lines too long")
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 ========== adding new GM to demostrate new shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2118553002 ========== to ========== adding new GM to demostrate new shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2118553002 Committed: https://skia.googlesource.com/skia/+/53da5ba619553aa47dfe27c315f7deae389e6b07 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://skia.googlesource.com/skia/+/53da5ba619553aa47dfe27c315f7deae389e6b07 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
