Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246243002:
Interactive Bevel Sample App (Closed)
DescriptionInteractive Bevel Sample App
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246243002
Committed: https://skia.googlesource.com/skia/+/6d391b6c37c66770913aaecc50ddb466705dde31
Patch Set 1 #Patch Set 2 : 2 ligths, labels, comments #
Total comments: 18
Patch Set 3 : Addressed patch 2 comments #
Total comments: 4
Patch Set 4 : Width maximum increased, controls now stay stationary regardless of CTM #
Total comments: 3
Patch Set 5 : Height can now be negative, fix missing 'f' on float literals #Patch Set 6 : Controls now use Rect::MakeXYWH, constants are now SkScalar #
Total comments: 4
Patch Set 7 : Initial position now a constant variable, number of bevel types now a constant variable, MaxWidth n… #
Total comments: 12
Patch Set 8 : Addressed patch set 7 comments #Messages
Total messages: 29 (9 generated)
Description was changed from ========== Interactive Bevel Sample App BUG=skia: ========== to ========== Interactive Bevel Sample App BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246243002 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com, robertphillips@google.com
https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:10: #include "SkNormalSource.h" Do we need the next three includes ? https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:18: BevelView() I don't know if you're getting any mileage out of having fRedLight & fBlueLight as member variables You probably get similar value by having a kRedLightColor & kBlurLightColor constants somewhere and having a helper routine to build the fLights object. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:37: it is odd that you touch fCtrlRangeRects twice with an intervening change to fWidthCtrlRect ... https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:87: paint.setColor(0xFFDDDDDD); missing ' ' here ? https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:114: use kNumControls ? https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:155: SkScalar yPos = fRect.height(); for (auto shape : { kCircle_Shape, kRect_Shape }) and remove kLast_Shape ? https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:263: What's this for ? https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:265: more descriptive name for 'fRect' ? can this be const ? It almost seems like you want kRectSize -> kGeometrySize and then just use that in drawShape https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:272: SkRect fTypeCtrlRect; Add kNumControls ?
https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:10: #include "SkNormalSource.h" On 2016/08/16 17:02:42, robertphillips wrote: > Do we need the next three includes ? I do need that last one, the other 2 are from a previous approach so I removed them. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:18: BevelView() On 2016/08/16 17:02:42, robertphillips wrote: > I don't know if you're getting any mileage out of having fRedLight & fBlueLight > as member variables > > You probably get similar value by having a kRedLightColor & kBlurLightColor > constants somewhere and having a helper routine to build the fLights object. I also use them to keep track of the state of light direction, since they change independently of each other so they need to be stored. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:37: On 2016/08/16 17:02:42, robertphillips wrote: > it is odd that you touch fCtrlRangeRects twice with an intervening change to > fWidthCtrlRect ... True! I changed it https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:87: paint.setColor(0xFFDDDDDD); On 2016/08/16 17:02:42, robertphillips wrote: > missing ' ' here ? Done. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:114: On 2016/08/16 17:02:42, robertphillips wrote: > use kNumControls ? Done. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:155: SkScalar yPos = fRect.height(); On 2016/08/16 17:02:42, robertphillips wrote: > for (auto shape : { kCircle_Shape, kRect_Shape }) > > and remove kLast_Shape ? Done. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:263: On 2016/08/16 17:02:42, robertphillips wrote: > What's this for ? Oops, leftover from old code. Removed. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:265: On 2016/08/16 17:02:42, robertphillips wrote: > more descriptive name for 'fRect' ? > > can this be const ? > > It almost seems like you want kRectSize -> kGeometrySize and then just use that > in drawShape Done. https://codereview.chromium.org/2246243002/diff/20001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:272: SkRect fTypeCtrlRect; On 2016/08/16 17:02:42, robertphillips wrote: > Add kNumControls ? Done.
lets come up with some way to visualize where the lights are on the screen. https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:202: fBevelHeight = (fHeightCtrlRect.fLeft / kCtrlRange) * kBevelHeightMax; Can height get negative? If not it should be able to.
On 2016/08/17 17:53:19, egdaniel wrote: > lets come up with some way to visualize where the lights are on the screen. > > https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.cpp > File samplecode/SampleBevel.cpp (right): > > https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.... > samplecode/SampleBevel.cpp:202: fBevelHeight = (fHeightCtrlRect.fLeft / > kCtrlRange) * kBevelHeightMax; > Can height get negative? If not it should be able to. If we want we can actually do the light visualization in a follow up CL, since I've got even more ideas for that :)
OK, let's leave it for another CL then haha https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:202: fBevelHeight = (fHeightCtrlRect.fLeft / kCtrlRange) * kBevelHeightMax; On 2016/08/17 17:53:18, egdaniel wrote: > Can height get negative? If not it should be able to. I tried it but all it does is look like the lights are flipped. Do you think it's worth it?
On 2016/08/17 17:56:29, dvonbeck wrote: > OK, let's leave it for another CL then haha > > https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.cpp > File samplecode/SampleBevel.cpp (right): > > https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.... > samplecode/SampleBevel.cpp:202: fBevelHeight = (fHeightCtrlRect.fLeft / > kCtrlRange) * kBevelHeightMax; > On 2016/08/17 17:53:18, egdaniel wrote: > > Can height get negative? If not it should be able to. > > I tried it but all it does is look like the lights are flipped. Do you think > it's worth it? Yes cause once you can visualize on the screen where the lights are it will make more sense
https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:35: fuse these two lines with MakeLTRB ? & for other 2 ?
https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:35: On 2016/08/17 18:11:43, robertphillips wrote: > fuse these two lines with MakeLTRB ? > > & for other 2 ? I feel like it is easier to read if I put it in 2 steps, no? https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/60001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:202: fBevelHeight = (fHeightCtrlRect.fLeft / kCtrlRange) * kBevelHeightMax; On 2016/08/17 17:56:29, dvonbeck wrote: > On 2016/08/17 17:53:18, egdaniel wrote: > > Can height get negative? If not it should be able to. > > I tried it but all it does is look like the lights are flipped. Do you think > it's worth it? Done.
https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:35: On 2016/08/17 18:28:40, dvonbeck wrote: > On 2016/08/17 18:11:43, robertphillips wrote: > > fuse these two lines with MakeLTRB ? > > > > & for other 2 ? > > I feel like it is easier to read if I put it in 2 steps, no? There is a MakeXYWH which I think is the best option here.
https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.cpp File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/40001/samplecode/SampleBevel.... samplecode/SampleBevel.cpp:35: On 2016/08/17 18:34:12, egdaniel wrote: > On 2016/08/17 18:28:40, dvonbeck wrote: > > On 2016/08/17 18:11:43, robertphillips wrote: > > > fuse these two lines with MakeLTRB ? > > > > > > & for other 2 ? > > > > I feel like it is easier to read if I put it in 2 steps, no? > > There is a MakeXYWH which I think is the best option here. Done.
https://codereview.chromium.org/2246243002/diff/100001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/100001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:47: fHeightCtrlRect = SkRect::MakeXYWH(0.75f * kCtrlRange, currY, lets make a constant kStartHeightSomeThing for 0.75 and similarly for the other bars. https://codereview.chromium.org/2246243002/diff/100001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:264: static constexpr SkScalar kBevelWidthMax = 100.0f; Can we make this equal to kShapeBoundsSize? Thus if we ever make the shapes bigger the width will still be wide "enough"
https://codereview.chromium.org/2246243002/diff/100001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/100001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:47: fHeightCtrlRect = SkRect::MakeXYWH(0.75f * kCtrlRange, currY, On 2016/08/17 19:12:54, egdaniel wrote: > lets make a constant kStartHeightSomeThing for 0.75 and similarly for the other > bars. Done. https://codereview.chromium.org/2246243002/diff/100001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:264: static constexpr SkScalar kBevelWidthMax = 100.0f; On 2016/08/17 19:12:54, egdaniel wrote: > Can we make this equal to kShapeBoundsSize? Thus if we ever make the shapes > bigger the width will still be wide "enough" Done.
https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:39: kSliderHeight); Well, since Greg sort of brought it up, would it make sense/be possible to have the ctrl rect structure parallel the ctrl range rects (i.e., have an array of them)? The ctrl selection code in the onClick handler could then loop over the array, fSelectedCtrlRect could be an int and, ideally, the parameter extraction could be standardized.
https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:39: kSliderHeight); On 2016/08/17 21:52:13, robertphillips wrote: > Well, since Greg sort of brought it up, would it make sense/be possible to have > the ctrl rect structure parallel the ctrl range rects (i.e., have an array of > them)? The ctrl selection code in the onClick handler could then loop over the > array, fSelectedCtrlRect could be an int and, ideally, the parameter extraction > could be standardized. I am actually working on this for my next CL. I am building a cleaner way of handling controls through a polymorphic pass-through of clicks so that I can share code between sliders and a directional control I am building for the lights. I can upload it to this one if you think it's necessary.
https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:70: SampleCode::TitleR(evt, "bevel"); We genearaly name samples with a capital letter and gm's with a lower case. So make this a 'B'
lgtm https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:39: kSliderHeight); On 2016/08/17 22:10:22, dvonbeck wrote: > On 2016/08/17 21:52:13, robertphillips wrote: > > Well, since Greg sort of brought it up, would it make sense/be possible to > have > > the ctrl rect structure parallel the ctrl range rects (i.e., have an array of > > them)? The ctrl selection code in the onClick handler could then loop over the > > array, fSelectedCtrlRect could be an int and, ideally, the parameter > extraction > > could be standardized. > > I am actually working on this for my next CL. I am building a cleaner way of > handling controls through a polymorphic pass-through of clicks so that I can > share code between sliders and a directional control I am building for the > lights. I can upload it to this one if you think it's necessary. Okay - let's leave it for the follow on. https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:85: We should probably be caching this and only recreating it when the parameters change. https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:132: SkPaint labelPaint; Can we cache off the typeface and reuse it ? https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:226: } should there be and else here ?
https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:39: kSliderHeight); On 2016/08/18 14:40:41, robertphillips wrote: > On 2016/08/17 22:10:22, dvonbeck wrote: > > On 2016/08/17 21:52:13, robertphillips wrote: > > > Well, since Greg sort of brought it up, would it make sense/be possible to > > have > > > the ctrl rect structure parallel the ctrl range rects (i.e., have an array > of > > > them)? The ctrl selection code in the onClick handler could then loop over > the > > > array, fSelectedCtrlRect could be an int and, ideally, the parameter > > extraction > > > could be standardized. > > > > I am actually working on this for my next CL. I am building a cleaner way of > > handling controls through a polymorphic pass-through of clicks so that I can > > share code between sliders and a directional control I am building for the > > lights. I can upload it to this one if you think it's necessary. > > Okay - let's leave it for the follow on. Acknowledged. https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:70: SampleCode::TitleR(evt, "bevel"); On 2016/08/18 14:33:28, egdaniel wrote: > We genearaly name samples with a capital letter and gm's with a lower case. So > make this a 'B' Done. https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:85: On 2016/08/18 14:40:41, robertphillips wrote: > We should probably be caching this and only recreating it when the parameters > change. Done. https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:132: SkPaint labelPaint; On 2016/08/18 14:40:41, robertphillips wrote: > Can we cache off the typeface and reuse it ? Done. https://codereview.chromium.org/2246243002/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:226: } On 2016/08/18 14:40:41, robertphillips wrote: > should there be and else here ? Done.
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 robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2246243002/#ps140001 (title: "Addressed patch set 7 comments")
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 ========== Interactive Bevel Sample App BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246243002 ========== to ========== Interactive Bevel Sample App BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246243002 Committed: https://skia.googlesource.com/skia/+/6d391b6c37c66770913aaecc50ddb466705dde31 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/6d391b6c37c66770913aaecc50ddb466705dde31 |
