Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2297693002:
made raster shadows variance shadows (Closed)
|
Created:
4 years, 3 months ago by vjiaoblack Modified:
4 years, 3 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionmade raster shadows variance shadows
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2297693002
Committed: https://skia.googlesource.com/skia/+/fdf2986b1965dfc6a6067858e558914d6103d43a
Patch Set 1 #
Total comments: 7
Patch Set 2 : made req changes #Patch Set 3 : made req changes #Messages
Total messages: 16 (8 generated)
Description was changed from ========== made raster shadows variance shadows BUG=skia: ========== to ========== made raster shadows variance shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2297693002 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
What do you guys think about this?
https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:743: shDepth = pvDepth; It seems like you don't have to compute the square b.c. it will never be used. https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:746: I expect a "if (blurAlgorithm == SkShadowParams::kVariance_ShadowType) {" test somewhere in here https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:754: This formula is a bit hard to parse with the given formatting.
https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:743: shDepth = pvDepth; On 2016/08/30 16:48:37, robertphillips wrote: > It seems like you don't have to compute the square b.c. it will never be used. Ah right. I guess it'll fail the next if case https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:746: On 2016/08/30 16:48:37, robertphillips wrote: > I expect a "if (blurAlgorithm == SkShadowParams::kVariance_ShadowType) {" test > somewhere in here That doesn't need to be here. If the shadow map isn't blurred, the shadows will be correctly sharp. https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:754: On 2016/08/30 16:48:37, robertphillips wrote: > This formula is a bit hard to parse with the given formatting. Done.
https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2297693002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:746: The variance path is far more expensive than the non-variance path. If one wanted to profile the performance cost of the variance math vs. plain, they would have no way to do so.
lgtm
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
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 ========== made raster shadows variance shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2297693002 ========== to ========== made raster shadows variance shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2297693002 Committed: https://skia.googlesource.com/skia/+/fdf2986b1965dfc6a6067858e558914d6103d43a ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/fdf2986b1965dfc6a6067858e558914d6103d43a |
