Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2080993002:
Added API for Bevel NormalSource. (Closed)
DescriptionAdded API for Bevel NormalSource.
This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time.
This CL's base is the CL for accepting nullptrs: https://codereview.chromium.org/2132113002
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002
Committed: https://skia.googlesource.com/skia/+/bba4cfebcabba3aca5c2452d7df1853258bd701c
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Style fixes #Patch Set 4 : Small init fix #
Total comments: 21
Patch Set 5 : Rebased, addressed patch 4 comments #
Total comments: 18
Patch Set 6 : Addressed patch 5 comments #Patch Set 7 : Style fixes, serialization fixes #
Total comments: 8
Patch Set 8 : Addressed patch 7 comments #
Total comments: 14
Patch Set 9 : Addressed patch 8 comments #
Total comments: 6
Patch Set 10 : Addressed patch 9 comments #
Total comments: 8
Patch Set 11 : Addressed patch 10 comments #Patch Set 12 : Fixed small type problem #Patch Set 13 : rebase #Patch Set 14 : Fixed small problem with bevel type label #Patch Set 15 : Fixed forward declarations #Patch Set 16 : fixed unused field #
Dependent Patchsets: Messages
Total messages: 53 (24 generated)
Description was changed from ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. It also adds a default NormalSource for SkLightingShader when the argument passed is nullptr. BUG=skia: ========== to ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. It also adds a default NormalSource for SkLightingShader when the argument passed is nullptr. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ==========
Description was changed from ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. It also adds a default NormalSource for SkLightingShader when the argument passed is nullptr. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ========== to ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. It also adds a default NormalSource for SkLightingShader when the argument passed is nullptr. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com
rebase
robertphillips@google.com changed reviewers: + robertphillips@google.com
https://codereview.chromium.org/2080993002/diff/60001/gm/lightingshader.cpp File gm/lightingshader.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:62: protected: Would it make sense to convert this to an enum of normal sources e.g., kHemiNormalMap_Source ... kBevel_Source ? We would then precompute and store sk_sp<SkNormalSource>'s in onOnceBeforeDraw. https://codereview.chromium.org/2080993002/diff/60001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:122: SkPaint paint; Add '\n' https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:497: } Hmmm, don't we just want the flat guy? https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:336: SkNormalSource::Provider* asProvider(const SkShader::ContextRec& rec, void* storage) const orphan https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:363: This is the base class. Does it need to be friended ? https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:372: ?? https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:474: void NormalBevelSourceImpl::Provider::fillScanLine(int x, int y, SkPoint3 output[], int count) orphan https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.h:76: @param type the type of bevel to add Mention what space 'width' and 'height' are in. Source, right ? https://codereview.chromium.org/2080993002/diff/60001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/tests/SerializationTest... tests/SerializationTest.cpp:605: reporter)); Do we need to hard cast to a NormalBevelSourceImpl and just call operator== ?
Rebased and addressed comments. Should be g2g after lgtm. https://codereview.chromium.org/2080993002/diff/60001/gm/lightingshader.cpp File gm/lightingshader.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:62: protected: On 2016/07/11 19:13:21, robertphillips wrote: > Would it make sense to convert this to an enum of normal sources e.g., > kHemiNormalMap_Source > ... > kBevel_Source > > ? > > We would then precompute and store sk_sp<SkNormalSource>'s in onOnceBeforeDraw. Can't do it. SkNormalSource's SkBitmapProcShader needs the local matrix to know the coordinates from which to sample the normals. https://codereview.chromium.org/2080993002/diff/60001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:122: SkPaint paint; On 2016/07/11 19:13:21, robertphillips wrote: > Add '\n' Done. https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:497: } On 2016/07/11 19:13:21, robertphillips wrote: > Hmmm, don't we just want the flat guy? This was before i wrote the flat guy. Now that I uploaded the rebase the flat guy is used here. https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:336: SkNormalSource::Provider* asProvider(const SkShader::ContextRec& rec, void* storage) const On 2016/07/11 19:13:21, robertphillips wrote: > orphan Done. https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:363: On 2016/07/11 19:13:21, robertphillips wrote: > This is the base class. Does it need to be friended ? No, it used to be when the structure was different. I removed it now. https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:372: On 2016/07/11 19:13:21, robertphillips wrote: > ?? Done. https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:474: void NormalBevelSourceImpl::Provider::fillScanLine(int x, int y, SkPoint3 output[], int count) On 2016/07/11 19:13:21, robertphillips wrote: > orphan Done. https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.h:76: @param type the type of bevel to add On 2016/07/11 19:13:21, robertphillips wrote: > Mention what space 'width' and 'height' are in. Source, right ? Yes. Done. https://codereview.chromium.org/2080993002/diff/60001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/tests/SerializationTest... tests/SerializationTest.cpp:605: reporter)); On 2016/07/11 19:13:21, robertphillips wrote: > Do we need to hard cast to a NormalBevelSourceImpl and just call operator== ? Not that easy, because serializing/deserializing creates a deep copy of the heap-allocated fields, so operator== would need to properly compare the DAG of components all the way down.
https://codereview.chromium.org/2080993002/diff/80001/gm/lightingshader.cpp File gm/lightingshader.cpp (right): https://codereview.chromium.org/2080993002/diff/80001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:116: This is fine. There is a SkBitmap::getBounds entry point though. https://codereview.chromium.org/2080993002/diff/80001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:149: Every other block draws the un-rotated then rotated versions. Should we eliminate kFrustum_NormalMap and just call drawBevelRect twice ? https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:477: NormalBevelSourceImpl(BevelType type, SkScalar width, SkScalar height) These seem over tabbed https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:535: public: Need to init your member variables to something here. https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:558: private: Usually these would be named fPrevType, fPrevWidth ... https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:603: NormalBevelSourceImpl::Provider::Provider(const NormalBevelSourceImpl& source) Over tabbed https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:618: int count) const { I think we're leaning to always having auto as the loop variable https://codereview.chromium.org/2080993002/diff/80001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2080993002/diff/80001/tests/SerializationTest... tests/SerializationTest.cpp:617: SkAutoTUnref<SkNormalSource>(TestFlattenableSerialization(bevelSource.get(), true, line this guy up ?
https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:497: } On 2016/07/13 14:23:37, dvonbeck wrote: > On 2016/07/11 19:13:21, robertphillips wrote: > > Hmmm, don't we just want the flat guy? > > This was before i wrote the flat guy. Now that I uploaded the rebase the flat > guy is used here. This doesn't look like the flat guy.
https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:497: } On 2016/07/13 14:40:22, robertphillips wrote: > On 2016/07/13 14:23:37, dvonbeck wrote: > > On 2016/07/11 19:13:21, robertphillips wrote: > > > Hmmm, don't we just want the flat guy? > > > > This was before i wrote the flat guy. Now that I uploaded the rebase the flat > > guy is used here. > > This doesn't look like the flat guy. Patch 5 no longer has modifications to lighting shader. Once this CL is landed (after the nullptr CL lands) it will not modify SkLightingShader.cpp, so the Make function will continue using the flat guy. https://codereview.chromium.org/2080993002/diff/80001/gm/lightingshader.cpp File gm/lightingshader.cpp (right): https://codereview.chromium.org/2080993002/diff/80001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:116: On 2016/07/13 14:39:02, robertphillips wrote: > This is fine. There is a SkBitmap::getBounds entry point though. Acknowledged. https://codereview.chromium.org/2080993002/diff/80001/gm/lightingshader.cpp#n... gm/lightingshader.cpp:149: On 2016/07/13 14:39:02, robertphillips wrote: > Every other block draws the un-rotated then rotated versions. Should we > eliminate kFrustum_NormalMap and just call drawBevelRect twice ? We could but I don't see a reason why. Having both be possible can help if one method has a bug introduced and the other one doesn't. https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:477: NormalBevelSourceImpl(BevelType type, SkScalar width, SkScalar height) On 2016/07/13 14:39:02, robertphillips wrote: > These seem over tabbed Done. https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:535: public: On 2016/07/13 14:39:02, robertphillips wrote: > Need to init your member variables to something here. Done. Why? https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:558: private: On 2016/07/13 14:39:02, robertphillips wrote: > Usually these would be named fPrevType, fPrevWidth ... Done. https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:603: NormalBevelSourceImpl::Provider::Provider(const NormalBevelSourceImpl& source) On 2016/07/13 14:39:02, robertphillips wrote: > Over tabbed Done. https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:618: int count) const { On 2016/07/13 14:39:02, robertphillips wrote: > I think we're leaning to always having auto as the loop variable I don't feel like it's appropriate here, being an int and not an iterator or reference to the element. https://codereview.chromium.org/2080993002/diff/80001/tests/SerializationTest... File tests/SerializationTest.cpp (right): https://codereview.chromium.org/2080993002/diff/80001/tests/SerializationTest... tests/SerializationTest.cpp:617: SkAutoTUnref<SkNormalSource>(TestFlattenableSerialization(bevelSource.get(), true, On 2016/07/13 14:39:02, robertphillips wrote: > line this guy up ? Done. https://codereview.chromium.org/2080993002/diff/80001/tests/SerializationTest... tests/SerializationTest.cpp:619: // TODO test equality? @robertphillips I know we talked about the comparison being possible, but I realized that for the comparison to work, SkNormalSource needs to be casted down to the sub-class, which is not possible because the sub-class is not exposed through the header.
https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/80001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:535: public: On 2016/07/13 16:09:16, dvonbeck wrote: > On 2016/07/13 14:39:02, robertphillips wrote: > > Need to init your member variables to something here. > > Done. Why? Normally, in onSetData you have some code like: if (fPrevType != normalBevelFP.fType) { // upload new bevel type as a uniform fPrevType = normalBevelFP.fType; } if you don't initialize fPrevType in the constructor then the comparison that uses it is rather dodgy on the first call to onSetData. For this reason the GL*FP classes usually initialize their fPrev* member variables to invalid values for each type.
https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/60001/src/core/SkNormalSource... src/core/SkNormalSource.cpp:363: On 2016/07/13 14:23:37, dvonbeck wrote: > On 2016/07/11 19:13:21, robertphillips wrote: > > This is the base class. Does it need to be friended ? > > No, it used to be when the structure was different. I removed it now. Nevermind, it is necessary for serialization to work.
Description was changed from ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. It also adds a default NormalSource for SkLightingShader when the argument passed is nullptr. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ========== to ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. his CL's base is the CL for accepting nullptrs: https://codereview.chromium.org/2132113002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ==========
Description was changed from ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. his CL's base is the CL for accepting nullptrs: https://codereview.chromium.org/2132113002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ========== to ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. This CL's base is the CL for accepting nullptrs: https://codereview.chromium.org/2132113002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ==========
I think at this point a design doc would be extremely useful! https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.cpp:475: class SK_API NormalBevelSourceImpl : public SkNormalSource { I feel like this should be in its own file. Same with the Bitmap version. A similar pattern to how we have SkGradientShader in one file, and the its sub classes (LinearGradient, RadialGradient, etc.) all in their own files. https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.h:72: enum class BevelType { Can you add comment on what these different BevelTypes mean. https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.h:84: static sk_sp<SkNormalSource> MakeBevel(BevelType, SkScalar width, SkScalar height); We need more info in the comments here. What happens with bevels that are much wider than the shape? Can the bevels be negative? Can the height be negative (inward sloping things).
https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.h:72: enum class BevelType { On 2016/07/14 17:23:30, egdaniel wrote: > Can you add comment on what these different BevelTypes mean. Do you know if I can change my source file's encoding to UTF? I'd like to do stuff like ◜¯¯¯¯◝ or ◝------◜ in the comments to make it easier to visualize the different types.
https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.h:72: enum class BevelType { On 2016/07/19 23:00:13, dvonbeck wrote: > On 2016/07/14 17:23:30, egdaniel wrote: > > Can you add comment on what these different BevelTypes mean. > > Do you know if I can change my source file's encoding to UTF? I'd like to do > stuff like ◜¯¯¯¯◝ or ◝------◜ in the comments to make it easier to visualize the > different types. I wouldn't change the encoding. Just describe in mathematical words and folks should be able to understand. Also I was thinking the other day that cubic bevels would also be nice. With them we can have C1 continuity of the edges with but the surface and the top of the object.
I split up the files and added more meaningful comments to the bevel normalsource. I'm not if the mathematical explanation I went into is clear or necessary though. Pls let me know! https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... File src/core/SkNormalSource.cpp (right): https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.cpp:475: class SK_API NormalBevelSourceImpl : public SkNormalSource { On 2016/07/14 17:23:30, egdaniel wrote: > I feel like this should be in its own file. Same with the Bitmap version. A > similar pattern to how we have SkGradientShader in one file, and the its sub > classes (LinearGradient, RadialGradient, etc.) all in their own files. Done. https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.h:72: enum class BevelType { On 2016/07/19 23:31:26, egdaniel wrote: > On 2016/07/19 23:00:13, dvonbeck wrote: > > On 2016/07/14 17:23:30, egdaniel wrote: > > > Can you add comment on what these different BevelTypes mean. > > > > Do you know if I can change my source file's encoding to UTF? I'd like to do > > stuff like ◜¯¯¯¯◝ or ◝------◜ in the comments to make it easier to visualize > the > > different types. > > I wouldn't change the encoding. Just describe in mathematical words and folks > should be able to understand. > > Also I was thinking the other day that cubic bevels would also be nice. With > them we can have C1 continuity of the edges with but the surface and the top of > the object. Done. https://codereview.chromium.org/2080993002/diff/120001/src/core/SkNormalSourc... src/core/SkNormalSource.h:84: static sk_sp<SkNormalSource> MakeBevel(BevelType, SkScalar width, SkScalar height); On 2016/07/14 17:23:30, egdaniel wrote: > We need more info in the comments here. What happens with bevels that are much > wider than the shape? Can the bevels be negative? Can the height be negative > (inward sloping things). Done.
https://codereview.chromium.org/2080993002/diff/140001/gm/lightingshader.cpp File gm/lightingshader.cpp (right): https://codereview.chromium.org/2080993002/diff/140001/gm/lightingshader.cpp#... gm/lightingshader.cpp:158: this->drawBevelRect(canvas, r); do we really want to change this GM from using normalMaps? With the change the GM is going to change but then will change again slowly as we start fixing beveling and the GeomProcs. Seems like we should leave this GM as is and make a new one to test beveling where we can adjust all the various bevel params in the one GM. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:28: #if SK_SUPPORT_GPU can we share the #if sk_support_gpu with the includes? https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:62: fPrevType = normalBevelFP.fType; can we put this in the form Rob suggested earlier, if (fPrevType != normalBevelFP.fType) { // upload new bevel type as a uniform fPrevType = normalBevelFP.fType; } obviously for now there will be no uploading of the uniform https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:68: SkNormalSource::BevelType fPrevType; Do you need to store the type here? Seems like this is something that will effect the emitCode but isn't actually a uniform for setData. In emitCode we can always query it off the passed in GrProcessor. It will also need to be used in the key but that can wait till we actually use it in emitCode. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:129: output[i] = {0, 0, 1.0}; 1.0f? https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalFlatS... File src/core/SkNormalFlatSource.cpp (right): https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalFlatS... src/core/SkNormalFlatSource.cpp:28: #if SK_SUPPORT_GPU merge sk_support_gpus https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalSourc... src/core/SkNormalSource.h:85: enum class BevelType { Change this to what we just discussed. Make sure to add that any d > width has normal (0,0,1).
https://codereview.chromium.org/2080993002/diff/140001/gm/lightingshader.cpp File gm/lightingshader.cpp (right): https://codereview.chromium.org/2080993002/diff/140001/gm/lightingshader.cpp#... gm/lightingshader.cpp:158: this->drawBevelRect(canvas, r); On 2016/07/25 15:48:00, egdaniel wrote: > do we really want to change this GM from using normalMaps? With the change the > GM is going to change but then will change again slowly as we start fixing > beveling and the GeomProcs. Seems like we should leave this GM as is and make a > new one to test beveling where we can adjust all the various bevel params in the > one GM. Agreed. Done. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... File src/core/SkNormalBevelSource.cpp (right): https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:28: #if SK_SUPPORT_GPU On 2016/07/25 15:48:00, egdaniel wrote: > can we share the #if sk_support_gpu with the includes? I felt like it was clearer this way when there were multiple GPU code blocks in the same file, but now that there's only one I guess it sounds better to just put the includes inside it. Done. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:62: fPrevType = normalBevelFP.fType; On 2016/07/25 15:48:00, egdaniel wrote: > can we put this in the form Rob suggested earlier, > > if (fPrevType != normalBevelFP.fType) { > // upload new bevel type as a uniform > fPrevType = normalBevelFP.fType; > } > > obviously for now there will be no uploading of the uniform Done. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:68: SkNormalSource::BevelType fPrevType; On 2016/07/25 15:48:00, egdaniel wrote: > Do you need to store the type here? Seems like this is something that will > effect the emitCode but isn't actually a uniform for setData. In emitCode we can > always query it off the passed in GrProcessor. It will also need to be used in > the key but that can wait till we actually use it in emitCode. Done. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalBevel... src/core/SkNormalBevelSource.cpp:129: output[i] = {0, 0, 1.0}; On 2016/07/25 15:48:00, egdaniel wrote: > 1.0f? Oops. Done. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalFlatS... File src/core/SkNormalFlatSource.cpp (right): https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalFlatS... src/core/SkNormalFlatSource.cpp:28: #if SK_SUPPORT_GPU On 2016/07/25 15:48:00, egdaniel wrote: > merge sk_support_gpus Done. https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/140001/src/core/SkNormalSourc... src/core/SkNormalSource.h:85: enum class BevelType { On 2016/07/25 15:48:00, egdaniel wrote: > Change this to what we just discussed. Make sure to add that any d > width has > normal (0,0,1). Done.
https://codereview.chromium.org/2080993002/diff/160001/gm/lightingshaderbevel... File gm/lightingshaderbevel.cpp (right): https://codereview.chromium.org/2080993002/diff/160001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:108: for (bool useTranslucentPaint : {true, false}) { Is there anything special with the bevel shader when using a translucent paint? Maybe this should just be left for the other lightshader gm you made. I would add something to test rotated geometries and different geometries (circles, rects, round rects, some convex patch, some concave path). https://codereview.chromium.org/2080993002/diff/160001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/160001/src/core/SkNormalSourc... src/core/SkNormalSource.h:107: * a perpendicular normal vector of <0,0,1>. What changes if h is negative, is confusing cause the normals you give are wrong in that case? Maybe instead of mentioning the normals, say the control point of the quadradic bezier is always at (w, 0, h). https://codereview.chromium.org/2080993002/diff/160001/src/core/SkNormalSourc... src/core/SkNormalSource.h:117: * a perpendicular normal vector of <1,0,0>. Same as above, but control point will be at (0,0,0).
https://codereview.chromium.org/2080993002/diff/160001/gm/lightingshaderbevel... File gm/lightingshaderbevel.cpp (right): https://codereview.chromium.org/2080993002/diff/160001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:108: for (bool useTranslucentPaint : {true, false}) { On 2016/07/26 19:54:41, egdaniel wrote: > Is there anything special with the bevel shader when using a translucent paint? > Maybe this should just be left for the other lightshader gm you made. > > I would add something to test rotated geometries and different geometries > (circles, rects, round rects, some convex patch, some concave path). At the time I was thinking the normal could get premul'd but now I realize that's not true. I took it out. https://codereview.chromium.org/2080993002/diff/160001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/160001/src/core/SkNormalSourc... src/core/SkNormalSource.h:107: * a perpendicular normal vector of <0,0,1>. On 2016/07/26 19:54:41, egdaniel wrote: > What changes if h is negative, is confusing cause the normals you give are wrong > in that case? Maybe instead of mentioning the normals, say the control point of > the quadradic bezier is always at (w, 0, h). Done. https://codereview.chromium.org/2080993002/diff/160001/src/core/SkNormalSourc... src/core/SkNormalSource.h:117: * a perpendicular normal vector of <1,0,0>. On 2016/07/26 19:54:41, egdaniel wrote: > Same as above, but control point will be at (0,0,0). Done.
https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... File gm/lightingshaderbevel.cpp (right): https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:122: paint.setAntiAlias(true); I would set AntiAlias for all the shapes https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:177: label.appendf("useNegativeHeight: %d", useNegativeHeight); info on the bevel height would be interesting to report as well here. https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:185: } lets also add a rotated column at the end, but we just can check the linear positive height case there. https://codereview.chromium.org/2080993002/diff/180001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/180001/src/core/SkNormalSourc... src/core/SkNormalSource.h:114: * Same as above, but with the control point at (0, 0, 0) instead. Ah I would actually copy the text for RoundOut, but just change the control point.
https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... File gm/lightingshaderbevel.cpp (right): https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:122: paint.setAntiAlias(true); On 2016/07/27 17:57:28, egdaniel wrote: > I would set AntiAlias for all the shapes Done. https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:177: label.appendf("useNegativeHeight: %d", useNegativeHeight); On 2016/07/27 17:57:28, egdaniel wrote: > info on the bevel height would be interesting to report as well here. Done. https://codereview.chromium.org/2080993002/diff/180001/gm/lightingshaderbevel... gm/lightingshaderbevel.cpp:185: } On 2016/07/27 17:57:28, egdaniel wrote: > lets also add a rotated column at the end, but we just can check the linear > positive height case there. Done. https://codereview.chromium.org/2080993002/diff/180001/src/core/SkNormalSource.h File src/core/SkNormalSource.h (right): https://codereview.chromium.org/2080993002/diff/180001/src/core/SkNormalSourc... src/core/SkNormalSource.h:114: * Same as above, but with the control point at (0, 0, 0) instead. On 2016/07/27 17:57:28, egdaniel wrote: > Ah I would actually copy the text for RoundOut, but just change the control > point. Hahaha I misinterpreted your comment. Done.
lgtm
The CQ bit was checked by dvonbeck@google.com
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: Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
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 Link to the patchset: https://codereview.chromium.org/2080993002/#ps240001 (title: "rebase")
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: Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
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 Link to the patchset: https://codereview.chromium.org/2080993002/#ps250001 (title: "Fixed small problem with bevel type label")
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: Build-Ubuntu-Clang-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
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 Link to the patchset: https://codereview.chromium.org/2080993002/#ps260001 (title: "Fixed forward declarations")
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 dvonbeck@google.com
The CQ bit was checked by dvonbeck@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 dvonbeck@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/2080993002/#ps280001 (title: "fixed unused field")
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 API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. This CL's base is the CL for accepting nullptrs: https://codereview.chromium.org/2132113002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 ========== to ========== Added API for Bevel NormalSource. This CL adds an API for Bevel normal source and a dummy implementation that returns a normal (0, 0, 1) every time. This CL's base is the CL for accepting nullptrs: https://codereview.chromium.org/2132113002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2080993002 Committed: https://skia.googlesource.com/skia/+/bba4cfebcabba3aca5c2452d7df1853258bd701c ==========
Message was sent while issue was closed.
Committed patchset #16 (id:280001) as https://skia.googlesource.com/skia/+/bba4cfebcabba3aca5c2452d7df1853258bd701c |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
