Add new SkPoint3 class

Add new SkPoint3 class

The existing Light filter and the upcoming Lighting Shader both need a Point3 class

Committed: https://skia.googlesource.com/skia/+/3d32d768cd8b66c49c070495c08f7933b9dd2423

[robertphillips, 2015-07-09 20:01:04]

robertphillips@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-07-10 14:22:06]

perhaps we add this to include/effects for now, since it still isn't referenced
by anything in core yet.

[robertphillips, 2015-07-10 15:39:30]

Moved

[reed1, 2015-07-10 15:46:29]

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint3.h
File include/effects/SkPoint3.h (right):

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint...
include/effects/SkPoint3.h:11: #include "SkColor.h"
why do we include this?

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint...
include/effects/SkPoint3.h:54: void scale(SkScalar scale, SkPoint3* dst) const {
as an API pattern, I think we should consider returning a new value instead for
this sort of thing.

e.g.

SkPoint3 = src.makeScale(...)

but just a thought.

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint...
include/effects/SkPoint3.h:56: dst->set(SkScalarMul(fX, scale), SkScalarMul(fY,
scale), SkScalarMul(fZ, scale));
SkScalarMul is not needed!

[robertphillips, 2015-07-10 16:54:24]

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint3.h
File include/effects/SkPoint3.h (right):

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint...
include/effects/SkPoint3.h:11: #include "SkColor.h"
On 2015/07/10 15:46:29, reed1 wrote:
> why do we include this?

Done.

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint...
include/effects/SkPoint3.h:54: void scale(SkScalar scale, SkPoint3* dst) const {
On 2015/07/10 15:46:29, reed1 wrote:
> as an API pattern, I think we should consider returning a new value instead
for
> this sort of thing.
> 
> e.g.
> 
> SkPoint3 = src.makeScale(...)
> 
> but just a thought.

Done. What do you think about propagating this back to SkPoint (in a separate
CL)?

https://codereview.chromium.org/1229693009/diff/60001/include/effects/SkPoint...
include/effects/SkPoint3.h:56: dst->set(SkScalarMul(fX, scale), SkScalarMul(fY,
scale), SkScalarMul(fZ, scale));
On 2015/07/10 15:46:29, reed1 wrote:
> SkScalarMul is not needed!

Done.

[reed1, 2015-07-10 17:40:17]

lgtm

[robertphillips, 2015-07-10 18:31:43]

This is going to causes a lot of layout test image differences
(https://codereview.chromium.org/1232073004/ Add suppressions for SVG lighting
image filters) so I will postpone landing until Monday.

[scroggo, 2015-07-10 18:35:54]

On 2015/07/10 18:31:43, robertphillips wrote:
> This is going to causes a lot of layout test image differences
> (https://codereview.chromium.org/1232073004/ Add suppressions for SVG lighting
> image filters) so I will postpone landing until Monday.

Did I get added to check whether there was an impact on Android? Android is not
using SkPoint3, which I believe is the only API change?

[robertphillips, 2015-07-13 20:03:49]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2015-07-13 20:04:09]

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

[commit-bot: I haz the power, 2015-07-13 20:16:49]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/3d32d768cd8b66c49c070495c08f7933b9dd2423