Moved ambient lights out of SkLight's light array

Moved ambient lights out of SkLight's light array

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

Committed: https://skia.googlesource.com/skia/+/8f98f0aa2d3f7571a890b916c7c4b5ee831e9686
Committed: https://skia.googlesource.com/skia/+/a8eabc4a2a5559a1410fdbb348f967cd1554b325

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

Description was changed from

==========
Moved ambient lights out of SkLight's light array

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

to

==========
Moved ambient lights out of SkLight's light array

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

[vjiaoblack, 2016-08-26 15:41:56]

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

[vjiaoblack, 2016-08-26 15:41:56]

Things might get a bit tricky after the point lights lands. We'll see...

[robertphillips, 2016-08-26 16:08:05]

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

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:57: const SkVector3& dir() const {
keep as yoda-speak. The changeable variable is put on right.

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:61: const SkPoint3& pos() const {
same here

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:101: SkColor3f   fColor;           // linear (unpremul)
color. Range is 0..1 in each channel.
line up comment

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:119: void add(const Light& light) {
keep if test in case the caller tries to add stuff after a finish call

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:123: void add(Light&& light) {
here too

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:161: SkTArray<Light> fNonAmbLights;
fAmbientLightColor

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

https://codereview.chromium.org/2287553002/diff/1/src/core/SkShadowShader.cpp...
src/core/SkShadowShader.cpp:620: 
set accum to the ambient light color now ?

[vjiaoblack, 2016-08-26 17:24:26]

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

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:119: void add(const Light& light) {
On 2016/08/26 16:08:05, robertphillips wrote:
> keep if test in case the caller tries to add stuff after a finish call

Done.

https://codereview.chromium.org/2287553002/diff/1/include/core/SkLights.h#new...
include/core/SkLights.h:123: void add(Light&& light) {
On 2016/08/26 16:08:05, robertphillips wrote:
> here too

Done.

[robertphillips, 2016-08-26 17:31:29]

https://codereview.chromium.org/2287553002/diff/40001/samplecode/SampleShadow...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2287553002/diff/40001/samplecode/SampleShadow...
samplecode/SampleShadowing.cpp:27: 
??

https://codereview.chromium.org/2287553002/diff/40001/samplecode/SampleShadow...
samplecode/SampleShadowing.cpp:214: if (dx != 0 || dy != 0) {
??

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

https://codereview.chromium.org/2287553002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:115: 
the old name was more descriptive of what is going on

https://codereview.chromium.org/2287553002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:119: fLightColor[fNumLights] =
lights->light(i).color();
Won't this fail an assert ?

https://codereview.chromium.org/2287553002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:685: 
Again, where does the ambient contribution get added ?

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

https://codereview.chromium.org/2287553002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.h:30: // 4 non-ambient lights (currently just refers to
directional lights).
I think the old version was more descriptive

https://codereview.chromium.org/2287553002/diff/40001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2287553002/diff/40001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:80: //  the light the want to render the
depth map with.
Don't we still need the if test to eliminate point lights here ?

[vjiaoblack, 2016-08-26 18:10:38]

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

https://codereview.chromium.org/2287553002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:115: 
On 2016/08/26 17:31:28, robertphillips wrote:
> the old name was more descriptive of what is going on

Done.

https://codereview.chromium.org/2287553002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:119: fLightColor[fNumLights] =
lights->light(i).color();
On 2016/08/26 17:31:29, robertphillips wrote:
> Won't this fail an assert ?

Done.

https://codereview.chromium.org/2287553002/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:685: 
On 2016/08/26 17:31:29, robertphillips wrote:
> Again, where does the ambient contribution get added ?

right at the beginning?

https://codereview.chromium.org/2287553002/diff/40001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2287553002/diff/40001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:80: //  the light the want to render the
depth map with.
On 2016/08/26 17:31:29, robertphillips wrote:
> Don't we still need the if test to eliminate point lights here ?

Done.

[robertphillips, 2016-08-26 19:16:03]

https://codereview.chromium.org/2287553002/diff/60001/samplecode/SampleShadow...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2287553002/diff/60001/samplecode/SampleShadow...
samplecode/SampleShadowing.cpp:27: 
In the prior CL you were calling updateLights. Why stop doing so?

https://codereview.chromium.org/2287553002/diff/60001/samplecode/SampleShadow...
samplecode/SampleShadowing.cpp:215: if (dx != 0 || dy != 0) {
this-> - from prior CL

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

https://codereview.chromium.org/2287553002/diff/60001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:129: }
It looks like this is still used, why did you remove it?

https://codereview.chromium.org/2287553002/diff/60001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2287553002/diff/60001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:81: if
(SkLights::Light::kDirectional_LightType == this->fLights->light(0).type()) {
This test was actually fine the way it was b.c. ".type()" cannot be written to

[vjiaoblack, 2016-08-26 19:25:17]

https://codereview.chromium.org/2287553002/diff/60001/samplecode/SampleShadow...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2287553002/diff/60001/samplecode/SampleShadow...
samplecode/SampleShadowing.cpp:27: 
On 2016/08/26 19:16:02, robertphillips wrote:
> In the prior CL you were calling updateLights. Why stop doing so?

Done.

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

https://codereview.chromium.org/2287553002/diff/60001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:129: }
On 2016/08/26 19:16:02, robertphillips wrote:
> It looks like this is still used, why did you remove it?

Done.

https://codereview.chromium.org/2287553002/diff/60001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2287553002/diff/60001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:81: if
(SkLights::Light::kDirectional_LightType == this->fLights->light(0).type()) {
On 2016/08/26 19:16:02, robertphillips wrote:
> This test was actually fine the way it was b.c. ".type()" cannot be written to

Acknowledged.

[robertphillips, 2016-08-26 19:45:00]

https://codereview.chromium.org/2287553002/diff/80001/samplecode/SampleShadow...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2287553002/diff/80001/samplecode/SampleShadow...
samplecode/SampleShadowing.cpp:162: void updateLights(int x, int y) {
I'm okay with this change but don't you want to keep the old point light
behavior ?

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

https://codereview.chromium.org/2287553002/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:128: 
overlength line

[vjiaoblack, 2016-08-28 20:11:57]

https://codereview.chromium.org/2287553002/diff/80001/samplecode/SampleShadow...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2287553002/diff/80001/samplecode/SampleShadow...
samplecode/SampleShadowing.cpp:162: void updateLights(int x, int y) {
On 2016/08/26 19:45:00, robertphillips wrote:
> I'm okay with this change but don't you want to keep the old point light
> behavior ?

Mmmm..... I was going to do another pass and fix it later, but then I guess
point lights already landed. 

I'll keep the old one.

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

https://codereview.chromium.org/2287553002/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:128: 
On 2016/08/26 19:45:00, robertphillips wrote:
> overlength line

Done.

[robertphillips, 2016-08-29 12:42:32]

lgtm

[robertphillips, 2016-08-29 12:42:38]

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

[commit-bot: I haz the power, 2016-08-29 12:42:47]

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-29 12:45:17]

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

[commit-bot: I haz the power, 2016-08-29 12:45:18]

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...)

[vjiaoblack, 2016-08-29 13:19:51]

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

[commit-bot: I haz the power, 2016-08-29 13:19:53]

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-29 13:49:43]

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

[commit-bot: I haz the power, 2016-08-29 13:49:44]

Dry run: This issue passed the CQ dry run.

[vjiaoblack, 2016-08-29 13:59:54]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-29 13:59:54]

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/2287553002/#ps120001
(title: "removed a MakeAmbient call")

[commit-bot: I haz the power, 2016-08-29 14:00:04]

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-29 14:01:46]

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

[commit-bot: I haz the power, 2016-08-29 14:01:47]

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...)

[djsollen, 2016-08-29 14:06:39]

djsollen@google.com changed reviewers:
+ djsollen@google.com

[djsollen, 2016-08-29 14:06:40]

api lgtm

[vjiaoblack, 2016-08-29 14:07:48]

The CQ bit was checked by vjiaoblack@google.com

[commit-bot: I haz the power, 2016-08-29 14:07:57]

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-29 14:08:55]

Description was changed from

==========
Moved ambient lights out of SkLight's light array

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

to

==========
Moved ambient lights out of SkLight's light array

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

Committed:
https://skia.googlesource.com/skia/+/8f98f0aa2d3f7571a890b916c7c4b5ee831e9686
==========

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

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/8f98f0aa2d3f7571a890b916c7c4b5ee831e9686

[vjiaoblack, 2016-08-29 15:37:54]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2291663002/ by vjiaoblack@google.com.

The reason for reverting is: Made Deigo's GM miss their ambient lights.

[vjiaoblack, 2016-08-29 15:51:07]

Description was changed from

==========
Moved ambient lights out of SkLight's light array

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

Committed:
https://skia.googlesource.com/skia/+/8f98f0aa2d3f7571a890b916c7c4b5ee831e9686
==========

to

==========
Moved ambient lights out of SkLight's light array

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

Committed:
https://skia.googlesource.com/skia/+/8f98f0aa2d3f7571a890b916c7c4b5ee831e9686
==========

[vjiaoblack, 2016-08-29 16:43:10]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-29 16:43:11]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2287553002/#ps160001
(title: "added ambient light calculations to Raster renderer of LightingShader")

[commit-bot: I haz the power, 2016-08-29 16:43:45]

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-29 17:22:12]

Description was changed from

==========
Moved ambient lights out of SkLight's light array

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

Committed:
https://skia.googlesource.com/skia/+/8f98f0aa2d3f7571a890b916c7c4b5ee831e9686
==========

to

==========
Moved ambient lights out of SkLight's light array

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

Committed:
https://skia.googlesource.com/skia/+/8f98f0aa2d3f7571a890b916c7c4b5ee831e9686
Committed:
https://skia.googlesource.com/skia/+/a8eabc4a2a5559a1410fdbb348f967cd1554b325
==========

[commit-bot: I haz the power, 2016-08-29 17:22:14]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/a8eabc4a2a5559a1410fdbb348f967cd1554b325