Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2026393005:
Add SampleApp slide with animating lightmapped objects & transparency (Closed)
|
Created:
4 years, 6 months ago by robertphillips Modified:
4 years, 6 months ago CC:
reviews_skia.org, dvonbeck Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd SampleApp slide with animating lightmapped objects & transparency
This is pulled out of the drawLitAtlas CL (may it someday land). It does nicely demonstrate animating normal mapped objects and normal maps combined with partially transparent diffuse textures.
It is a crude Asteroids game.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005
TBR=reed@google.com
Committed: https://skia.googlesource.com/skia/+/adf5afa628adb62a0ad451d07ef1442381a0ee20
Patch Set 1 #Patch Set 2 : Address compiler complaints #Patch Set 3 : Add unnecessary hack to silence compilers #Patch Set 4 : ... #
Total comments: 6
Patch Set 5 : Address code review comments #
Messages
Total messages: 39 (21 generated)
Description was changed from ========== Add SampleApp slide with animating lightmapped objects & transparency ========== to ========== Add SampleApp slide with animating lightmapped objects & transparency GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005 ==========
Description was changed from ========== Add SampleApp slide with animating lightmapped objects & transparency GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005 ========== to ========== Add SampleApp slide with animating lightmapped objects & transparency This is pulled out of the drawLitAtlas CL (may it someday land). It does nicely demonstrate animating normal mapped objects and normal maps combined with partially transparent diffuse textures. It is a crude Asteroids game. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005 ==========
robertphillips@google.com changed reviewers: + egdaniel@google.com
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026393005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026393005/1
The CQ bit was unchecked by commit-bot@chromium.org
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...) 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...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026393005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026393005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026393005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026393005/40001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026393005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2026393005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
lgtm with some nits and question on const removal https://codereview.chromium.org/2026393005/diff/60001/include/core/SkLights.h File include/core/SkLights.h (left): https://codereview.chromium.org/2026393005/diff/60001/include/core/SkLights.h... include/core/SkLights.h:64: const sk_sp<SkLights> finish() { Why remove const here? Is this something where we want something like sk_sp<const SkLights>? Is that even legal to say? https://codereview.chromium.org/2026393005/diff/60001/samplecode/SampleLitAtl... File samplecode/SampleLitAtlas.cpp (right): https://codereview.chromium.org/2026393005/diff/60001/samplecode/SampleLitAtl... samplecode/SampleLitAtlas.cpp:13: #include "SkLights.h" nit: flip Lights and Lighting in alpha order here https://codereview.chromium.org/2026393005/diff/60001/samplecode/SampleLitAtl... samplecode/SampleLitAtlas.cpp:64: newVel.fY = -c; DON' NEED TO CHANGE: You might want to consider capping the magnitude of the max velocity once you make these += again.
https://codereview.chromium.org/2026393005/diff/60001/include/core/SkLights.h File include/core/SkLights.h (left): https://codereview.chromium.org/2026393005/diff/60001/include/core/SkLights.h... include/core/SkLights.h:64: const sk_sp<SkLights> finish() { On 2016/06/03 15:52:48, egdaniel wrote: > Why remove const here? Is this something where we want something like > sk_sp<const SkLights>? Is that even legal to say? We discussed in person. The consensus is that sk_sp<const SkLights> is problematic for sk_sp and unnecessary b.c. SkLights is functionally const already. https://codereview.chromium.org/2026393005/diff/60001/samplecode/SampleLitAtl... File samplecode/SampleLitAtlas.cpp (right): https://codereview.chromium.org/2026393005/diff/60001/samplecode/SampleLitAtl... samplecode/SampleLitAtlas.cpp:13: #include "SkLights.h" On 2016/06/03 15:52:48, egdaniel wrote: > nit: flip Lights and Lighting in alpha order here Done. https://codereview.chromium.org/2026393005/diff/60001/samplecode/SampleLitAtl... samplecode/SampleLitAtlas.cpp:64: newVel.fY = -c; On 2016/06/03 15:52:48, egdaniel wrote: > DON' NEED TO CHANGE: You might want to consider capping the magnitude of the max > velocity once you make these += again. Done.
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026393005/80001
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 robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2026393005/#ps80001 (title: "Address code review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026393005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Description was changed from ========== Add SampleApp slide with animating lightmapped objects & transparency This is pulled out of the drawLitAtlas CL (may it someday land). It does nicely demonstrate animating normal mapped objects and normal maps combined with partially transparent diffuse textures. It is a crude Asteroids game. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005 ========== to ========== Add SampleApp slide with animating lightmapped objects & transparency This is pulled out of the drawLitAtlas CL (may it someday land). It does nicely demonstrate animating normal mapped objects and normal maps combined with partially transparent diffuse textures. It is a crude Asteroids game. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005 TBR=reed@google.com ==========
robertphillips@google.com changed reviewers: + reed@google.com
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026393005/80001
Message was sent while issue was closed.
Description was changed from ========== Add SampleApp slide with animating lightmapped objects & transparency This is pulled out of the drawLitAtlas CL (may it someday land). It does nicely demonstrate animating normal mapped objects and normal maps combined with partially transparent diffuse textures. It is a crude Asteroids game. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005 TBR=reed@google.com ========== to ========== Add SampleApp slide with animating lightmapped objects & transparency This is pulled out of the drawLitAtlas CL (may it someday land). It does nicely demonstrate animating normal mapped objects and normal maps combined with partially transparent diffuse textures. It is a crude Asteroids game. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2026393005 TBR=reed@google.com Committed: https://skia.googlesource.com/skia/+/adf5afa628adb62a0ad451d07ef1442381a0ee20 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/adf5afa628adb62a0ad451d07ef1442381a0ee20
Message was sent while issue was closed.
Patchset #6 (id:100001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
