Added PointLights to SkLights::Light

Added PointLights to SkLights::Light

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2237493002

Committed: https://skia.googlesource.com/skia/+/772b5ee446d7545eec4ca7d6dc9d75461e2a9c38

[vjiaoblack, 2016-08-10 15:41:48]

Description was changed from

==========
Added PointLights to SkLights::Light

BUG=skia:
==========

to

==========
Added PointLights to SkLights::Light

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2237493002
==========

[vjiaoblack, 2016-08-10 15:42:16]

vjiaoblack@google.com changed reviewers:
+ jvanverth@google.com, robertphillips@google.com

[robertphillips, 2016-08-10 16:34:37]

Where is the serialization stuff?

https://codereview.chromium.org/2237493002/diff/1/gm/lightingshader.cpp
File gm/lightingshader.cpp (right):

https://codereview.chromium.org/2237493002/diff/1/gm/lightingshader.cpp#newco...
gm/lightingshader.cpp:50: 
I don't think we need the Light suffix (e.g., just MakeDirectional)

https://codereview.chromium.org/2237493002/diff/1/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:40: 
We could have a private Light(type, color, vec3) ctor that each one of these
calls

https://codereview.chromium.org/2237493002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:59: 
SkPoint3 for pos. Right now they are typedefed to be the same.

https://codereview.chromium.org/2237493002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:73: }
SkPoint3

[robertphillips, 2016-08-10 17:48:39]

