Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2301173004:
added radial lights to SkLights (Closed)
|
Created:
4 years, 3 months ago by vjiaoblack Modified:
4 years, 3 months ago CC:
reviews_skia.org Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionadded radial lights to SkLights
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2301173004
Committed: https://skia.googlesource.com/skia/+/c1a50e1b735bd717eed1fc72a8f093a1b399cb07
Patch Set 1 #Patch Set 2 : changed 'flat' to 'radial' #
Total comments: 4
Patch Set 3 : made req changes #
Total comments: 2
Patch Set 4 : fixed one flat ->radial #
Total comments: 2
Patch Set 5 : made req comment fix #Patch Set 6 : made req comment fix #Messages
Total messages: 32 (18 generated)
Description was changed from ========== added flat lights to SkLights BUG=skia: ========== to ========== added flat lights to SkLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2301173004 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
In general this looks good, but I'm not too keen on the term 'flat'. Why not '2D' or 'radial'?
Description was changed from ========== added flat lights to SkLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2301173004 ========== to ========== added radial lights to SkLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2301173004 ==========
lgtm Have you run it through unit tests to make sure it passes? https://codereview.chromium.org/2301173004/diff/20001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2301173004/diff/20001/include/core/SkLights.h... include/core/SkLights.h:127: bool fIsRadial; // Whether the light is flat or not. Flat lights will cast Suggestion: change flat to 2D (2D light seems to be a standard term in games) https://codereview.chromium.org/2301173004/diff/20001/src/core/SkLights.cpp File src/core/SkLights.cpp (right): https://codereview.chromium.org/2301173004/diff/20001/src/core/SkLights.cpp#n... src/core/SkLights.cpp:78: bool isFlat = light.isRadial(); Nit: change to isRadial
I ran "dm --src tests" and everything seemed to pass fine. I also fixed the little naming inconsistencies, and changed the mentioning of "flat" in the SkLights.h comments to "radial" to match the current naming scheme we have. https://codereview.chromium.org/2301173004/diff/20001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2301173004/diff/20001/include/core/SkLights.h... include/core/SkLights.h:127: bool fIsRadial; // Whether the light is flat or not. Flat lights will cast On 2016/09/02 14:01:58, jvanverth1 wrote: > Suggestion: change flat to 2D (2D light seems to be a standard term in games) Done - changed to radial to match the naming scheme https://codereview.chromium.org/2301173004/diff/20001/src/core/SkLights.cpp File src/core/SkLights.cpp (right): https://codereview.chromium.org/2301173004/diff/20001/src/core/SkLights.cpp#n... src/core/SkLights.cpp:78: bool isFlat = light.isRadial(); On 2016/09/02 14:01:59, jvanverth1 wrote: > Nit: change to isRadial Done.
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com Link to the patchset: https://codereview.chromium.org/2301173004/#ps40001 (title: "made req changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2301173004/diff/40001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2301173004/diff/40001/include/core/SkLights.h... include/core/SkLights.h:126: sk_sp<SkImage> fShadowMap; Missed a 'Flat'
https://codereview.chromium.org/2301173004/diff/40001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2301173004/diff/40001/include/core/SkLights.h... include/core/SkLights.h:126: sk_sp<SkImage> fShadowMap; On 2016/09/02 18:57:27, robertphillips wrote: > Missed a 'Flat' Done.
vjiaoblack@google.com changed reviewers: + bsalomon@google.com
Needing the lgtm from bsalomon
lgtm, comment on comment. https://codereview.chromium.org/2301173004/diff/60001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2301173004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:127: bool fIsRadial; // Whether the light is radial or not. Radia lights will "Radial"?
https://codereview.chromium.org/2301173004/diff/60001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2301173004/diff/60001/include/core/SkLights.h... include/core/SkLights.h:127: bool fIsRadial; // Whether the light is radial or not. Radia lights will On 2016/09/06 19:38:17, bsalomon wrote: > "Radial"? Done.
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2301173004/#ps100001 (title: "made req comment fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== added radial lights to SkLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2301173004 ========== to ========== added radial lights to SkLights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2301173004 Committed: https://skia.googlesource.com/skia/+/c1a50e1b735bd717eed1fc72a8f093a1b399cb07 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/c1a50e1b735bd717eed1fc72a8f093a1b399cb07 |
