Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220633002:
moved code into onDrawShadowedPic, only renders into shadow maps if needed (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@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionmoved code into onDrawShadowedPic, only renders into shadow maps if needed
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2220633002
Committed: https://skia.googlesource.com/skia/+/904527d165ce98c9fbaa9c05d3890070e3132170
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added 'd' interrupt to force depth map redraw on switch to gpu" #Patch Set 3 : fixed small crumbs #
Total comments: 36
Patch Set 4 : made req changes #
Total comments: 8
Patch Set 5 : Made req changes #Patch Set 6 : Fixed incorrect handling of paint and matrix in onDrawShadowedPic #
Total comments: 2
Patch Set 7 : updated light pos #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== moved code into onDrawShadowedPic, only renders into shadow maps if needed BUG=skia: ========== to ========== moved code into onDrawShadowedPic, only renders into shadow maps if needed BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2220633002 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (left): https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:200: this->updateDepthMaps(canvas); Any thoughts on how you'd manage this caching with drawShadowedPicture? https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:62: fClearShadowMaps = true; Should this be a toggle? How do you set it to false? https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:186: bool fClearShadowMaps; This is never initialized. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:40: #include <SkShadowPaintFilterCanvas.h> No <>, put up with the other Sk includes. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3072: #include "GrDrawContext.h" This belongs at the top of the file in the SK_SUPPORT_GPU block. Though it's not clear to me why it's needed at all.
https://codereview.chromium.org/2220633002/diff/1/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2220633002/diff/1/src/core/SkCanvas.cpp#newco... src/core/SkCanvas.cpp:39: #include <new> "" style - not <> style https://codereview.chromium.org/2220633002/diff/40001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/gm/shadowmaps.cpp#newco... gm/shadowmaps.cpp:98: SkPaint paint; pass in nullptr for the paint parameter ? https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:60: break; // Comment about why you need this here ? https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:113: canvas->setLights(fLights); pass in nullptr for paint argument ? https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:196: rm fPovDepthMap, fDiffuseMap & fShadowShader ? https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:39: #include <new> put with others https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:45: #include "SkGrPriv.h" put with the Sk headers ? https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3071: put with Gr headers ? https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3072: #include "GrDrawContext.h" Need to handle matrix parameter too https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3088: SkISize shMapSize = SkShadowPaintFilterCanvas:: put the method name on the same line as its class https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3162: handle nullptr parameter here https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3165: sk_sp<SkShader> povDepthShader = povDepthMap->makeShader(SkShader::kClamp_TileMode, line up https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3168: sk_sp<SkShader> diffuseShader = diffuseMap->makeShader(SkShader::kClamp_TileMode, line up https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3179: this->drawRect(SkRect::MakeIWH(diffuseMap->width(), diffuseMap->height()), paint2); extra \ns
https://codereview.chromium.org/2220633002/diff/40001/gm/shadowmaps.cpp File gm/shadowmaps.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/gm/shadowmaps.cpp#newco... gm/shadowmaps.cpp:98: SkPaint paint; On 2016/08/08 14:18:31, robertphillips wrote: > pass in nullptr for the paint parameter ? Done. https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (left): https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:200: this->updateDepthMaps(canvas); On 2016/08/08 14:15:47, jvanverth1 wrote: > Any thoughts on how you'd manage this caching with drawShadowedPicture? yeah, we just only cache when there is no change at all. If anything changes (lights, scene, etc) we have to redraw everything. https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:60: break; On 2016/08/08 14:18:31, robertphillips wrote: > // Comment about why you need this here ? Done. https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:62: fClearShadowMaps = true; On 2016/08/08 14:15:47, jvanverth1 wrote: > Should this be a toggle? How do you set it to false? When the if (fClearShadowMaps) triggers, I edited it to make it turn it back to false. https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:113: canvas->setLights(fLights); On 2016/08/08 14:18:31, robertphillips wrote: > pass in nullptr for paint argument ? Done. https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:186: bool fClearShadowMaps; On 2016/08/08 14:15:47, jvanverth1 wrote: > This is never initialized. Done. https://codereview.chromium.org/2220633002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:196: On 2016/08/08 14:18:31, robertphillips wrote: > rm fPovDepthMap, fDiffuseMap & fShadowShader ? Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:39: #include <new> On 2016/08/08 14:18:31, robertphillips wrote: > put with others Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:40: #include <SkShadowPaintFilterCanvas.h> On 2016/08/08 14:15:47, jvanverth1 wrote: > No <>, put up with the other Sk includes. Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:45: #include "SkGrPriv.h" On 2016/08/08 14:18:32, robertphillips wrote: > put with the Sk headers ? Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3071: On 2016/08/08 14:18:32, robertphillips wrote: > put with Gr headers ? Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3072: #include "GrDrawContext.h" On 2016/08/08 14:18:31, robertphillips wrote: > Need to handle matrix parameter too Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3072: #include "GrDrawContext.h" On 2016/08/08 14:15:47, jvanverth1 wrote: > This belongs at the top of the file in the SK_SUPPORT_GPU block. Though it's not > clear to me why it's needed at all. Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3088: SkISize shMapSize = SkShadowPaintFilterCanvas:: On 2016/08/08 14:18:32, robertphillips wrote: > put the method name on the same line as its class Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3162: On 2016/08/08 14:18:31, robertphillips wrote: > handle nullptr parameter here Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3165: sk_sp<SkShader> povDepthShader = povDepthMap->makeShader(SkShader::kClamp_TileMode, On 2016/08/08 14:18:31, robertphillips wrote: > line up Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3168: sk_sp<SkShader> diffuseShader = diffuseMap->makeShader(SkShader::kClamp_TileMode, On 2016/08/08 14:18:32, robertphillips wrote: > line up Done. https://codereview.chromium.org/2220633002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3179: this->drawRect(SkRect::MakeIWH(diffuseMap->width(), diffuseMap->height()), paint2); On 2016/08/08 14:18:32, robertphillips wrote: > extra \ns Done.
https://codereview.chromium.org/2220633002/diff/60001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2220633002/diff/60001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:63: case 'd': // Raster generated shadow maps have their origin in the UL corner while GPU generated shadow maps can have an arbitrary origin. For now, the origin of the shadow map needs to match the origin of the final canvas. Override the 'd' keypress so that, when the device is cycled, the shadow maps will be re-generated for the new backend ? https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:46: #include "SkGrPriv.h" stray '\n' https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3087: Use the SkAutoCanvasMatrixPaint object. You can mess up the MCRec stack in this call. I would move this down to where you actually use 'this' canvas to draw. https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3177: Look at the documentation for drawPicture for what the 'paint' parameter is supposed to do. SkAutoCanvasMatrixPaint is your friend.
https://codereview.chromium.org/2220633002/diff/60001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2220633002/diff/60001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:63: case 'd': On 2016/08/08 16:28:25, robertphillips wrote: > // Raster generated shadow maps have their origin in the UL corner while GPU > generated shadow maps can have an arbitrary origin. For now, the origin of the > shadow map needs to match the origin of the final canvas. Override the 'd' > keypress so that, when the device is cycled, the shadow maps will be > re-generated for the new backend > > ? Done. https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:46: #include "SkGrPriv.h" On 2016/08/08 16:28:25, robertphillips wrote: > stray '\n' Done. https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3087: On 2016/08/08 16:28:25, robertphillips wrote: > Use the SkAutoCanvasMatrixPaint object. You can mess up the MCRec stack in this > call. > > I would move this down to where you actually use 'this' canvas to draw. Done. https://codereview.chromium.org/2220633002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:3177: On 2016/08/08 16:28:25, robertphillips wrote: > Look at the documentation for drawPicture for what the 'paint' parameter is > supposed to do. SkAutoCanvasMatrixPaint is your friend. seems like SkAutoCanvasMatrixPaint doesn't handle it, so this is correct?
lgtm https://codereview.chromium.org/2220633002/diff/100001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2220633002/diff/100001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:63: case 'd': have their origin ?
https://codereview.chromium.org/2220633002/diff/100001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2220633002/diff/100001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:63: case 'd': On 2016/08/09 14:50:37, robertphillips wrote: > have their origin ? Done.
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/2220633002/#ps120001 (title: "updated light pos")
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 ========== moved code into onDrawShadowedPic, only renders into shadow maps if needed BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2220633002 ========== to ========== moved code into onDrawShadowedPic, only renders into shadow maps if needed BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2220633002 Committed: https://skia.googlesource.com/skia/+/904527d165ce98c9fbaa9c05d3890070e3132170 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/904527d165ce98c9fbaa9c05d3890070e3132170 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
