Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285133002:
Optimizations and more documentation of SkShadowShader (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. |
DescriptionOptimizations and more documentation of SkShadowShader
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2285133002
Committed: https://skia.googlesource.com/skia/+/bb106060fab32d048ef5052fcc7dc12a0f06b961
Patch Set 1 #Patch Set 2 : Cleaned up 'offset' code #Patch Set 3 : removed redundant vec2 ing #
Total comments: 16
Patch Set 4 : made req changes #
Total comments: 15
Patch Set 5 : made req changes #
Messages
Total messages: 24 (16 generated)
Description was changed from ========== Optimizations and more documentation of SkShadowShader BUG=skia: ========== to ========== Optimizations and more documentation of SkShadowShader BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2285133002 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
https://codereview.chromium.org/2285133002/diff/40001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2285133002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:56: fShadowParams.fMinVariance = 0.015625; It's not clear to me why this is now 1/64. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:123: fInvSqLightIntensity[fNumNonAmbLights] = 1.0f / The standard formula for a point light is L = I * clamp(N*L, 0, 1) / attenuation, where I is the light's radiant intensity and L is outgoing radiance. Physically, I has no upper bound per component, but we're using color, which usually does have an upper bound of one. So either you can approximate the full range by saying that I = lightIntensity*lightColor, where lightIntensity is a single value that scales the color, or you can let the color mean radiant intensity and allow values greater than one. Since we're using a uniform for color I'd recommend the latter. Multiplying by 1.0/lightIntensity^2 makes no sense to me physically. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:270: fragBuilder->codeAppendf("vec3 totalLightColor = vec3(0,0,0);"); You can just do vec3(0); Don't need appendf. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:277: fragBuilder->codeAppendf("vec3 worldCor;"); Don't need appendf here, can use append. Same for line above. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:311: "vec2(1.0, -1.0);", Replace 255*vec2(1, -1) with vec2(255, -255)? And is this the right order? Should it be -255, 255? Either way, a comment would be helpful as it's a little unclear what's going on here. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:345: fragBuilder->codeAppendf("if (%s.b <= %s.b + .001953125) {", Good catch. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:349: // so here we multiply it by 256. Where are we multiplying by 256? https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:370: "(1 - %s), 0, 1);", Nit: Indent to parenthesis.
https://codereview.chromium.org/2285133002/diff/40001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2285133002/diff/40001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:56: fShadowParams.fMinVariance = 0.015625; On 2016/08/29 18:30:14, jvanverth1 wrote: > It's not clear to me why this is now 1/64. I'll put the tranform in later. It was because I use variance in a 0 - 1 space instead of 0 - 65536 space in the shader now https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:123: fInvSqLightIntensity[fNumNonAmbLights] = 1.0f / On 2016/08/29 18:30:14, jvanverth1 wrote: > The standard formula for a point light is L = I * clamp(N*L, 0, 1) / > attenuation, where I is the light's radiant intensity and L is outgoing > radiance. Physically, I has no upper bound per component, but we're using color, > which usually does have an upper bound of one. So either you can approximate the > full range by saying that I = lightIntensity*lightColor, where lightIntensity is > a single value that scales the color, or you can let the color mean radiant > intensity and allow values greater than one. Since we're using a uniform for > color I'd recommend the latter. Multiplying by 1.0/lightIntensity^2 makes no > sense to me physically. Okay, i'll simplify the model! Done. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:270: fragBuilder->codeAppendf("vec3 totalLightColor = vec3(0,0,0);"); On 2016/08/29 18:30:14, jvanverth1 wrote: > You can just do vec3(0); > Don't need appendf. Done. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:277: fragBuilder->codeAppendf("vec3 worldCor;"); On 2016/08/29 18:30:14, jvanverth1 wrote: > Don't need appendf here, can use append. Same for line above. Done. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:311: "vec2(1.0, -1.0);", On 2016/08/29 18:30:14, jvanverth1 wrote: > Replace 255*vec2(1, -1) with vec2(255, -255)? And is this the right order? > Should it be -255, 255? Either way, a comment would be helpful as it's a little > unclear what's going on here. Done. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:345: fragBuilder->codeAppendf("if (%s.b <= %s.b + .001953125) {", On 2016/08/29 18:30:14, jvanverth1 wrote: > Good catch. Done. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:349: // so here we multiply it by 256. On 2016/08/29 18:30:14, jvanverth1 wrote: > Where are we multiplying by 256? Acknowledged. https://codereview.chromium.org/2285133002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:370: "(1 - %s), 0, 1);", On 2016/08/29 18:30:14, jvanverth1 wrote: > Nit: Indent to parenthesis. Done.
https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:123: fLightIntensity[fNumNonAmbLights] = lights->light(i).intensity(); Suggestion: allow SkLight to have color and intensity (because it's a clearer API), but when you create the uniforms, you only need fLightColor. For point lights, multiply light.color() and light.intensity() together and store that in fLightColor. That avoids a multiply for every fragment. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:178: {nullptr}; Why is this on a different line? https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:280: fragBuilder->codeAppendf("worldCor = vec3(vMatrixCoord_0_1_Stage0 * " You might want to put a note in the header that the shadow shader is required to be in Stage0. Otherwise this won't work -- you'll get the wrong texture coords or compilation will fail because vMatrixCoord_0_1_Stage0 isn't defined. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:289: // vMatrixCoord_0_1_Stage0 is the texture sampler coordinates. This comment should move up to where you first use vMatrixCoord_0_1_Stage0. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:352: // We mess with depth and depth^2 in their given scales. Since you reverted the variance change, do you need to revert this too? https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:394: "%s * %s * 2 /" Why multiply by 2? https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:463: pdman.set1f(fMinVarianceUni, minVariance / 65536.0f); Should this be reverted?
https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:123: fLightIntensity[fNumNonAmbLights] = lights->light(i).intensity(); On 2016/08/30 15:55:42, jvanverth1 wrote: > Suggestion: allow SkLight to have color and intensity (because it's a clearer > API), but when you create the uniforms, you only need fLightColor. For point > lights, multiply light.color() and light.intensity() together and store that in > fLightColor. That avoids a multiply for every fragment. Done. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:178: {nullptr}; On 2016/08/30 15:55:42, jvanverth1 wrote: > Why is this on a different line? Done. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:178: {nullptr}; On 2016/08/30 15:55:42, jvanverth1 wrote: > Why is this on a different line? Acknowledged. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:280: fragBuilder->codeAppendf("worldCor = vec3(vMatrixCoord_0_1_Stage0 * " On 2016/08/30 15:55:42, jvanverth1 wrote: > You might want to put a note in the header that the shadow shader is required to > be in Stage0. Otherwise this won't work -- you'll get the wrong texture coords > or compilation will fail because vMatrixCoord_0_1_Stage0 isn't defined. Done. Know of any way to do this in a non hacky way? https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:289: // vMatrixCoord_0_1_Stage0 is the texture sampler coordinates. On 2016/08/30 15:55:42, jvanverth1 wrote: > This comment should move up to where you first use vMatrixCoord_0_1_Stage0. Done. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:352: // We mess with depth and depth^2 in their given scales. On 2016/08/30 15:55:42, jvanverth1 wrote: > Since you reverted the variance change, do you need to revert this too I think this is fine. Everything also works as expected. What variance change do you mean? https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:394: "%s * %s * 2 /" On 2016/08/30 15:55:42, jvanverth1 wrote: > Why multiply by 2? Done. https://codereview.chromium.org/2285133002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:463: pdman.set1f(fMinVarianceUni, minVariance / 65536.0f); On 2016/08/30 15:55:42, jvanverth1 wrote: > Should this be reverted? What revert are you talking about?
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: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
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: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
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 ========== Optimizations and more documentation of SkShadowShader BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2285133002 ========== to ========== Optimizations and more documentation of SkShadowShader BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2285133002 Committed: https://skia.googlesource.com/skia/+/bb106060fab32d048ef5052fcc7dc12a0f06b961 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/bb106060fab32d048ef5052fcc7dc12a0f06b961 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
