Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043393002:
Refactoring of GPU NormalMap handling out into its own class (Closed)
DescriptionRefactoring of GPU NormalMap handling out into its own class.
The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources.
What this CL includes:
- Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP.
- Encapsulates this new fragment processor on a new class NormalMapSource.
- Created a NormalSource abstraction that will interface with SkLightingShader.
- Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002
Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da
Committed: https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318
Committed: https://skia.googlesource.com/skia/+/8811e40850ac3310c17fe8cdaffe72817a5e317d
Patch Set 1 #
Total comments: 26
Patch Set 2 : Rebased, unified headers, NormalSource is now serialized, addressed patch 1 comments #
Total comments: 28
Patch Set 3 : Refactoring, style fixes #
Total comments: 14
Patch Set 4 : More style fixes, comments, rebasing #Patch Set 5 : Small comment fix #
Total comments: 12
Patch Set 6 : Addressed patch 5 comments #Patch Set 7 : Removed use of VLAs, sufixed floats with 'f' #Patch Set 8 : Moved include out of #if SK_SUPPORT_GPU #Patch Set 9 : Fixed memory leak on serializationtest #Patch Set 10 : Fixed SK_API flags, got rid of NormalMapSource::Make #
Dependent Patchsets: Messages
Total messages: 54 (25 generated)
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: ========== to ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 ==========
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: ========== to ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: ========== to ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. Execution should be unaffected though. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: ==========
I know you are working on changes to clean up some of this, but here are some of my first quick pass thoughts. https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.cpp File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.c... src/core/SkLightingShader.cpp:17: #include "SkNormalSourceImpl.h" we place include in alphabetical order, so move this up between SkMathPriv and SkPoint3 https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.c... src/core/SkLightingShader.cpp:217: // TODO does this need changing now? It shouldn't need to. The parent and child processors emit their own keys independent of each other. https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.c... src/core/SkLightingShader.cpp:353: // TODO is this correct memory handling? should work. When you pass it into LightingFP and you call registerChild it should ref it there. Thus you want to then remove your ref from this version. https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.h File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.h... src/core/SkLightingShader.h:19: remove this \n https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... File src/core/SkNormalSourceImpl.cpp (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:15: : fNormDeviceTransform(kLocal_GrCoordSet, normMatrix, normal, normParams.filterMode()) Skia's style has these initiliazers only tabbed in once https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:24: class NormalMapGLFP : public GrGLSLFragmentProcessor { Usually we put the GL in front of the class name, and actaully using GL is an old style and should be GLSL at this point. So lets call this GLSLNormalMapFP https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:64: b->add32(0x0); This should be fine outputing zero. Clean up the comment since it still refers to Lighting https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:91: inout->mulByUnknownFourComponents(); Since this guy doesn't actually use his input, and we don't know what the normals he will produce are, I would do inout->setUnknownFourComponents() https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:139: const SkMatrix *normLocalM) alight with other parameters https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:140: : fNormalMap(normal) again remove one tab https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:172: // TODO Here, the old code was checking that diffuse map and normal map are same size, that We should really have a way so that the diffuse bitmap isn't required and instead is treated more as a color input to the lighting shader https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:199: // TODO is this correct memory handling? yes this is fine here. It is the callers responsibility to handle unreffing of the object
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. Execution should be unaffected though. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: ========== to ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. Execution should be unaffected though. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 ==========
https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.cpp File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.c... src/core/SkLightingShader.cpp:17: #include "SkNormalSourceImpl.h" On 2016/06/09 16:56:37, egdaniel wrote: > we place include in alphabetical order, so move this up between SkMathPriv and > SkPoint3 Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.c... src/core/SkLightingShader.cpp:217: // TODO does this need changing now? On 2016/06/09 16:56:37, egdaniel wrote: > It shouldn't need to. The parent and child processors emit their own keys > independent of each other. Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.c... src/core/SkLightingShader.cpp:353: // TODO is this correct memory handling? On 2016/06/09 16:56:37, egdaniel wrote: > should work. When you pass it into LightingFP and you call registerChild it > should ref it there. Thus you want to then remove your ref from this version. The master upstream changed and asFragmentProcessor() now returns an sk_sp<>. I assume the memory handling is still correct since it will unref when the sk_sp goes out of scope. https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.h File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkLightingShader.h... src/core/SkLightingShader.h:19: On 2016/06/09 16:56:37, egdaniel wrote: > remove this \n Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... File src/core/SkNormalSourceImpl.cpp (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:15: : fNormDeviceTransform(kLocal_GrCoordSet, normMatrix, normal, normParams.filterMode()) On 2016/06/09 16:56:38, egdaniel wrote: > Skia's style has these initiliazers only tabbed in once Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:24: class NormalMapGLFP : public GrGLSLFragmentProcessor { On 2016/06/09 16:56:38, egdaniel wrote: > Usually we put the GL in front of the class name, and actaully using GL is an > old style and should be GLSL at this point. So lets call this GLSLNormalMapFP Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:64: b->add32(0x0); On 2016/06/09 16:56:38, egdaniel wrote: > This should be fine outputing zero. Clean up the comment since it still refers > to Lighting Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:91: inout->mulByUnknownFourComponents(); On 2016/06/09 16:56:38, egdaniel wrote: > Since this guy doesn't actually use his input, and we don't know what the > normals he will produce are, I would do inout->setUnknownFourComponents() The object doesn't have a setUnknownFourComponents() method. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:139: const SkMatrix *normLocalM) On 2016/06/09 16:56:38, egdaniel wrote: > alight with other parameters Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:140: : fNormalMap(normal) On 2016/06/09 16:56:38, egdaniel wrote: > again remove one tab Done. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:172: // TODO Here, the old code was checking that diffuse map and normal map are same size, that On 2016/06/09 16:56:38, egdaniel wrote: > We should really have a way so that the diffuse bitmap isn't required and > instead is treated more as a color input to the lighting shader Acknowledged. https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:199: // TODO is this correct memory handling? On 2016/06/09 16:56:38, egdaniel wrote: > yes this is fine here. It is the callers responsibility to handle unreffing of > the object Done. https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... tests/SerializationTest.cpp:187: // TODO Is this ok? or was the 4KB limit there for some technical reason? I think it is OK for me to do this, since a previous commit had increased it from 1KB with no apparent cause or consequence. The test still passes. https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... tests/SerializationTest.cpp:576: SkVector invNormRotation = { sqrt(0.3), sqrt(0.7) }; I'm allowed to use ::sqrt(), right? https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... tests/SerializationTest.cpp:585: // TODO test equality? I realized there aren't any tests for the correctness of SkLightingShader serialization. This might be a concern.
https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... File src/core/SkNormalSourceImpl.cpp (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:91: inout->mulByUnknownFourComponents(); On 2016/06/09 18:51:29, dvonbeck wrote: > On 2016/06/09 16:56:38, egdaniel wrote: > > Since this guy doesn't actually use his input, and we don't know what the > > normals he will produce are, I would do inout->setUnknownFourComponents() > > The object doesn't have a setUnknownFourComponents() method. Sorry that was the function on the inner struct. This should be a setToUnknown or something similar to that. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:56: sk_sp<const SkLightingShader::NormalSource> normalSource) putting a const inside of an sk_sp is generally a bad thing to do. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:71: fNormalSource = normalSource; use std::move( https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:122: sk_sp<const SkLightingShader::NormalSource> fNormalSource; again const sk_sp is bad. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:165: this->registerChildProcessor(normalFP); use std::move(noramlFP) here https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:276: // TODO add comparison of NormalFPs The base class of isEquals takes care of calling child processor onIsEqual so this guy really just needs to check his own stuff. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:351: /* TODO is this correct memory handling? FPs were changed to use sk_sp so it is different from This should still be fine still https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:358: new LightingFP(diffuseTexture, diffM, diffParams, fLights, normalFP)); I believe you want to use std::move here around normalFP https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:572: &diffLocalM, &normLocalM, normalSource); wrap normalSouce in std::move https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.h:20: class NormalSource : public SkFlattenable { For SK_API, it is believed that having it on the outer class should extend it to all the inner objects including nested classes. So for now lets leave it out, put it becomes very obvious if certain builds fail that this is the issue and we can add it back. So talking with Mike Klein, we don't want this class to subclass SkFlattenable. Instead just add functions (and just match Flattenable names) for the various reads and writes for flattening. Then just call them from SkLightingShader's flatten commands. This way you don't need this up in the enum. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.h:21: remove \n between class and public https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.h:35: SK_DEFINE_FLATTENABLE_TYPE(NormalSource) These macros just declare/define various flattenable functions. If you want to use the declare ones to get the names that is fine, or just declare them manually yourself (especially since we won't be subclassing flattenable). https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... tests/SerializationTest.cpp:576: SkVector invNormRotation = { sqrt(0.3), sqrt(0.7) }; On 2016/06/09 18:51:29, dvonbeck wrote: > I'm allowed to use ::sqrt(), right? Use SkScalarSqrt() instead
Pls look at SkFlattenable comment: https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... File src/core/SkNormalSourceImpl.cpp (right): https://codereview.chromium.org/2043393002/diff/1/src/core/SkNormalSourceImpl... src/core/SkNormalSourceImpl.cpp:91: inout->mulByUnknownFourComponents(); On 2016/06/10 14:23:27, egdaniel wrote: > On 2016/06/09 18:51:29, dvonbeck wrote: > > On 2016/06/09 16:56:38, egdaniel wrote: > > > Since this guy doesn't actually use his input, and we don't know what the > > > normals he will produce are, I would do inout->setUnknownFourComponents() > > > > The object doesn't have a setUnknownFourComponents() method. > > Sorry that was the function on the inner struct. This should be a setToUnknown > or something similar to that. Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:56: sk_sp<const SkLightingShader::NormalSource> normalSource) On 2016/06/10 14:23:28, egdaniel wrote: > putting a const inside of an sk_sp is generally a bad thing to do. Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:71: fNormalSource = normalSource; On 2016/06/10 14:23:28, egdaniel wrote: > use std::move( Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:122: sk_sp<const SkLightingShader::NormalSource> fNormalSource; On 2016/06/10 14:23:27, egdaniel wrote: > again const sk_sp is bad. Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:165: this->registerChildProcessor(normalFP); On 2016/06/10 14:23:27, egdaniel wrote: > use std::move(noramlFP) here Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:276: // TODO add comparison of NormalFPs On 2016/06/10 14:23:27, egdaniel wrote: > The base class of isEquals takes care of calling child processor onIsEqual so > this guy really just needs to check his own stuff. Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:351: /* TODO is this correct memory handling? FPs were changed to use sk_sp so it is different from On 2016/06/10 14:23:28, egdaniel wrote: > This should still be fine still Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:358: new LightingFP(diffuseTexture, diffM, diffParams, fLights, normalFP)); On 2016/06/10 14:23:27, egdaniel wrote: > I believe you want to use std::move here around normalFP Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:572: &diffLocalM, &normLocalM, normalSource); On 2016/06/10 14:23:27, egdaniel wrote: > wrap normalSouce in std::move Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.h:20: class NormalSource : public SkFlattenable { On 2016/06/10 14:23:28, egdaniel wrote: > For SK_API, it is believed that having it on the outer class should extend it to > all the inner objects including nested classes. So for now lets leave it out, > put it becomes very obvious if certain builds fail that this is the issue and we > can add it back. > > So talking with Mike Klein, we don't want this class to subclass SkFlattenable. > Instead just add functions (and just match Flattenable names) for the various > reads and writes for flattening. Then just call them from SkLightingShader's > flatten commands. This way you don't need this up in the enum. The problem with the approach you suggested is that normal source is an abstract class. To be able to recreate it from a byte stream without subclassing SkFlattenable I would need to encode the subclass in an enum that I set and then have a switch statement that forks into the appropriate constructor, etc. I feel like NormalSource should take advantage of the fact that serialization is already implemented. Let me know what you think. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.h:21: On 2016/06/10 14:23:28, egdaniel wrote: > remove \n between class and public Done. https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.h:35: SK_DEFINE_FLATTENABLE_TYPE(NormalSource) On 2016/06/10 14:23:28, egdaniel wrote: > These macros just declare/define various flattenable functions. If you want to > use the declare ones to get the names that is fine, or just declare them > manually yourself (especially since we won't be subclassing flattenable). Acknowledged. https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2043393002/diff/20001/tests/SerializationTest... tests/SerializationTest.cpp:576: SkVector invNormRotation = { sqrt(0.3), sqrt(0.7) }; On 2016/06/10 14:23:28, egdaniel wrote: > On 2016/06/09 18:51:29, dvonbeck wrote: > > I'm allowed to use ::sqrt(), right? > > Use SkScalarSqrt() instead Done.
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. Execution should be unaffected though. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 ========== to ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. Execution should be unaffected though. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 ==========
dvonbeck@google.com changed reviewers: + reed@google.com
Hi Mike, Here's the code changes I was talking about. All the comments marked with FLATTENABLE point to the relevant code snippets. Please let me know if there's any problems! https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.h:20: class NormalSource : public SkFlattenable { On 2016/06/10 15:22:29, dvonbeck wrote: > On 2016/06/10 14:23:28, egdaniel wrote: > > For SK_API, it is believed that having it on the outer class should extend it > to > > all the inner objects including nested classes. So for now lets leave it out, > > put it becomes very obvious if certain builds fail that this is the issue and > we > > can add it back. > > > > So talking with Mike Klein, we don't want this class to subclass > SkFlattenable. > > Instead just add functions (and just match Flattenable names) for the various > > reads and writes for flattening. Then just call them from SkLightingShader's > > flatten commands. This way you don't need this up in the enum. > > The problem with the approach you suggested is that normal source is an abstract > class. To be able to recreate it from a byte stream without subclassing > SkFlattenable I would need to encode the subclass in an enum that I set and then > have a switch statement that forks into the appropriate constructor, etc. I feel > like NormalSource should take advantage of the fact that serialization is > already implemented. Let me know what you think. Done. https://codereview.chromium.org/2043393002/diff/40001/include/core/SkFlattena... File include/core/SkFlattenable.h (right): https://codereview.chromium.org/2043393002/diff/40001/include/core/SkFlattena... include/core/SkFlattenable.h:84: kNormalSource_Type, FLATTENABLE: Added NormalSource flattenable type enum https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:565: sk_sp<SkLightingShader::NormalSource> normalSource( FLATTENABLE: Reading NormalSource from bytestream https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:598: buf.writeFlattenable(fNormalSource.get()); FLATTENABLE: Writing NormalSource into buffer here https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader.h:32: SK_DEFINE_FLATTENABLE_TYPE(NormalSource) FLATTENABLE: Macro https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader.h:40: SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP() FLATTENABLE: Macro https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... File src/core/SkLightingShader_NormalSource.cpp (right): https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader_NormalSource.cpp:43: SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(NormalMapSourceImpl) FLATTENABLE: Macro https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader_NormalSource.cpp:232: sk_sp<SkFlattenable> NormalMapSourceImpl::CreateProc(SkReadBuffer& buf) { FLATTENABLE: Reading NormalMapSourceImpl from buffer https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader_NormalSource.cpp:256: void NormalMapSourceImpl::flatten(SkWriteBuffer& buf) const { FLATTENABLE: Flattening NormalMapSourceImpl https://codereview.chromium.org/2043393002/diff/40001/src/ports/SkGlobalIniti... File src/ports/SkGlobalInitialization_default.cpp (right): https://codereview.chromium.org/2043393002/diff/40001/src/ports/SkGlobalIniti... src/ports/SkGlobalInitialization_default.cpp:91: SkLightingShader::NormalMapSource::InitializeFlattenables(); FLATTENABLE: Initializing SkFlattenable registrar https://codereview.chromium.org/2043393002/diff/40001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2043393002/diff/40001/tests/SerializationTest... tests/SerializationTest.cpp:552: // Test SkLightingShader/NormalMapSource serialization FLATTENABLE: Testing serialization
https://codereview.chromium.org/2043393002/diff/40001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/2043393002/diff/40001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:24: static bool bitmapIsTooBig(const SkBitmap&); 1. Needs a comment explaining what the criteria of "too big" is (and why) 2. Static Methods Begin With Capitals https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader.h:24: virtual sk_sp<GrFragmentProcessor> asFragmentProcessor( use override instead
https://codereview.chromium.org/2043393002/diff/40001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/2043393002/diff/40001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:24: static bool bitmapIsTooBig(const SkBitmap&); On 2016/06/10 19:15:23, reed1 wrote: > 1. Needs a comment explaining what the criteria of "too big" is (and why) > 2. Static Methods Begin With Capitals Done. https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/40001/src/core/SkLightingShad... src/core/SkLightingShader.h:24: virtual sk_sp<GrFragmentProcessor> asFragmentProcessor( On 2016/06/10 19:15:23, reed1 wrote: > use override instead This is not overriding anything, it's the NormalSource base class, which doesn't inherit from SkShader.
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. DO NOT LAND YET. Implementation's structure is clunky, lacking comments, and does not comply with style guide. Execution should be unaffected though. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 ========== to ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 ==========
api lgtm
https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:144: #include "SkBitmapProcShader.h" alphabetize this in include list (above SkGr.h) https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.h:29: /** Returns a fragment processor that takes no input and outputs a normal (already rotated) in general the no input here isn't true. I believe the input will be an unnormalized vector (distance and direction) to closest edge. It is up to the subclasses how they will treat this input https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.h:100: static sk_sp<SkShader> Make(const SkBitmap& diffuse, const SkBitmap& normal, We should probably also have a version here that takes a NormalSource instead of a normal bitmap https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader_NormalSource.cpp (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader_NormalSource.cpp:16: SkLightingShader::NormalSource::~NormalSource(){} nit: add space between () and {}
https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.h:100: static sk_sp<SkShader> Make(const SkBitmap& diffuse, const SkBitmap& normal, On 2016/06/14 18:30:45, egdaniel wrote: > We should probably also have a version here that takes a NormalSource instead of > a normal bitmap Just saw this change in the next CL :)
https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:144: #include "SkBitmapProcShader.h" On 2016/06/14 18:30:45, egdaniel wrote: > alphabetize this in include list (above SkGr.h) Done. https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.h:29: /** Returns a fragment processor that takes no input and outputs a normal (already rotated) On 2016/06/14 18:30:45, egdaniel wrote: > in general the no input here isn't true. I believe the input will be an > unnormalized vector (distance and direction) to closest edge. It is up to the > subclasses how they will treat this input That would be coming in as a universal though, right? Not as a direct color input to the fragment shader. https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.h:100: static sk_sp<SkShader> Make(const SkBitmap& diffuse, const SkBitmap& normal, On 2016/06/14 18:35:48, egdaniel wrote: > On 2016/06/14 18:30:45, egdaniel wrote: > > We should probably also have a version here that takes a NormalSource instead > of > > a normal bitmap > > Just saw this change in the next CL :) Acknowledged. https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader_NormalSource.cpp (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader_NormalSource.cpp:16: SkLightingShader::NormalSource::~NormalSource(){} On 2016/06/14 18:30:45, egdaniel wrote: > nit: add space between () and {} Done.
https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:144: #include "SkBitmapProcShader.h" On 2016/06/14 18:41:44, dvonbeck wrote: > On 2016/06/14 18:30:45, egdaniel wrote: > > alphabetize this in include list (above SkGr.h) > > Done. is it? https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader.h (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader.h:29: /** Returns a fragment processor that takes no input and outputs a normal (already rotated) On 2016/06/14 18:41:44, dvonbeck wrote: > On 2016/06/14 18:30:45, egdaniel wrote: > > in general the no input here isn't true. I believe the input will be an > > unnormalized vector (distance and direction) to closest edge. It is up to the > > subclasses how they will treat this input > > That would be coming in as a universal though, right? Not as a direct color > input to the fragment shader. Ah right this is asFragmentProcessor, I was thinking about the general NormalSource. Talking to Mike, I think we should update the comment for NormalSource to say something along the lines of it will take a matrix and position as input, and will have ways to access necessary geometry information in order to generate normals. https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... File src/core/SkLightingShader_NormalSource.cpp (right): https://codereview.chromium.org/2043393002/diff/80001/src/core/SkLightingShad... src/core/SkLightingShader_NormalSource.cpp:16: SkLightingShader::NormalSource::~NormalSource(){} On 2016/06/14 18:41:44, dvonbeck wrote: > On 2016/06/14 18:30:45, egdaniel wrote: > > nit: add space between () and {} > > Done. ? Generally we respond done, after uploading the new patch which makes the change
lgtm
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/2043393002/#ps100001 (title: "Addressed patch 5 comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043393002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2043393002/#ps120001 (title: "Removed use of VLAs, sufixed floats with 'f'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043393002/120001
Message was sent while issue was closed.
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 ========== to ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2062133004/ by egdaniel@google.com. The reason for reverting is: Breaking build and deps roll. Need to move include of SkBitmapProcShader in SkLightingShader.cpp from gpu include list to general list..
Message was sent while issue was closed.
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da ========== to ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da ==========
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2043393002/#ps140001 (title: "Moved include out of #if SK_SUPPORT_GPU")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043393002/140001
The CQ bit was unchecked by dvonbeck@google.com
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2043393002/#ps160001 (title: "Fixed memory leak on serializationtest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043393002/160001
On 2016/06/15 16:53:06, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/2043393002/160001 The patch passed CQ but it's not being commited yet. How come?
On 2016/06/15 17:29:48, dvonbeck wrote: > On 2016/06/15 16:53:06, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/2043393002/160001 > > The patch passed CQ but it's not being commited yet. How come? Nevermind, I realized the tree is closed.
Message was sent while issue was closed.
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da ========== to ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da Committed: https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2068983005/ by egdaniel@google.com. The reason for reverting is: break deps roll.
Message was sent while issue was closed.
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da Committed: https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318 ========== to ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da Committed: https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318 ==========
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2043393002/#ps180001 (title: "Fixed SK_API flags, got rid of NormalMapSource::Make")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043393002/180001
Message was sent while issue was closed.
Description was changed from ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da Committed: https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318 ========== to ========== Refactoring of GPU NormalMap handling out into its own class. The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources. What this CL includes: - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP. - Encapsulates this new fragment processor on a new class NormalMapSource. - Created a NormalSource abstraction that will interface with SkLightingShader. - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002 Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da Committed: https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318 Committed: https://skia.googlesource.com/skia/+/8811e40850ac3310c17fe8cdaffe72817a5e317d ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/8811e40850ac3310c17fe8cdaffe72817a5e317d
Message was sent while issue was closed.
CQ bit was unchecked |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
