Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190023002:
Added distance vector support for CircleGeometryProcessor (Closed)
DescriptionAdded distance vector support for CircleGeometryProcessor
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=2190023002
Committed: https://skia.googlesource.com/skia/+/779e1924eaf7c5652dee93e0e8e975bbf5723b37
Committed: https://skia.googlesource.com/skia/+/68f2f7dc4229c3761dd5074a94c35878abcb9a36
Patch Set 1 #Patch Set 2 : Fixed convoluted math statement #
Total comments: 6
Patch Set 3 : Addressed patch 2 comments, added length zero vector failsafe #
Total comments: 4
Patch Set 4 : Addressed Patch 3 comments #Messages
Total messages: 23 (11 generated)
Description was changed from ========== Added distance vector support for CircleGeometryProcessor BUG=skia: ========== to ========== Added distance vector support for CircleGeometryProcessor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190023002 ==========
Description was changed from ========== Added distance vector support for CircleGeometryProcessor BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190023002 ========== to ========== Added distance vector support for CircleGeometryProcessor 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=2190023002 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com, robertphillips@google.com
https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:141: fragBuilder->codeAppendf("%s = %s.z * (normalize(%s.xy) - %s.xy);", I feel like this would read easier as something like devVec = normalize(in.xy) * (in.z - d); where d is from above. Aka scale the vector by how far away we are. I believe though both equations are the same. https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:142: args.fDistanceVectorName, v.vsOut(), v.vsOut(), v.vsOut()); why not us fsIn() like the coverage code above?
https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:141: fragBuilder->codeAppendf("%s = %s.z * (normalize(%s.xy) - %s.xy);", On 2016/07/28 17:30:53, egdaniel wrote: > I feel like this would read easier as something like devVec = normalize(in.xy) * > (in.z - d); where d is from above. Aka scale the vector by how far away we are. > I believe though both equations are the same. That wouldn't work because d is a number from 0-1, not from 0-radius. I could do devVec = normalize(in.xy) * in.z * (1 - d). That looks easier to read, right?
https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:141: fragBuilder->codeAppendf("%s = %s.z * (normalize(%s.xy) - %s.xy);", On 2016/07/28 17:55:14, dvonbeck wrote: > On 2016/07/28 17:30:53, egdaniel wrote: > > I feel like this would read easier as something like devVec = normalize(in.xy) > * > > (in.z - d); where d is from above. Aka scale the vector by how far away we > are. > > I believe though both equations are the same. > > That wouldn't work because d is a number from 0-1, not from 0-radius. I could do > devVec = normalize(in.xy) * in.z * (1 - d). That looks easier to read, right? Ahh yes duh. Since we do in.z * (1-d) above, lets store that result in a variable earlier and reuse it here.
https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:141: fragBuilder->codeAppendf("%s = %s.z * (normalize(%s.xy) - %s.xy);", On 2016/07/28 18:01:52, egdaniel wrote: > On 2016/07/28 17:55:14, dvonbeck wrote: > > On 2016/07/28 17:30:53, egdaniel wrote: > > > I feel like this would read easier as something like devVec = > normalize(in.xy) > > * > > > (in.z - d); where d is from above. Aka scale the vector by how far away we > > are. > > > I believe though both equations are the same. > > > > That wouldn't work because d is a number from 0-1, not from 0-radius. I could > do > > devVec = normalize(in.xy) * in.z * (1 - d). That looks easier to read, right? > Ahh yes duh. > > Since we do in.z * (1-d) above, lets store that result in a variable earlier and > reuse it here. Done. https://codereview.chromium.org/2190023002/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:142: args.fDistanceVectorName, v.vsOut(), v.vsOut(), v.vsOut()); On 2016/07/28 17:30:53, egdaniel wrote: > why not us fsIn() like the coverage code above? Done.
lgtm https://codereview.chromium.org/2190023002/diff/40001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2190023002/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:143: args.fDistanceVectorName); align this inside ( of function https://codereview.chromium.org/2190023002/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:146: args.fDistanceVectorName, v.fsIn()); align this to function
The CQ bit was checked by dvonbeck@google.com
The CQ bit was unchecked by dvonbeck@google.com
https://codereview.chromium.org/2190023002/diff/40001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2190023002/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:143: args.fDistanceVectorName); On 2016/07/29 15:11:59, egdaniel wrote: > align this inside ( of function Done. https://codereview.chromium.org/2190023002/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:146: args.fDistanceVectorName, v.fsIn()); On 2016/07/29 15:11:59, egdaniel wrote: > align this to function Done.
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/2190023002/#ps60001 (title: "Addressed Patch 3 comments")
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 vector support for CircleGeometryProcessor 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=2190023002 ========== to ========== Added distance vector support for CircleGeometryProcessor 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=2190023002 Committed: https://skia.googlesource.com/skia/+/779e1924eaf7c5652dee93e0e8e975bbf5723b37 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/779e1924eaf7c5652dee93e0e8e975bbf5723b37
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2196053002/ by mtklein@google.com. The reason for reverting is: Reverting so I can revert https://codereview.chromium.org/2114993002/.
Message was sent while issue was closed.
Description was changed from ========== Added distance vector support for CircleGeometryProcessor 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=2190023002 Committed: https://skia.googlesource.com/skia/+/779e1924eaf7c5652dee93e0e8e975bbf5723b37 ========== to ========== Added distance vector support for CircleGeometryProcessor 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=2190023002 Committed: https://skia.googlesource.com/skia/+/779e1924eaf7c5652dee93e0e8e975bbf5723b37 ==========
The CQ bit was checked by dvonbeck@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 distance vector support for CircleGeometryProcessor 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=2190023002 Committed: https://skia.googlesource.com/skia/+/779e1924eaf7c5652dee93e0e8e975bbf5723b37 ========== to ========== Added distance vector support for CircleGeometryProcessor 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=2190023002 Committed: https://skia.googlesource.com/skia/+/779e1924eaf7c5652dee93e0e8e975bbf5723b37 Committed: https://skia.googlesource.com/skia/+/68f2f7dc4229c3761dd5074a94c35878abcb9a36 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/68f2f7dc4229c3761dd5074a94c35878abcb9a36 |
