Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151653002:
Added math for SkNormalBevelSource to create bevels on GPU side (Closed)
DescriptionThis CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002
Committed: https://skia.googlesource.com/skia/+/1b9e2fb49415d8dc41e449bee5f8ebec6f616d71
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Math changed to use quadratic bezier #Patch Set 4 : rebase #Patch Set 5 : Corrected math for linear bevels #Patch Set 6 : Corrected math for rounded bevels #Patch Set 7 : Formatted string fix #Patch Set 8 : Fixed rebasing problem #Patch Set 9 : Fixed some oversized lines #
Total comments: 12
Patch Set 10 : Optimized math calculations, addressed patch 9 comments #
Total comments: 6
Patch Set 11 : Corrected and optimized linear bevel, addressed patch 10 comments #
Total comments: 2
Patch Set 12 : Addressed patch 11 comments #Patch Set 13 : Consistency fix #Messages
Total messages: 19 (9 generated)
Description was changed from ========== Added math to create bevels on GPU side BUG=skia: ========== to ========== Added math to create bevels on GPU side BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ==========
Description was changed from ========== Added math to create bevels on GPU side BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ========== to ========== BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com, robertphillips@google.com
Description was changed from ========== BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ========== to ========== This CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ==========
Description was changed from ========== This CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ========== to ========== DO NOT LAND, math is incorrect, and in source space as opposed to the required device space This CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ==========
CL is ready for review!
Remove the "DO NOT LAND" disclaimer ? https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:25: * What do you think about renaming these bevelWidth, fBevelWidth, ... ? https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:60: Do you think the shader compilers are going to be smart enough to coalesce all the length(distanceVector) calls? https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:109: break; Add a "// fall through" comment https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:113: // setting it equal to the negative reciprocal of the derivative of the bezier cure -> curve One would hope the shader compilers can coalesce the sqrt(t) calls. https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:121: // negative height Is sign() verboten?
Description was changed from ========== DO NOT LAND, math is incorrect, and in source space as opposed to the required device space This CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ========== to ========== This CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ==========
https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:60: On 2016/08/03 16:33:14, robertphillips wrote: > Do you think the shader compilers are going to be smart enough to coalesce all > the length(distanceVector) calls? I assume so but I don't know. Do you think I should change it? https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:113: // setting it equal to the negative reciprocal of the derivative of the bezier On 2016/08/03 16:33:15, robertphillips wrote: > cure -> curve > > One would hope the shader compilers can coalesce the sqrt(t) calls. Do you think they will, or should I make it explicit?
Optimized math, addressed comments. https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:25: * On 2016/08/03 16:33:15, robertphillips wrote: > What do you think about renaming these bevelWidth, fBevelWidth, ... > > ? Done. Should I do this in the user-facing API too? https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:60: On 2016/08/03 16:39:24, dvonbeck wrote: > On 2016/08/03 16:33:14, robertphillips wrote: > > Do you think the shader compilers are going to be smart enough to coalesce all > > the length(distanceVector) calls? > > I assume so but I don't know. Do you think I should change it? No longer necessary. https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:109: break; On 2016/08/03 16:33:15, robertphillips wrote: > Add a "// fall through" comment Done. https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:113: // setting it equal to the negative reciprocal of the derivative of the bezier On 2016/08/03 16:33:15, robertphillips wrote: > cure -> curve > > One would hope the shader compilers can coalesce the sqrt(t) calls. Done. https://codereview.chromium.org/2151653002/diff/160001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:121: // negative height On 2016/08/03 16:33:15, robertphillips wrote: > Is sign() verboten? No longer necessary.
https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:104: if ( type == SkNormalSource::BevelType::kRoundedIn ) { this should be inside roundOut/In section right? https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:112: fb->codeAppendf("normal = normalize(%s*vec3(%s, 0.0) + vec3(0.0, 0.0, %s));", I don't think this is correct. Without going into detail here, the fact that the final z value will depend on the x,y of the distance vector (cause of the normalization), must be wrong. Z should only depend on width and height. I can show you my approach tomorrow but the tldr is we can pass up two uniforms (in the width height slots) and the normal is vec3(dv_norm * H', W') with no normalization needed. https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:123: fb->codeAppendf("vec2 unnormalizedNormal_dz = vec2(%s*(1.0-rootTOverW), " just trying to confirm in my head, since we multiply the d component by height, it will just naturally give us the right x,y for a negative height, right?
Corrected and optimized linear bevel, addressed patch 10 comments https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:104: if ( type == SkNormalSource::BevelType::kRoundedIn ) { On 2016/08/04 03:08:17, egdaniel wrote: > this should be inside roundOut/In section right? I guess it makes more sense over there. Done. https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:112: fb->codeAppendf("normal = normalize(%s*vec3(%s, 0.0) + vec3(0.0, 0.0, %s));", On 2016/08/04 03:08:17, egdaniel wrote: > I don't think this is correct. Without going into detail here, the fact that the > final z value will depend on the x,y of the distance vector (cause of the > normalization), must be wrong. Z should only depend on width and height. I can > show you my approach tomorrow but the tldr is we can pass up two uniforms (in > the width height slots) and the normal is vec3(dv_norm * H', W') with no > normalization needed. Done. https://codereview.chromium.org/2151653002/diff/180001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:123: fb->codeAppendf("vec2 unnormalizedNormal_dz = vec2(%s*(1.0-rootTOverW), " On 2016/08/04 03:08:17, egdaniel wrote: > just trying to confirm in my head, since we multiply the d component by height, > it will just naturally give us the right x,y for a negative height, right? Yes. I think it's more that my math explicitly built the vector pointing outwards, so I had to flip it when it didn't. I don't know why I had to flip the z-component before and not now, but it works ¯\_(ツ)_/¯
lgtm with 2 little nits https://codereview.chromium.org/2151653002/diff/200001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2151653002/diff/200001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:158: fb->codeAppendf("normal = vec3(%s * dv_norm, %s);", for sanity lets assert normailizedHeight and normalizedWidth here https://codereview.chromium.org/2151653002/diff/200001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:163: case SkNormalSource::BevelType::kRoundedIn: assert the height in here
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2151653002/#ps240001 (title: "Consistency fix")
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 ========== This CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 ========== to ========== This CL's base is the CL that sets up the distance vector field req. exposure: https://codereview.chromium.org/2114993002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2151653002 Committed: https://skia.googlesource.com/skia/+/1b9e2fb49415d8dc41e449bee5f8ebec6f616d71 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://skia.googlesource.com/skia/+/1b9e2fb49415d8dc41e449bee5f8ebec6f616d71 |
