Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246463004:
Added distance attenuation and diffuse shading to PointLights (Closed)
|
Created:
4 years, 4 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. |
DescriptionAdded distance attenuation and diffuse shading to PointLights
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246463004
Committed: https://skia.googlesource.com/skia/+/56f33ea2acb39ebb041340a8ab7564facb95afce
Patch Set 1 #Patch Set 2 : Added diffuse shading for point lights #Patch Set 3 : Cleaned up code #
Total comments: 8
Patch Set 4 : Made req changes- made light intensity a light attribute #
Total comments: 22
Patch Set 5 : made req changes; removed the unused isPointLight uni #
Total comments: 31
Patch Set 6 : made some requested changes #Patch Set 7 : removed unnecessary name strings #Patch Set 8 : made req changes #
Total comments: 18
Patch Set 9 : made req changes #
Total comments: 3
Patch Set 10 : made one req change #Patch Set 11 : rebased off of master (with variance shadow mapping) #
Total comments: 17
Patch Set 12 : made req changes #
Total comments: 6
Patch Set 13 : made req changes #Patch Set 14 : disabled shadows: #
Messages
Total messages: 44 (14 generated)
Description was changed from ========== Added distance rebased; made changes creating point lights, and handling shadowing them done _ lightning storm created point lights BUG=skia: ========== to ========== Added distance rebased; made changes creating point lights, and handling shadowing them done _ lightning storm created point lights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246463004 ==========
Description was changed from ========== Added distance rebased; made changes creating point lights, and handling shadowing them done _ lightning storm created point lights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246463004 ========== to ========== Added distance attenuation and diffuse shading to PointLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246463004 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
On 2016/08/12 20:48:39, vjiaoblack wrote: ** Not yet ready for review, sorry!
https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:203: kFloat_GrSLType, we have kBool_GrSLType https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:246: "vec2(%s, %s), %s.b * 256);\n", Why 256? And create a constant for this magic number. E.g., #define MAGIC_NUMBER 256 and then "vec2(%s, %s), %s.b * " MAGIC_NUMBER ");\n", https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:272: fragBuilder->codeAppendf("float dist%d = length(fragToLight%d);" Do you use dist? If not, why not use normalize()? If so, you probably want dist^2, which is dot(fragToLight, fragToLight). Then multiply by inversesqrt(dist^2). https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:325: fragBuilder->codeAppendf("%s += fragToLight%d.z * %s / pow(((dist%d / 1024.0) + 1.0), 2);", Ah, I see you use dist here. Using pow to do a square seems excessive, though -- just compute a quantity and multiply by itself, or get the dist^2 as described above. Also, why divide by 1024? what are the units here? And if you do need to do that, multiply by 0.0009765625. Finally, it looks like you're using (1 + dist)^2 as attenuation. I use 1 + dist^2 as attenuation -- seems to give good results without having to get the square root.
I'm leaving the optimization for later - i.e. right now, I'm gonna stick with passing in light intensity and then dividing dist^2 by intensity^2 in the light attenuation function. that ok? https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:203: kFloat_GrSLType, On 2016/08/16 01:17:25, jvanverth1 wrote: > we have kBool_GrSLType Done. https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:246: "vec2(%s, %s), %s.b * 256);\n", On 2016/08/16 01:17:25, jvanverth1 wrote: > Why 256? And create a constant for this magic number. E.g., #define MAGIC_NUMBER > 256 and then > "vec2(%s, %s), %s.b * " MAGIC_NUMBER ");\n", Done. https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:272: fragBuilder->codeAppendf("float dist%d = length(fragToLight%d);" On 2016/08/16 01:17:25, jvanverth1 wrote: > Do you use dist? If not, why not use normalize()? If so, you probably want > dist^2, which is dot(fragToLight, fragToLight). Then multiply by > inversesqrt(dist^2). Done. https://codereview.chromium.org/2246463004/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:325: fragBuilder->codeAppendf("%s += fragToLight%d.z * %s / pow(((dist%d / 1024.0) + 1.0), 2);", On 2016/08/16 01:17:25, jvanverth1 wrote: > Ah, I see you use dist here. Using pow to do a square seems excessive, though -- > just compute a quantity and multiply by itself, or get the dist^2 as described > above. Also, why divide by 1024? what are the units here? And if you do need to > do that, multiply by 0.0009765625. Finally, it looks like you're using (1 + > dist)^2 as attenuation. I use 1 + dist^2 as attenuation -- seems to give good > results without having to get the square root. Done.
https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:102: SkVector3 fDirection; // For directional lights, holds the direction towards the Reusing the name fDirection for point lights seems odd to me. Maybe fDirOrPos? https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:107: SkScalar fIntensity; // Dictates the light intensity. Basically this divides You need to mention that this is only used for point lights. And the equation should be I * (n dot L) * color / dist^2. That would be clearer and get rid of the divide. https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:115: fIntensity = 0.0f; SK_ScalarZero https://codereview.chromium.org/2246463004/diff/60001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2246463004/diff/60001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:42: fSceneChanged = true; Ideally these simple types should be in the initializer list, i.e. ShadowingView() : fSceneChanged(true), etc. https://codereview.chromium.org/2246463004/diff/60001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:149: builder.add(SkLights::Light::MakePoint(SkColor3f::Make(0.3f, 0.5f, 0.7f), If you stick these lines in a method that takes x & y, you can call them here and in the constructor and only have to update in one place. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkLights.cpp File src/core/SkLights.cpp (right): https://codereview.chromium.org/2246463004/diff/60001/src/core/SkLights.cpp#n... src/core/SkLights.cpp:32: SkScalar intensity = 0.0f; SK_ScalarZero https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:121: fLightDir[fNumDirLights] = lights->light(i).pos(); fLightDirOrPos? https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:167: bool isPointLight[SkShadowShader::kMaxNonAmbientLights]; I'm wondering if it might be worthwhile to change the code that gets outputted rather than have this boolean uniform. That would mean a shader created for each different light setup, but odds are you'll only have one light setup anyway. That could be a separate change. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:288: "(povDepth.b) / vec2(%s, %s);\n", I don't like these divides, but can live with it for now. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:305: "vec2(%s.x, 0 - %s.y)) " Indent https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:338: fragBuilder->codeAppendf("%s += fragToLight%d.z * %s / (1 + distsq%d / (%s * %s));", Greater than 100 char line.
https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:102: SkVector3 fDirection; // For directional lights, holds the direction towards the On 2016/08/17 19:59:02, jvanverth1 wrote: > Reusing the name fDirection for point lights seems odd to me. Maybe fDirOrPos? Done. https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:107: SkScalar fIntensity; // Dictates the light intensity. Basically this divides On 2016/08/17 19:59:02, jvanverth1 wrote: > You need to mention that this is only used for point lights. And the equation > should be I * (n dot L) * color / dist^2. That would be clearer and get rid of > the divide. That equation would also provide a slightly incorrect equation / different effect than what I originally coded as, which is fine. https://codereview.chromium.org/2246463004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:115: fIntensity = 0.0f; On 2016/08/17 19:59:02, jvanverth1 wrote: > SK_ScalarZero This does not exist. SK_ScalarNearlyZero exists, but I'm sure that's not the same. https://codereview.chromium.org/2246463004/diff/60001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2246463004/diff/60001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:42: fSceneChanged = true; On 2016/08/17 19:59:02, jvanverth1 wrote: > Ideally these simple types should be in the initializer list, i.e. > ShadowingView() : fSceneChanged(true), etc. Done. https://codereview.chromium.org/2246463004/diff/60001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:149: builder.add(SkLights::Light::MakePoint(SkColor3f::Make(0.3f, 0.5f, 0.7f), On 2016/08/17 19:59:02, jvanverth1 wrote: > If you stick these lines in a method that takes x & y, you can call them here > and in the constructor and only have to update in one place. Done. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkLights.cpp File src/core/SkLights.cpp (right): https://codereview.chromium.org/2246463004/diff/60001/src/core/SkLights.cpp#n... src/core/SkLights.cpp:32: SkScalar intensity = 0.0f; On 2016/08/17 19:59:02, jvanverth1 wrote: > SK_ScalarZero Doesn't exist? https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:121: fLightDir[fNumDirLights] = lights->light(i).pos(); On 2016/08/17 19:59:02, jvanverth1 wrote: > fLightDirOrPos? Done. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:167: bool isPointLight[SkShadowShader::kMaxNonAmbientLights]; On 2016/08/17 19:59:02, jvanverth1 wrote: > I'm wondering if it might be worthwhile to change the code that gets outputted > rather than have this boolean uniform. That would mean a shader created for each > different light setup, but odds are you'll only have one light setup anyway. > That could be a separate change. Done. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:288: "(povDepth.b) / vec2(%s, %s);\n", On 2016/08/17 19:59:02, jvanverth1 wrote: > I don't like these divides, but can live with it for now. Done. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:305: "vec2(%s.x, 0 - %s.y)) " On 2016/08/17 19:59:02, jvanverth1 wrote: > Indent Done. https://codereview.chromium.org/2246463004/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:338: fragBuilder->codeAppendf("%s += fragToLight%d.z * %s / (1 + distsq%d / (%s * %s));", On 2016/08/17 19:59:02, jvanverth1 wrote: > Greater than 100 char line. Done.
https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h... include/core/SkLights.h:58: static Light MakePoint(const SkColor3f& color, const SkPoint3& pos, SkScalar intensity) { Add intensity as last param to the unifying ctor and default it to 0.0 https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h... include/core/SkLights.h:94: fDirOrPos = b.fDirOrPos; copy fIntensity too ? https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h... include/core/SkLights.h:106: Now, why can't we just make the light's color dimmer ? https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:26: this-> ? https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:141: 1024)); no ambient ? https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:153: if (dx != 0 || dy != 0) { this-> ? https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:113: refers to directional lights -> count of directional and point lights ? https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:127: fIsPointLight[fNumNonAmbLights] = make the '==' yoda speak https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:166: a) can this be a memcpy ? b) shouldn't it only be for 'numLights' ? https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:178: Are we really getting any mileage out of having these be separate ? https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:193: lightIntensityUniNameStr.appendf("%d", i); We don't seem to use this https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:464: const SkColor3f& ambientColor() const { return fAmbientColor; } just return 'bool' - "const bool&" is way overkill https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:468: } lightDirOrPos ?
https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h... include/core/SkLights.h:58: static Light MakePoint(const SkColor3f& color, const SkPoint3& pos, SkScalar intensity) { On 2016/08/18 17:12:59, robertphillips wrote: > Add intensity as last param to the unifying ctor and default it to 0.0 > Done. https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h... include/core/SkLights.h:94: fDirOrPos = b.fDirOrPos; On 2016/08/18 17:12:59, robertphillips wrote: > copy fIntensity too ? Done. https://codereview.chromium.org/2246463004/diff/80001/include/core/SkLights.h... include/core/SkLights.h:106: On 2016/08/18 17:12:59, robertphillips wrote: > Now, why can't we just make the light's color dimmer ? We can. But we can't make the light's color brighter. https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:26: On 2016/08/18 17:12:59, robertphillips wrote: > this-> ? Done. https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:141: 1024)); On 2016/08/18 17:12:59, robertphillips wrote: > no ambient ? That is correct. https://codereview.chromium.org/2246463004/diff/80001/samplecode/SampleShadow... samplecode/SampleShadowing.cpp:153: if (dx != 0 || dy != 0) { On 2016/08/18 17:12:59, robertphillips wrote: > this-> ? Done. https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:113: On 2016/08/18 17:12:59, robertphillips wrote: > refers to directional lights -> count of directional and point lights ? Done. https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:127: fIsPointLight[fNumNonAmbLights] = On 2016/08/18 17:12:59, robertphillips wrote: > make the '==' yoda speak Done. https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:166: On 2016/08/18 17:12:59, robertphillips wrote: > a) can this be a memcpy ? > b) shouldn't it only be for 'numLights' ? a) ok sure b) no, because we need to use this value outside of the glsl code. https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:178: On 2016/08/18 17:12:59, robertphillips wrote: > Are we really getting any mileage out of having these be separate ? Yes we are. You'd want to be able to change color separately from how far or big or powerful the light is. Arbitrarily deciding on a default or a maximum color brightness, and then having all light colors be a small fraction (for smaller lights) isn't really something that'll work for all cases. Besides, in the correct light attenuation formula, lightIntensity (or really lightRadius, the radius of the reach of the light; which I renamed based on a suggestion) occupies a different part of the equation than lightColor https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:193: lightIntensityUniNameStr.appendf("%d", i); On 2016/08/18 17:12:59, robertphillips wrote: > We don't seem to use this Okay, true https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:464: const SkColor3f& ambientColor() const { return fAmbientColor; } On 2016/08/18 17:12:59, robertphillips wrote: > just return 'bool' - "const bool&" is way overkill Done. https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:468: } On 2016/08/18 17:12:59, robertphillips wrote: > lightDirOrPos ? Done.
https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:166: On 2016/08/18 18:05:34, vjiaoblack wrote: > On 2016/08/18 17:12:59, robertphillips wrote: > > a) can this be a memcpy ? > > b) shouldn't it only be for 'numLights' ? > > a) ok sure > b) no, because we need to use this value outside of the glsl code. okay, do you even need to copy it then? https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:178: On 2016/08/18 18:05:34, vjiaoblack wrote: > On 2016/08/18 17:12:59, robertphillips wrote: > > Are we really getting any mileage out of having these be separate ? > > Yes we are. You'd want to be able to change color separately from how far or big > or powerful the light is. Arbitrarily deciding on a default or a maximum color > brightness, and then having all light colors be a small fraction (for smaller > lights) isn't really something that'll work for all cases. > > Besides, in the correct light attenuation formula, lightIntensity (or really > lightRadius, the radius of the reach of the light; which I renamed based on a > suggestion) occupies a different part of the equation than lightColor I meant having lightDirUniNameBase broken out into its own variable. Seems like we could just modify the following appendf calls.
https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:166: On 2016/08/18 18:32:11, robertphillips wrote: > On 2016/08/18 18:05:34, vjiaoblack wrote: > > On 2016/08/18 17:12:59, robertphillips wrote: > > > a) can this be a memcpy ? > > > b) shouldn't it only be for 'numLights' ? > > > > a) ok sure > > b) no, because we need to use this value outside of the glsl code. > > okay, do you even need to copy it then? Yes, because without passing this value in, we wouldn't be able to output appropriate code. Check out line 275 for example? https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:178: On 2016/08/18 18:32:11, robertphillips wrote: > On 2016/08/18 18:05:34, vjiaoblack wrote: > > On 2016/08/18 17:12:59, robertphillips wrote: > > > Are we really getting any mileage out of having these be separate ? > > > > Yes we are. You'd want to be able to change color separately from how far or > big > > or powerful the light is. Arbitrarily deciding on a default or a maximum color > > brightness, and then having all light colors be a small fraction (for smaller > > lights) isn't really something that'll work for all cases. > > > > Besides, in the correct light attenuation formula, lightIntensity (or really > > lightRadius, the radius of the reach of the light; which I renamed based on a > > suggestion) occupies a different part of the equation than lightColor > > I meant having lightDirUniNameBase broken out into its own variable. Seems like > we could just modify the following appendf calls. Okay, sure.
https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:166: On 2016/08/18 19:08:16, vjiaoblack wrote: > On 2016/08/18 18:32:11, robertphillips wrote: > > On 2016/08/18 18:05:34, vjiaoblack wrote: > > > On 2016/08/18 17:12:59, robertphillips wrote: > > > > a) can this be a memcpy ? > > > > b) shouldn't it only be for 'numLights' ? > > > > > > a) ok sure > > > b) no, because we need to use this value outside of the glsl code. > > > > okay, do you even need to copy it then? > > Yes, because without passing this value in, we wouldn't be able to output > appropriate code. > > Check out line 275 for example? Right, why can't we access the copy in ShadowFP ?
https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:174: for (int i = 0; i < shadowFP.fNumNonAmbLights; i++) { Should lightDir be lightDirOrPos ? https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:359: pdman.set3fv(fLightDirUni[i], 1, &lightDirOrPos.fX); Should fLightDir be fLightDirOrPos ? https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:482: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); \n
https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:269: lightDirUniName[i], widthUniName, heightUniName); Could you store 1/w, 1/h in these uniforms and so avoid the divide every fragment? Also, why not combine them into a vec2? https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:278: depthMapWidthUniName[i], depthMapHeightUniName[i]); Same here with depthMapWidth, depthMapHeight. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:317: fragBuilder->codeAppendf("%s += fragToLight%d.z * %s / " Clamp fragToLight.z to be >= 0. The light could have a smaller z than the primitive so you'd end up with a negative value. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:318: "(1 + distsq%d / (%s * %s));", Still dividing by intensity^2 here. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:408: SkVector3 fLightDir[SkShadowShader::kMaxNonAmbientLights]; fLightDirOrPos https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:410: fLightDirUni[SkShadowShader::kMaxNonAmbientLights]; fLightDirOrPosUni
I would like to avoid /adding/ more uniforms etc. to the code, and any nonessential changes that force changes in other sections, because that will make merging a lot harder. I have to merge this with the pointlights shadows code I have, that with the blurry shadows code I have, then with raster (which should be fine but then I'll have to merge over the new code correctly, which could be challenging depending on what else I fix), then that with the 2D shadows / 1D shadow map code I have. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:174: for (int i = 0; i < shadowFP.fNumNonAmbLights; i++) { On 2016/08/23 16:51:02, robertphillips wrote: > Should lightDir be lightDirOrPos ? Done. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:269: lightDirUniName[i], widthUniName, heightUniName); On 2016/08/23 17:25:22, jvanverth1 wrote: > Could you store 1/w, 1/h in these uniforms and so avoid the divide every > fragment? Also, why not combine them into a vec2? Yes; I wanted to make a final run for efficiency after more of the code has landed, to avoid making redundant changes in separate branches though. That okay? https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:278: depthMapWidthUniName[i], depthMapHeightUniName[i]); On 2016/08/23 17:25:22, jvanverth1 wrote: > Same here with depthMapWidth, depthMapHeight. Acknowledged. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:317: fragBuilder->codeAppendf("%s += fragToLight%d.z * %s / " On 2016/08/23 17:25:22, jvanverth1 wrote: > Clamp fragToLight.z to be >= 0. The light could have a smaller z than the > primitive so you'd end up with a negative value. Done. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:318: "(1 + distsq%d / (%s * %s));", On 2016/08/23 17:25:22, jvanverth1 wrote: > Still dividing by intensity^2 here. I think this preserves conceptual clarity right now, and in the future will either be an optimization or another design discussion. Is it okay to leave it? https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:359: pdman.set3fv(fLightDirUni[i], 1, &lightDirOrPos.fX); On 2016/08/23 16:51:02, robertphillips wrote: > Should fLightDir be fLightDirOrPos ? Done. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:408: SkVector3 fLightDir[SkShadowShader::kMaxNonAmbientLights]; On 2016/08/23 17:25:22, jvanverth1 wrote: > fLightDirOrPos Done. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:410: fLightDirUni[SkShadowShader::kMaxNonAmbientLights]; On 2016/08/23 17:25:22, jvanverth1 wrote: > fLightDirOrPosUni Done. https://codereview.chromium.org/2246463004/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:482: const ShadowFP& shadowFP = proc.cast<ShadowFP>(); On 2016/08/23 16:51:02, robertphillips wrote: > \n Done.
Uploaded new code. My bad
https://codereview.chromium.org/2246463004/diff/160001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:238: fragBuilder->codeAppendf("vec3 worldCor = vec3(vMatrixCoord_0_1_Stage0 * " Don't need '\n' in shader code. The pretty printer takes care of it for us. Here and below too. https://codereview.chromium.org/2246463004/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:359: pdman.set3fv(fLightDirOrPosUni[i], 1, &lightDirOrPos.fX); Shouldn't fLightDir be fLightDirOrPos ?
https://codereview.chromium.org/2246463004/diff/160001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:238: fragBuilder->codeAppendf("vec3 worldCor = vec3(vMatrixCoord_0_1_Stage0 * " On 2016/08/23 19:24:51, robertphillips wrote: > Don't need '\n' in shader code. The pretty printer takes care of it for us. > > Here and below too. I would like to make this change after I merge the two branches, to avoid making the same change repeatedly and then having to deal with the merges.
https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:52: These are now set in ctor list https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:55: this too https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:57: fSelectedSlider = -1; this too https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:59: these too https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:164: } ?? we're doing this above https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:174: Didn't you remove this copy once before ? https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:180: what ? https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:433: Didn't you remove this once before ?
https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... File samplecode/SampleShadowing.cpp (right): https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:52: On 2016/08/25 17:41:43, robertphillips wrote: > These are now set in ctor list Done. https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:55: On 2016/08/25 17:41:43, robertphillips wrote: > this too Done. https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:57: fSelectedSlider = -1; On 2016/08/25 17:41:43, robertphillips wrote: > this too Done. https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:59: On 2016/08/25 17:41:43, robertphillips wrote: > these too Done. https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:164: } On 2016/08/25 17:41:43, robertphillips wrote: > ?? we're doing this above Done. https://codereview.chromium.org/2246463004/diff/200001/samplecode/SampleShado... samplecode/SampleShadowing.cpp:164: } On 2016/08/25 17:41:43, robertphillips wrote: > ?? we're doing this above Done. https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:174: On 2016/08/25 17:41:43, robertphillips wrote: > Didn't you remove this copy once before ? Done. https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:180: On 2016/08/25 17:41:43, robertphillips wrote: > what ? Done. https://codereview.chromium.org/2246463004/diff/200001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:433: On 2016/08/25 17:41:43, robertphillips wrote: > Didn't you remove this once before ? Done.
https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:171: // add uniforms use shadowFP here instead of re-casting here and throughout this method https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:260: fragBuilder->codeAppendf("vec3 worldCor = vec3(vMatrixCoord_0_1_Stage0 * " rm '\n's in new shader code https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:381: } else { Didn't we already perform this test to get into this else block ?
https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:171: // add uniforms On 2016/08/26 14:28:28, robertphillips wrote: > use shadowFP here instead of re-casting > > here and throughout this method Done. https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:260: fragBuilder->codeAppendf("vec3 worldCor = vec3(vMatrixCoord_0_1_Stage0 * " On 2016/08/26 14:28:28, robertphillips wrote: > rm '\n's in new shader code Done. https://codereview.chromium.org/2246463004/diff/220001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:381: } else { On 2016/08/26 14:28:28, robertphillips wrote: > Didn't we already perform this test to get into this else block ? Done.
lgtm
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...
The CQ bit was unchecked by vjiaoblack@google.com
vjiaoblack@google.com changed reviewers: + bsalomon@google.com
Heya! Could you (bsalomon) check this over for lgtm-worthiness?
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.
djsollen@google.com changed reviewers: + djsollen@google.com
api lgtm
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/2246463004/#ps260001 (title: "disabled shadows:")
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 distance attenuation and diffuse shading to PointLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246463004 ========== to ========== Added distance attenuation and diffuse shading to PointLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246463004 Committed: https://skia.googlesource.com/skia/+/56f33ea2acb39ebb041340a8ab7564facb95afce ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://skia.googlesource.com/skia/+/56f33ea2acb39ebb041340a8ab7564facb95afce |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