It looks like the lighting serialization is duplicated in two places. Shall we
centralize it?

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:41: static Light MakeAmbient(const SkColor3f& color) {
just "return Light(...);"
no need for the temporary variable

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:54: static Light MakePoint(const SkColor3f& color, const
SkPoint3& pos) {
Same here

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:92: SkColor3f   fColor;           // linear (unpremul)
color. Range is 0..1 in each channel.
Fix comment

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:97: Light(LightType type, const SkColor3f color, const
SkVector3 dir) {
Don't need "this->" on member variables

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkLightingShad...
File src/core/SkLightingShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkLightingShad...
src/core/SkLightingShader.cpp:434: } else {
dir -> dirOrPos

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:606: 
What about serialization of point lights here ?

[vjiaoblack, 2016-08-11 14:27:23]

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:41: static Light MakeAmbient(const SkColor3f& color) {
On 2016/08/10 17:48:39, robertphillips wrote:
> just "return Light(...);"
> no need for the temporary variable

Done.

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:54: static Light MakePoint(const SkColor3f& color, const
SkPoint3& pos) {
On 2016/08/10 17:48:39, robertphillips wrote:
> Same here

Done.

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:92: SkColor3f   fColor;           // linear (unpremul)
color. Range is 0..1 in each channel.
On 2016/08/10 17:48:39, robertphillips wrote:
> Fix comment

Done.

https://codereview.chromium.org/2237493002/diff/40001/include/core/SkLights.h...
include/core/SkLights.h:97: Light(LightType type, const SkColor3f color, const
SkVector3 dir) {
On 2016/08/10 17:48:39, robertphillips wrote:
> Don't need "this->" on member variables

Done.

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkLightingShad...
File src/core/SkLightingShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkLightingShad...
src/core/SkLightingShader.cpp:434: } else {
On 2016/08/10 17:48:39, robertphillips wrote:
> dir -> dirOrPos

Done.

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:606: 
On 2016/08/10 17:48:39, robertphillips wrote:
> What about serialization of point lights here ?

Done.

[robertphillips, 2016-08-11 14:56:18]

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:12: #include "../private/SkTArray.h"
Do we need to include SkImage or can we just predeclare it ?

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:14: #include "SkPoint3.h"
predeclare ReadBuffer and WriteBuffer and outline the code.

One isn't allowed to pull headers from src in include

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:92: SkColor3f   fColor;           // linear (unpremul)
color. Range is 0..1 in each channel.
// For directional lights, holds the direction towards ...

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:140: 
The pattern seems to be:

static sk_sp<SkLights> MakeFromBuffer(SkReadBuffer& buffer);

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:172: 
Are you intentionally not serializing/deserializing the shadow maps ?

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkLightingShad...
File src/core/SkLightingShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkLightingShad...
src/core/SkLightingShader.cpp:419: sk_sp<SkLights> lights;
How does this work? Isn't lights null here ?

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:595: sk_sp<SkLights> lights;
same here

[vjiaoblack, 2016-08-11 19:18:54]

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:12: #include "../private/SkTArray.h"
On 2016/08/11 14:56:18, robertphillips wrote:
> Do we need to include SkImage or can we just predeclare it ?

Done.

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:14: #include "SkPoint3.h"
On 2016/08/11 14:56:18, robertphillips wrote:
> predeclare ReadBuffer and WriteBuffer and outline the code.
> 
> One isn't allowed to pull headers from src in include

Done.

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:92: SkColor3f   fColor;           // linear (unpremul)
color. Range is 0..1 in each channel.
On 2016/08/11 14:56:18, robertphillips wrote:
> // For directional lights, holds the direction towards ...

Done.

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:140: 
On 2016/08/11 14:56:18, robertphillips wrote:
> The pattern seems to be:
> 
> static sk_sp<SkLights> MakeFromBuffer(SkReadBuffer& buffer);

Done.

https://codereview.chromium.org/2237493002/diff/60001/include/core/SkLights.h...
include/core/SkLights.h:172: 
On 2016/08/11 14:56:18, robertphillips wrote:
> Are you intentionally not serializing/deserializing the shadow maps ?

Done.

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkLightingShad...
File src/core/SkLightingShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkLightingShad...
src/core/SkLightingShader.cpp:419: sk_sp<SkLights> lights;
On 2016/08/11 14:56:18, robertphillips wrote:
> How does this work? Isn't lights null here ?

Done.

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2237493002/diff/60001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:595: sk_sp<SkLights> lights;
On 2016/08/11 14:56:18, robertphillips wrote:
> same here

Done.

[robertphillips, 2016-08-12 13:34:49]

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h
File include/core/SkLights.h (left):

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:114: int numLights() const {
Why out line these ?

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:98: 
const SkColor3f& color, const SkVector3& dir);

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:126: private:
no ';' needed

https://codereview.chromium.org/2237493002/diff/80001/src/core/SkLights.cpp
File src/core/SkLights.cpp (right):

https://codereview.chromium.org/2237493002/diff/80001/src/core/SkLights.cpp#n...
src/core/SkLights.cpp:67: sk_sp<SkImage> depthMap;
Not all lights need to have a shadow map!

[robertphillips, 2016-08-12 13:34:49]

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h
File include/core/SkLights.h (left):

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:114: int numLights() const {
Why out line these ?

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:98: 
const SkColor3f& color, const SkVector3& dir);

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:126: private:
no ';' needed

https://codereview.chromium.org/2237493002/diff/80001/src/core/SkLights.cpp
File src/core/SkLights.cpp (right):

https://codereview.chromium.org/2237493002/diff/80001/src/core/SkLights.cpp#n...
src/core/SkLights.cpp:67: sk_sp<SkImage> depthMap;
Not all lights need to have a shadow map!

[vjiaoblack, 2016-08-12 14:12:10]

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:98: 
On 2016/08/12 13:34:48, robertphillips wrote:
> const SkColor3f& color, const SkVector3& dir);

Done.

https://codereview.chromium.org/2237493002/diff/80001/include/core/SkLights.h...
include/core/SkLights.h:126: private:
On 2016/08/12 13:34:48, robertphillips wrote:
> no ';' needed

Done.

https://codereview.chromium.org/2237493002/diff/80001/src/core/SkLights.cpp
File src/core/SkLights.cpp (right):

https://codereview.chromium.org/2237493002/diff/80001/src/core/SkLights.cpp#n...
src/core/SkLights.cpp:67: sk_sp<SkImage> depthMap;
On 2016/08/12 13:34:48, robertphillips wrote:
> Not all lights need to have a shadow map!

Done.

[robertphillips, 2016-08-12 14:27:49]

lgtm + nits

https://codereview.chromium.org/2237493002/diff/100001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/100001/include/core/SkLights....
include/core/SkLights.h:109: 
Don't need SkLights:: here

https://codereview.chromium.org/2237493002/diff/100001/include/core/SkLights....
include/core/SkLights.h:115: 
Or here

https://codereview.chromium.org/2237493002/diff/100001/src/core/SkLights.cpp
File src/core/SkLights.cpp (right):

https://codereview.chromium.org/2237493002/diff/100001/src/core/SkLights.cpp#...
src/core/SkLights.cpp:40: }
Don't need this else clause - sk_sp self inits

[vjiaoblack, 2016-08-12 14:56:46]

https://codereview.chromium.org/2237493002/diff/100001/include/core/SkLights.h
File include/core/SkLights.h (right):

https://codereview.chromium.org/2237493002/diff/100001/include/core/SkLights....
include/core/SkLights.h:109: 
On 2016/08/12 14:27:49, robertphillips wrote:
> Don't need SkLights:: here

Done.

https://codereview.chromium.org/2237493002/diff/100001/include/core/SkLights....
include/core/SkLights.h:115: 
On 2016/08/12 14:27:49, robertphillips wrote:
> Or here

Done.

https://codereview.chromium.org/2237493002/diff/100001/src/core/SkLights.cpp
File src/core/SkLights.cpp (right):

https://codereview.chromium.org/2237493002/diff/100001/src/core/SkLights.cpp#...
src/core/SkLights.cpp:40: }
On 2016/08/12 14:27:49, robertphillips wrote:
> Don't need this else clause - sk_sp self inits

Done.

[vjiaoblack, 2016-08-12 14:57:05]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-12 14:57:05]

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/2237493002/#ps160001
(title: "removed extra new lines")

[commit-bot: I haz the power, 2016-08-12 14:57:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 14:59:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 14:59:04]

Try jobs failed on following builders:
  skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[vjiaoblack, 2016-08-12 15:21:53]

The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 15:22:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 15:26:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 15:26:35]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[vjiaoblack, 2016-08-12 15:30:12]

The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 15:30:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vjiaoblack, 2016-08-12 15:36:48]

vjiaoblack@google.com changed reviewers:
+ bsalomon@google.com

[vjiaoblack, 2016-08-12 15:36:49]

Public API call! need specific reviewer (bsalomon)

[commit-bot: I haz the power, 2016-08-12 15:40:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 15:40:07]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[bsalomon, 2016-08-12 15:52:26]

api lgtm

[vjiaoblack, 2016-08-12 18:09:03]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-12 18:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2237493002/#ps220001
(title: "fixed serialization bug")

[commit-bot: I haz the power, 2016-08-12 18:09:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 18:38:50]

Description was changed from

==========
Added PointLights to SkLights::Light

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2237493002
==========

to

==========
Added PointLights to SkLights::Light

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2237493002

Committed:
https://skia.googlesource.com/skia/+/772b5ee446d7545eec4ca7d6dc9d75461e2a9c38
==========

[commit-bot: I haz the power, 2016-08-12 18:38:51]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/772b5ee446d7545eec4ca7d6dc9d75461e2a9c38