Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(414)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1245883003: Move LightingShader to effects (Closed)

Created:
5 years, 5 months ago by robertphillips
Modified:
5 years, 4 months ago
Reviewers:
jvanverth1, bsalomon, reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Move LightingShader to effects Additionally this CL: forces the light colors to be opaque forces the light direction to be normalized adds a raster implementation adds a gm Committed: https://skia.googlesource.com/skia/+/5f865b9cf11ac16e41f10d78276d6b8cf68762de

Patch Set 1 #

Patch Set 2 : update #

Total comments: 3

Patch Set 3 : Address code review comments (rm setLight and hide class behind factory) #

Patch Set 4 : update comments #

Patch Set 5 : add todo #

Patch Set 6 : Moved SkLightingShader.h to src/effects #

Patch Set 7 : Fix no-GPU build #

Patch Set 8 : Address 64b gcc memory issues (seems to require more space than others) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+762 lines, -360 lines) Patch
M gm/lighting.cpp View 1 2 3 4 5 1 chunk +1 line, -2 lines 0 comments Download
A gm/lightingshader.cpp View 1 2 1 chunk +111 lines, -0 lines 0 comments Download
M gyp/effects.gypi View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M samplecode/SampleLighting.cpp View 1 2 3 chunks +15 lines, -333 lines 0 comments Download
M src/core/SkBitmapProcShader.h View 1 2 3 4 5 6 7 3 chunks +2 lines, -4 lines 0 comments Download
M src/core/SkBitmapProcShader.cpp View 4 chunks +6 lines, -21 lines 0 comments Download
M src/core/SkBitmapProcState.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A src/effects/SkLightingShader.h View 1 2 3 4 5 1 chunk +45 lines, -0 lines 0 comments Download
A src/effects/SkLightingShader.cpp View 1 2 3 4 5 6 7 1 chunk +579 lines, -0 lines 0 comments Download

Messages

Total messages: 33 (14 generated)
robertphillips
5 years, 5 months ago (2015-07-23 17:10:14 UTC) #2
jvanverth1
Couple of minor comments, otherwise looks fine to me. https://codereview.chromium.org/1245883003/diff/20001/include/effects/SkLightingShader.h File include/effects/SkLightingShader.h (right): https://codereview.chromium.org/1245883003/diff/20001/include/effects/SkLightingShader.h#newcode24 include/effects/SkLightingShader.h:24: ...
5 years, 5 months ago (2015-07-23 17:40:14 UTC) #3
robertphillips
ping (Mike)
5 years, 4 months ago (2015-07-27 17:33:39 UTC) #4
robertphillips
ptal - this version should address all code review comments https://codereview.chromium.org/1245883003/diff/20001/include/effects/SkLightingShader.h File include/effects/SkLightingShader.h (right): https://codereview.chromium.org/1245883003/diff/20001/include/effects/SkLightingShader.h#newcode24 ...
5 years, 4 months ago (2015-07-28 14:15:46 UTC) #5
robertphillips
ping (Mike)
5 years, 4 months ago (2015-07-29 15:48:24 UTC) #6
robertphillips
Moved SkLightingShader.h to src/effects
5 years, 4 months ago (2015-07-29 16:42:27 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245883003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245883003/100001
5 years, 4 months ago (2015-07-29 16:42:51 UTC) #9
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot/builds/2233)
5 years, 4 months ago (2015-07-29 16:44:13 UTC) #11
reed1
lgtm future ideas: - vec3 for ambient and light-color - lights[] - dox for both ...
5 years, 4 months ago (2015-07-29 16:45:13 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245883003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245883003/120001
5 years, 4 months ago (2015-07-29 16:50:19 UTC) #14
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/2215)
5 years, 4 months ago (2015-07-29 17:00:50 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245883003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245883003/120001
5 years, 4 months ago (2015-07-29 17:01:46 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/2217)
5 years, 4 months ago (2015-07-29 17:11:37 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245883003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245883003/120001
5 years, 4 months ago (2015-07-29 17:28:50 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/2221)
5 years, 4 months ago (2015-07-29 17:38:43 UTC) #25
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245883003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245883003/140001
5 years, 4 months ago (2015-07-29 19:13:44 UTC) #27
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 4 months ago (2015-07-29 19:21:23 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245883003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245883003/140001
5 years, 4 months ago (2015-07-29 19:27:28 UTC) #32
commit-bot: I haz the power
5 years, 4 months ago (2015-07-29 19:28:07 UTC) #33
Message was sent while issue was closed. 
Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/5f865b9cf11ac16e41f10d78276d6b8cf68762de

Powered by Google App Engine
This is Rietveld 408576698