Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2298603004:
added point light attenuations to raster (Closed)
|
Created:
4 years, 3 months ago by vjiaoblack Modified:
4 years, 3 months ago CC:
reviews_skia.org Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionadded point light attenuations to raster
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2298603004
Committed: https://skia.googlesource.com/skia/+/bcdc405ea07e44e4bd54b6cdcade300161a330a3
Patch Set 1 #
Total comments: 12
Patch Set 2 : made req changes #
Total comments: 1
Patch Set 3 : added i back to light direction calculation #Messages
Total messages: 19 (8 generated)
Description was changed from ========== added point light attenuations to raster BUG=skia: ========== to ========== added point light attenuations to raster BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2298603004 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
It's hella slow though - moved the sample fps from around 33 to 55
On 2016/08/31 13:29:51, vjiaoblack wrote: > It's hella slow though - moved the sample fps from around 33 to 55 *ms per frame from 33 to 55
https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:768: Maybe: } else { SkASSERT(light.type() == SkLights::Light::kPoint_LightType); ? https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:769: } else if (light.type() == SkLights::Light::kPoint_LightType) { Move this line out of the if-then block and share with the prior block ? https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:772: SkVector3 fragToLight = SkVector3::Make(light.pos().fX - (x + i), where's this 400 coming from? https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:781: // assume object normals are pointing straight up
https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:768: On 2016/08/31 13:44:27, robertphillips wrote: > Maybe: > > } else { > SkASSERT(light.type() == SkLights::Light::kPoint_LightType); > > ? Done. https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:769: } else if (light.type() == SkLights::Light::kPoint_LightType) { On 2016/08/31 13:44:27, robertphillips wrote: > Move this line out of the if-then block and share with the prior block ? Done. https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:772: SkVector3 fragToLight = SkVector3::Make(light.pos().fX - (x + i), On 2016/08/31 13:44:27, robertphillips wrote: > where's this 400 coming from? Done. https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:781: On 2016/08/31 13:44:27, robertphillips wrote: > // assume object normals are pointing straight up Done.
Using forward differencing to compute the vector to the light might be slightly faster. I.e., compute the direction vectors for the first and last pixels, then use that to compute the delta for each pixel. As you step from pixel to pixel, add the delta to get the new light direction. https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:779: SkScalar attenuationValue = dist / light.intensity(); This is the old lighting equation. https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:782: totalLight.fX += normalizedZ * light.color().fX / attenuationValue; If you precompute 1/attenuation and multiply by that, that should help perf.
https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:779: SkScalar attenuationValue = dist / light.intensity(); On 2016/08/31 17:18:23, jvanverth1 wrote: > This is the old lighting equation. Done. https://codereview.chromium.org/2298603004/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:782: totalLight.fX += normalizedZ * light.color().fX / attenuationValue; On 2016/08/31 17:18:22, jvanverth1 wrote: > If you precompute 1/attenuation and multiply by that, that should help perf. Done.
https://codereview.chromium.org/2298603004/diff/20001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2298603004/diff/20001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:775: isn't there supposed to be a "+i" somewhere in here?
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 ========== added point light attenuations to raster BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2298603004 ========== to ========== added point light attenuations to raster BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2298603004 Committed: https://skia.googlesource.com/skia/+/bcdc405ea07e44e4bd54b6cdcade300161a330a3 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/bcdc405ea07e44e4bd54b6cdcade300161a330a3 |
