Dash using vertex attributes for interval and stroke width

Dash using vertex attributes for interval and stroke width

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/5224ba7860ca8c734055895eac4b67888737047d

[joshualitt, 2015-02-02 22:18:00]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, joshualitt@google.com

[joshualitt, 2015-02-02 22:18:00]

A change in preparation for GrBatch to move some of the Dash GP functionality to
vertex attributes.

[bsalomon, 2015-02-03 01:02:01]

lgtm, but Greg should look too.

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.cpp (right):

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:194: verts[idx + 1].fRadius = radius;
nit, optional.. prolly fewer lines to write this as a loop

[egdaniel, 2015-02-03 14:45:59]

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.cpp (right):

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:78: SkScalar fIntervalLength;
move fIntervalLength before radius and center to match the LineVertex structure

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:205: SkASSERT(kNonRound_DashCap == cap &&
vertexStride == sizeof(DashLineVertex));
is the NonRound assert really needed since we already are checking this with the
if

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:214: verts[idx].fRect.set(halfOffLen + 0.5f,
-halfStroke + 0.5f,
as with Brians comment, probably can loop this. Also maybe just make the rect
locally once and I think you can use join(SkRect) to reduce the code here.

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:648: // The param attribute, xyz, refer to
circle radius - 0.5, cicles center x coord, and
Lets move the interval length to the second parameter so we can be consistent
for circle and line dashes. So the second parameter holds info on the
transformed dash position and the dashing interval. The third param will hold
info on how to draw an individual dash. So DashParams and Circle/RectParams.

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:888: // XY, and Z is interval length
"XY refer to the transformed dash position, and Z..."

[joshualitt, 2015-02-03 15:50:10]

On 2015/02/03 14:45:59, egdaniel wrote:
>
https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
> File src/gpu/effects/GrDashingEffect.cpp (right):
> 
>
https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
> src/gpu/effects/GrDashingEffect.cpp:78: SkScalar fIntervalLength;
> move fIntervalLength before radius and center to match the LineVertex
structure
> 
>
https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
> src/gpu/effects/GrDashingEffect.cpp:205: SkASSERT(kNonRound_DashCap == cap &&
> vertexStride == sizeof(DashLineVertex));
> is the NonRound assert really needed since we already are checking this with
the
> if
> 
>
https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
> src/gpu/effects/GrDashingEffect.cpp:214: verts[idx].fRect.set(halfOffLen +
0.5f,
> -halfStroke + 0.5f,
> as with Brians comment, probably can loop this. Also maybe just make the rect
> locally once and I think you can use join(SkRect) to reduce the code here.
> 
>
https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
> src/gpu/effects/GrDashingEffect.cpp:648: // The param attribute, xyz, refer to
> circle radius - 0.5, cicles center x coord, and
> Lets move the interval length to the second parameter so we can be consistent
> for circle and line dashes. So the second parameter holds info on the
> transformed dash position and the dashing interval. The third param will hold
> info on how to draw an individual dash. So DashParams and Circle/RectParams.
> 
>
https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
> src/gpu/effects/GrDashingEffect.cpp:888: // XY, and Z is interval length
> "XY refer to the transformed dash position, and Z..."

feedback incorporated

[egdaniel, 2015-02-03 15:57:53]

lgtm with the one minor nit

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.h (right):

https://codereview.chromium.org/892203003/diff/20001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.h:24: class SkPath;
doesn't look like we need this guy either

[joshualitt, 2015-02-03 22:01:18]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-03 22:02:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/892203003/60001

[commit-bot: I haz the power, 2015-02-03 23:07:54]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/5224ba7860ca8c734055895eac4b67888737047d