Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2198933002:
Making a sample for shadow maps for more intensive development (Closed)
|
Created:
4 years, 4 months ago by vjiaoblack Modified:
4 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia@shadow-gm Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionMaking a sample for shadow maps for more intensive development
Merge branch 'shadow-gm' into shadow-sample
Added variable size shadow maps. Also fixed some bugs
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198933002
Committed: https://skia.googlesource.com/skia/+/955e879c6dc9c74224d5a25a67e9eecdee4d4ae8
Patch Set 1 #Patch Set 2 : Moved rendering code to canvas:onDrawShadowedPic #Patch Set 3 : fixed lights #Patch Set 4 : fixed lights #Patch Set 5 : undo change #
Total comments: 36
Patch Set 6 : made req changes, updating fLights in the sample is buggy #Patch Set 7 : Removed setDir() and re-added resizing shadow maps #
Total comments: 48
Patch Set 8 : Made requested changes #
Total comments: 30
Patch Set 9 : Made req chang #
Total comments: 12
Patch Set 10 : Made req changes #
Total comments: 7
Patch Set 11 : Made change to onCLick() as req., +quarantined code #Patch Set 12 : merge #Patch Set 13 : argh fixed utils include error #
Messages
Total messages: 44 (17 generated)
Description was changed from ========== Making a sample for shadow maps for more intensive development Merge branch 'shadow-gm' into shadow-sample Added variable size shadow maps. Also fixed some bugs BUG=skia: ========== to ========== Making a sample for shadow maps for more intensive development Merge branch 'shadow-gm' into shadow-sample Added variable size shadow maps. Also fixed some bugs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198933002 ==========
https://codereview.chromium.org/2198933002/diff/80001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2198933002/diff/80001/gm/shadowmaps.cpp#newco... gm/shadowmaps.cpp:10: #include "SkPaintFilterCanvas.h" alphabetize Also, I expect an addition to utils.gypi https://codereview.chromium.org/2198933002/diff/80001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2198933002/diff/80001/include/core/SkLights.h... include/core/SkLights.h:57: The entire idea of having the builder and then built object is to have the built object be immutable after it is constructed! https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:32: Will this fit on one line? https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:63: Shouldn't this now read something like: // The user needs to update the lights so that ... ? https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:74: What happened to all the overrides? https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:225: typedef SkPaintFilterCanvas INHERITED; extra '\n' here ? https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:9: #include "SampleCode.h" Do we need SkDrawFilter.h ? https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:11: #include "SkNormalSource.h" Same for SkPathEffect.h https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:41: public: add INHERITED and use it here https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:69: don't need this-> on member variables https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:110: This is file static naming. You either need to make it file static or change the style. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:140: SkTMin ? https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:144: // It's used to generate the depth maps. Do you need to recreate this every time ? https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:146: Don't you only need to recreate the shadow map if a light or part of the scene moves ? https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:244: if (fMoveLight) { Rebuild the object here ... https://codereview.chromium.org/2198933002/diff/80001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/80001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3015: Rm this ?
https://codereview.chromium.org/2198933002/diff/80001/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.chromium.org/2198933002/diff/80001/gyp/common_variables.gy... gyp/common_variables.gypi:257: 'skia_experimental_shadowing': 1, # for experimental shadow-drawing Don't forget to disable this. https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:9: #define SkShadowPaintFilterCanvas_DEFINED Does this need to be inside an #ifdef SK_EXPERIMENTAL_SHADOWING block?
https://codereview.chromium.org/2198933002/diff/80001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2198933002/diff/80001/gm/shadowmaps.cpp#newco... gm/shadowmaps.cpp:10: #include "SkPaintFilterCanvas.h" On 2016/08/02 16:05:06, robertphillips wrote: > alphabetize > > Also, I expect an addition to utils.gypi Done. https://codereview.chromium.org/2198933002/diff/80001/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.chromium.org/2198933002/diff/80001/gyp/common_variables.gy... gyp/common_variables.gypi:257: 'skia_experimental_shadowing': 1, # for experimental shadow-drawing On 2016/08/02 17:43:33, jvanverth1 wrote: > Don't forget to disable this. I will! Figured I might as well leave it on up until I commit. https://codereview.chromium.org/2198933002/diff/80001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2198933002/diff/80001/include/core/SkLights.h... include/core/SkLights.h:57: On 2016/08/02 16:05:06, robertphillips wrote: > The entire idea of having the builder and then built object is to have the built > object be immutable after it is constructed! Done. https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:9: #define SkShadowPaintFilterCanvas_DEFINED On 2016/08/02 17:43:33, jvanverth1 wrote: > Does this need to be inside an #ifdef SK_EXPERIMENTAL_SHADOWING block? Hmm... I guess it would make sense for it to be https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:32: On 2016/08/02 16:05:06, robertphillips wrote: > Will this fit on one line? Yeah, but I moved some of the code around and this should now be a non-issue https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:63: On 2016/08/02 16:05:06, robertphillips wrote: > Shouldn't this now read something like: > > // The user needs to update the lights so that ... > > ? Done. https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:74: On 2016/08/02 16:05:06, robertphillips wrote: > What happened to all the overrides? Currently, I can't have them. (after the refactoring this out into a cpp and an h) https://codereview.chromium.org/2198933002/diff/80001/include/utils/SkShadowP... include/utils/SkShadowPaintFilterCanvas.h:225: typedef SkPaintFilterCanvas INHERITED; On 2016/08/02 16:05:06, robertphillips wrote: > extra '\n' here ? Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:9: #include "SampleCode.h" On 2016/08/02 16:05:06, robertphillips wrote: > Do we need SkDrawFilter.h ? Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:11: #include "SkNormalSource.h" On 2016/08/02 16:05:06, robertphillips wrote: > Same for SkPathEffect.h Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:41: public: On 2016/08/02 16:05:06, robertphillips wrote: > add INHERITED and use it here Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:69: On 2016/08/02 16:05:07, robertphillips wrote: > don't need this-> on member variables Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:110: On 2016/08/02 16:05:06, robertphillips wrote: > This is file static naming. You either need to make it file static or change the > style. Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:140: On 2016/08/02 16:05:07, robertphillips wrote: > SkTMin ? Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:144: // It's used to generate the depth maps. On 2016/08/02 16:05:06, robertphillips wrote: > Do you need to recreate this every time ? Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:146: On 2016/08/02 16:05:07, robertphillips wrote: > Don't you only need to recreate the shadow map if a light or part of the scene > moves ? Done. https://codereview.chromium.org/2198933002/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:244: if (fMoveLight) { On 2016/08/02 16:05:07, robertphillips wrote: > Rebuild the object here ... Done. https://codereview.chromium.org/2198933002/diff/80001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/80001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3015: On 2016/08/02 16:05:07, robertphillips wrote: > Rm this ? Done.
vjiaoblack@google.com changed reviewers: - brianosman@google.com, bsalomon@google.com, reed@google.com
https://codereview.chromium.org/2198933002/diff/120001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2198933002/diff/120001/include/core/SkLights.... include/core/SkLights.h:65: For better or for worse, Skia doesn't usually return sk_sp from accessors. https://codereview.chromium.org/2198933002/diff/120001/include/utils/SkShadow... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/120001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:10: I think all we need to include is SkPaintFilterCanvas.h https://codereview.chromium.org/2198933002/diff/120001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:37: I think all the onDraw*s are supposed to be protected https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:7: Why gm.h? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:16: Can this be a class scope constant? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:36: class IndexClick : public SkView::Click { move this to the private block ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:49: class ShadowingView : public SampleView { Make this "static const int kZoom = 96;" down in the private block ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:85: protected: Can all these member variables be private ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:87: static const int kHeight = 400; Maybe have: struct { SkRect fGeometry; int fDepth; SkColor fColor; } fTestRects[kNumTestRects]; https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:90: SkColor fColors[NUM_TEST_RECTS]; You initialize all the other guys in the ctor. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:162: // TODO: find actual max depth of picture Can this computation be a sub routine: static SkISize compute_dmap_size(int maxDepth, const SkLights::Light&, int width, int height); ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:164: int dMapWidth = SkMin32(maxDepth * fabs(fLights->light(i).dir().fX) + kWidth, line up ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:166: int dMapHeight = SkMin32(maxDepth * fabs(fLights->light(i).dir().fY) + kHeight, line up ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:232: rm this TODO ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:235: if (fUpdatePicture || !fPicture) { add this-> for member function calls here & below ? https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:255: SkPaint paint; I don't think you want to std::move here https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:296: return true; extra '\n' https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:316: It seems like you actually have two states: fSceneChanged fLightsChanged In the former, all the things need to be updated. In the latter only the light's shadow maps need to be updated. https://codereview.chromium.org/2198933002/diff/120001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2198933002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.h:13: Can this class scoped ? https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... File src/utils/SkShadowPaintFilterCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:7: I think we only need the "SkShadowPaintFilterCanvas.h" include https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:40: if (onFilter(&filteredPaint, kPicture_Type)) Add brackets around this call https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:49: if (this->fLights->light(0).type() != SkLights::Light::kAmbient_LightType) { const SkVector3& lightDir = ... ? https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:59: void SkShadowPaintFilterCanvas::onDrawPaint(const SkPaint &paint) { add this-> to member function calls here and elsewhere
https://codereview.chromium.org/2198933002/diff/120001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2198933002/diff/120001/include/core/SkLights.... include/core/SkLights.h:65: On 2016/08/03 18:47:47, robertphillips wrote: > For better or for worse, Skia doesn't usually return sk_sp from accessors. Ah. That must've been why this change was made, then. (I don't remember it being just an SkImage*.) I'll make the changes in my code. https://codereview.chromium.org/2198933002/diff/120001/include/utils/SkShadow... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/120001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:10: On 2016/08/03 18:47:47, robertphillips wrote: > I think all we need to include is SkPaintFilterCanvas.h Done. https://codereview.chromium.org/2198933002/diff/120001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:37: On 2016/08/03 18:47:47, robertphillips wrote: > I think all the onDraw*s are supposed to be protected Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:7: On 2016/08/03 18:47:48, robertphillips wrote: > Why gm.h? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:16: On 2016/08/03 18:47:47, robertphillips wrote: > Can this be a class scope constant? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:36: class IndexClick : public SkView::Click { On 2016/08/03 18:47:47, robertphillips wrote: > move this to the private block ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:49: class ShadowingView : public SampleView { On 2016/08/03 18:47:48, robertphillips wrote: > Make this "static const int kZoom = 96;" down in the private block ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:85: protected: On 2016/08/03 18:47:48, robertphillips wrote: > Can all these member variables be private ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:87: static const int kHeight = 400; On 2016/08/03 18:47:48, robertphillips wrote: > Maybe have: > > struct { > SkRect fGeometry; > int fDepth; > SkColor fColor; > } fTestRects[kNumTestRects]; Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:90: SkColor fColors[NUM_TEST_RECTS]; On 2016/08/03 18:47:48, robertphillips wrote: > You initialize all the other guys in the ctor. Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:162: // TODO: find actual max depth of picture On 2016/08/03 18:47:48, robertphillips wrote: > Can this computation be a sub routine: > > static SkISize compute_dmap_size(int maxDepth, const SkLights::Light&, int > width, int height); > > ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:164: int dMapWidth = SkMin32(maxDepth * fabs(fLights->light(i).dir().fX) + kWidth, On 2016/08/03 18:47:47, robertphillips wrote: > line up ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:166: int dMapHeight = SkMin32(maxDepth * fabs(fLights->light(i).dir().fY) + kHeight, On 2016/08/03 18:47:48, robertphillips wrote: > line up ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:232: On 2016/08/03 18:47:48, robertphillips wrote: > rm this TODO ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:235: if (fUpdatePicture || !fPicture) { On 2016/08/03 18:47:48, robertphillips wrote: > add this-> for member function calls here & below ? Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:255: SkPaint paint; On 2016/08/03 18:47:48, robertphillips wrote: > I don't think you want to std::move here I see. Since we need it next frame as well (perhaps) https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:296: return true; On 2016/08/03 18:47:48, robertphillips wrote: > extra '\n' Done. https://codereview.chromium.org/2198933002/diff/120001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:316: On 2016/08/03 18:47:48, robertphillips wrote: > It seems like you actually have two states: > > fSceneChanged > fLightsChanged > > In the former, all the things need to be updated. > In the latter only the light's shadow maps need to be updated. Done. Seems like the slowest part of the loop is still the light Builder thing. https://codereview.chromium.org/2198933002/diff/120001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2198933002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.h:13: On 2016/08/03 18:47:48, robertphillips wrote: > Can this class scoped ? Done. https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... File src/utils/SkShadowPaintFilterCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:7: On 2016/08/03 18:47:48, robertphillips wrote: > I think we only need the "SkShadowPaintFilterCanvas.h" include Done. https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:40: if (onFilter(&filteredPaint, kPicture_Type)) On 2016/08/03 18:47:48, robertphillips wrote: > Add brackets around this call Done. https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:49: if (this->fLights->light(0).type() != SkLights::Light::kAmbient_LightType) { On 2016/08/03 18:47:48, robertphillips wrote: > const SkVector3& lightDir = ... ? Done. https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:59: void SkShadowPaintFilterCanvas::onDrawPaint(const SkPaint &paint) { On 2016/08/03 18:47:48, robertphillips wrote: > add this-> to member function calls here and elsewhere Done.
https://codereview.chromium.org/2198933002/diff/140001/include/utils/SkShadow... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/140001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:14: class is now named differently https://codereview.chromium.org/2198933002/diff/140001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:16: It's not really a base class https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:11: #include "SkSurface.h" out of order here https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:38: We don't seem to use this entry point. Do we ever use fIndex either? Can we just create a raw SkView::Click object? https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:99: This is file scope syntax. Either move to file scope or switch to in-class syntax. I would suggest making this a static helper function in SkLights. Then we could reuse it in the GM. I would put the light parameter first as it seems to be the most important. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:101: int width, int height) { assert it isn't an ambient light? https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:227: [Optional] It looks like we keep recreating the shadow shader all the time. Maybe: if (fSceneChanged || fLightsChanged || !fShadowShader) { fShadowShader = make_shadow_shader(); } SkPaint paint; paint.setShader(fShadowShader); https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:235: What does the base class' implementation do? https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:249: if (fMoveLight) { Shouldn't this be an || ? https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:277: if (fSelectedRect > -1) { Didn't we already store off/compute these above ? https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:283: fTestRects[fSelectedRect].offset(dx, dy); ? https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:297: fSelectedRect = i; fTestRects[i].offset(dx, dy); ? https://codereview.chromium.org/2198933002/diff/140001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2198933002/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:16: Can this actually go inside SkShadowShaderImpl ? https://codereview.chromium.org/2198933002/diff/140001/src/utils/SkShadowPain... File src/utils/SkShadowPaintFilterCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/140001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:37: SkTCopyOnFirstWrite<SkPaint> filteredPaint(paint); this->onFilter
https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... File src/utils/SkShadowPaintFilterCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:49: if (this->fLights->light(0).type() != SkLights::Light::kAmbient_LightType) { On 2016/08/03 18:47:48, robertphillips wrote: > const SkVector3& lightDir = ... ? Done. https://codereview.chromium.org/2198933002/diff/120001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:59: void SkShadowPaintFilterCanvas::onDrawPaint(const SkPaint &paint) { On 2016/08/03 18:47:48, robertphillips wrote: > add this-> to member function calls here and elsewhere Done. https://codereview.chromium.org/2198933002/diff/140001/include/utils/SkShadow... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/140001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:14: On 2016/08/04 15:17:35, robertphillips wrote: > class is now named differently Done. https://codereview.chromium.org/2198933002/diff/140001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:16: On 2016/08/04 15:17:35, robertphillips wrote: > It's not really a base class Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:11: #include "SkSurface.h" On 2016/08/04 15:17:35, robertphillips wrote: > out of order here Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:38: On 2016/08/04 15:17:35, robertphillips wrote: > We don't seem to use this entry point. > > Do we ever use fIndex either? Can we just create a raw SkView::Click object? Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:99: On 2016/08/04 15:17:35, robertphillips wrote: > This is file scope syntax. Either move to file scope or switch to in-class > syntax. > > I would suggest making this a static helper function in SkLights. Then we could > reuse it in the GM. > > I would put the light parameter first as it seems to be the most important. Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:101: int width, int height) { On 2016/08/04 15:17:35, robertphillips wrote: > assert it isn't an ambient light? Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:227: On 2016/08/04 15:17:35, robertphillips wrote: > [Optional] It looks like we keep recreating the shadow shader all the time. > Maybe: > > if (fSceneChanged || fLightsChanged || !fShadowShader) { > fShadowShader = make_shadow_shader(); > } > > SkPaint paint; > paint.setShader(fShadowShader); Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:227: On 2016/08/04 15:17:35, robertphillips wrote: > [Optional] It looks like we keep recreating the shadow shader all the time. > Maybe: > > if (fSceneChanged || fLightsChanged || !fShadowShader) { > fShadowShader = make_shadow_shader(); > } > > SkPaint paint; > paint.setShader(fShadowShader); Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:235: On 2016/08/04 15:17:35, robertphillips wrote: > What does the base class' implementation do? I don't think there's a base class' implementation, is there? It's just all .h Anyways, I've changed this to just return a Click* and it seemed to work https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:235: On 2016/08/04 15:17:35, robertphillips wrote: > What does the base class' implementation do? Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:249: if (fMoveLight) { On 2016/08/04 15:17:35, robertphillips wrote: > Shouldn't this be an || ? Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:277: if (fSelectedRect > -1) { On 2016/08/04 15:17:35, robertphillips wrote: > Didn't we already store off/compute these above ? Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:283: On 2016/08/04 15:17:35, robertphillips wrote: > fTestRects[fSelectedRect].offset(dx, dy); ? Done. https://codereview.chromium.org/2198933002/diff/140001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:297: fSelectedRect = i; On 2016/08/04 15:17:35, robertphillips wrote: > fTestRects[i].offset(dx, dy); ? Done. https://codereview.chromium.org/2198933002/diff/140001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2198933002/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:16: On 2016/08/04 15:17:35, robertphillips wrote: > Can this actually go inside SkShadowShaderImpl ? Done. https://codereview.chromium.org/2198933002/diff/140001/src/utils/SkShadowPain... File src/utils/SkShadowPaintFilterCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/140001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:37: SkTCopyOnFirstWrite<SkPaint> filteredPaint(paint); On 2016/08/04 15:17:35, robertphillips wrote: > this->onFilter Done.
https://codereview.chromium.org/2198933002/diff/160001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2198933002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:120: Use our new helper here ? https://codereview.chromium.org/2198933002/diff/160001/include/utils/SkShadow... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/160001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:34: This needs class-scope static syntax i.e., ComputeDepthMapSize https://codereview.chromium.org/2198933002/diff/160001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/160001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:7: I don't think we need sk_tool_utils.h https://codereview.chromium.org/2198933002/diff/160001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2198933002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.h:27: // The shadow shader supports any number of ambient lights but only kMaxNonAmbientLights. Non-ambient lights beyond the maximum will be ignored. ? https://codereview.chromium.org/2198933002/diff/160001/src/utils/SkShadowPain... File src/utils/SkShadowPaintFilterCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/160001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:35: SkISize SkShadowPaintFilterCanvas::computeDepthMapSize(const SkLights::Light& light, int maxDepth, line these guys up with the '(' ? https://codereview.chromium.org/2198933002/diff/160001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:54: void SkShadowPaintFilterCanvas::updateMatrix() { this->save()
https://codereview.chromium.org/2198933002/diff/160001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2198933002/diff/160001/gm/shadowmaps.cpp#newc... gm/shadowmaps.cpp:120: On 2016/08/04 18:16:06, robertphillips wrote: > Use our new helper here ? Done. https://codereview.chromium.org/2198933002/diff/160001/include/utils/SkShadow... File include/utils/SkShadowPaintFilterCanvas.h (right): https://codereview.chromium.org/2198933002/diff/160001/include/utils/SkShadow... include/utils/SkShadowPaintFilterCanvas.h:34: On 2016/08/04 18:16:06, robertphillips wrote: > This needs class-scope static syntax i.e., ComputeDepthMapSize Done. https://codereview.chromium.org/2198933002/diff/160001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/160001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:7: On 2016/08/04 18:16:06, robertphillips wrote: > I don't think we need sk_tool_utils.h Done. https://codereview.chromium.org/2198933002/diff/160001/src/core/SkShadowShader.h File src/core/SkShadowShader.h (right): https://codereview.chromium.org/2198933002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.h:27: On 2016/08/04 18:16:06, robertphillips wrote: > // The shadow shader supports any number of ambient lights but only > kMaxNonAmbientLights. Non-ambient lights beyond the maximum will be ignored. > > ? Currently, I've been told to use SkAssert instead of just ignoring, so i'll put that into comment! https://codereview.chromium.org/2198933002/diff/160001/src/utils/SkShadowPain... File src/utils/SkShadowPaintFilterCanvas.cpp (right): https://codereview.chromium.org/2198933002/diff/160001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:35: SkISize SkShadowPaintFilterCanvas::computeDepthMapSize(const SkLights::Light& light, int maxDepth, On 2016/08/04 18:16:06, robertphillips wrote: > line these guys up with the '(' ? Done. https://codereview.chromium.org/2198933002/diff/160001/src/utils/SkShadowPain... src/utils/SkShadowPaintFilterCanvas.cpp:54: void SkShadowPaintFilterCanvas::updateMatrix() { On 2016/08/04 18:16:06, robertphillips wrote: > this->save() Done.
lgtm. Other onClick handlers seem to invalue the screen when the scene changes. https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:244: this->inval(nullptr); ? https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:257: this->inval(nullptr); ? https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:267: fSceneChanged = true; this->inval(nullptr); ?
https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:244: On 2016/08/04 19:16:17, robertphillips wrote: > this->inval(nullptr); ? Done. https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:244: On 2016/08/04 19:16:17, robertphillips wrote: > this->inval(nullptr); ? Done. https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:257: On 2016/08/04 19:16:17, robertphillips wrote: > this->inval(nullptr); ? Done. https://codereview.chromium.org/2198933002/diff/180001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:267: fSceneChanged = true; On 2016/08/04 19:16:17, robertphillips wrote: > this->inval(nullptr); ? 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/2198933002/#ps200001 (title: "Made change to onCLick() as req., +quarantined code")
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: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
rebased with master
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/2198933002/#ps220001 (title: "merge")
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...)
vjiaoblack@google.com changed reviewers: + djsollen@google.com, reed@google.com
hallo! Making change to public API, so I need a lgtm from a swagmaster
On 2016/08/05 13:50:31, vjiaoblack wrote: > hallo! > Making change to public API, so I need a lgtm from a swagmaster It looks like you might have some build configuration issues as well.
I'll defer to Mike on this public API change, but is there a reason for this to be a public include at this point?
On 2016/08/05 14:01:13, djsollen wrote: > I'll defer to Mike on this public API change, but is there a reason for this to > be a public include at this point? I may be doing this wrong, but - I'm pretty sure the gyp variable prevents it from actually being public? There's include guards around my code
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
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/2198933002/#ps240001 (title: "argh fixed utils include error")
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 ========== Making a sample for shadow maps for more intensive development Merge branch 'shadow-gm' into shadow-sample Added variable size shadow maps. Also fixed some bugs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198933002 ========== to ========== Making a sample for shadow maps for more intensive development Merge branch 'shadow-gm' into shadow-sample Added variable size shadow maps. Also fixed some bugs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198933002 Committed: https://skia.googlesource.com/skia/+/955e879c6dc9c74224d5a25a67e9eecdee4d4ae8 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://skia.googlesource.com/skia/+/955e879c6dc9c74224d5a25a67e9eecdee4d4ae8 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
