Improve elevation bounds for physical shape layers #8044
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The value was picked by looking at composition in the Flutter Gallery's "Elevation" demo page - in particular, this value works for the 16 and 24pt elevation demos. |
liyuqian
reviewed
Mar 5, 2019
flow/layers/physical_shape_layer.cc
Outdated
| // doesn't provide a way to calculate it. We fill this whole region | ||
| // and clip children to it so we don't need to join the child paint bounds. | ||
| SkRect bounds(path_.getBounds()); | ||
| bounds.outset(20.0, 20.0); | ||
| bounds.outset(elevation_ * 1.5, elevation_ * 1.5); |
There was a problem hiding this comment.
The following outset might be more accurate according to the physical model used by Flutter and Skia:
ex = (kLightRadius * device_pixel_ratio_ + bounds.width() * 0.5) / kLightHeight;
ey = (kLightRadius * device_pixel_ratio_ + bounds.height() * 0.5) / kLightHeight;
bounds.outset(elevation_ * ex, elevation_ * ey);
See
for the illustration.
There was a problem hiding this comment.
BTW, be careful about device_pixel_ratio_. It seems that all elevation_, kLightRadius, kLightHeight should be multiplied with device_pixel_ratio_. The ratio of elevation_ and kLightHeight cancels out, but you'll still need kLightRadius * device_pixel_ratio_.
There was a problem hiding this comment.
I've updated it and tried to incorporate your diagram as a comment. Does that look right?
liyuqian
reviewed
Mar 5, 2019
| // | ||
| // E = lx } x = (r + w/2)/h | ||
| // } => | ||
| // r + w/2 = hx } E = (l/h)(r + w/2) |
There was a problem hiding this comment.
Looks good! It would be better to add a few legends about what E, x, h, l mean.
10 tasks
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Mar 6, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Mar 6, 2019
This was referenced Mar 6, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Mar 6, 2019
engine-flutter-autoroll
added a commit
to flutter/flutter
that referenced
this pull request
Mar 6, 2019
flutter/engine@f4951df...471a2c8 git log f4951df..471a2c8 --no-merges --oneline 471a2c8 Send scroll events from the macOS shell (flutter/engine#8056) 2fe9c9b Roll src/third_party/skia 72542816cadb..801a9c16d81e (46 commits) (flutter/engine#8060) 3335764 Skip skp files in license check (flutter/engine#8050) 7f16789 Remove redundant thread checker in FML. (flutter/engine#8053) 840c523 Correct URL for Cirrus CI build status badge (flutter/engine#8054) 57c120a remove extra source files (flutter/engine#8052) 4773375 Used named conditionals for platform specific dependencies and suppress Android and Windows hooks on Mac. (flutter/engine#8051) 70a18b5 Add clang static analysis support to gn wrapper (flutter/engine#8047) b30f989 Improve elevation bounds for physical shape layers (flutter/engine#8044) e37bd27 Fix weak pointer use violations in shell and platform view. (flutter/engine#8046) dd80fc9 Add engine support for scrollwheel events (flutter/engine#7494) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (mklim@google.com), and stop the roller if necessary.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Mar 7, 2019
GaryQian
added a commit
to flutter/flutter
that referenced
this pull request
Mar 8, 2019
…ine (19 commits) (#28688) git log --oneline --no-merges f4951df..a48cd16 a48cd16 Update a11y word forward/back enum names (flutter/engine#8073) b5f59ed Delay the vsync callback till the frame start time specified by embedder. (flutter/engine#8072) 7426305 Mark const extern (flutter/engine#8077) d3f6d7a only partial rule revert (flutter/engine#8078) d71bfe5 Only build a full Dart SDK when building for the host system (flutter/engine#8071) de90dbf Refactor web configuration/ Add dartdevc (flutter/engine#7978) ff46dd3 Roll src/third_party/skia 4c1ea43a79b5..88b8d1124b72 (8 commits) (flutter/engine#8070) 80c6dd2 Roll src/third_party/skia 692122e3ef23..4c1ea43a79b5 (3 commits) (flutter/engine#8069) 68ed654 Roll src/third_party/skia 3c957d575c58..692122e3ef23 (6 commits) (flutter/engine#8067) ca0bac4 Revert "add signal to pointer kinds" (flutter/engine#8066) 3fb627f add signal to pointer kinds (flutter/engine#8065) 5a06afa Roll src/third_party/skia 801a9c16d81e..3c957d575c58 (19 commits) (flutter/engine#8063) a93d99d A11y callback (flutter/engine#8005) 3661d5e Re-land "Buffer lifecycle in WindowData" (flutter/engine#8032) 471a2c8 Send scroll events from the macOS shell (flutter/engine#8056) 2fe9c9b Roll src/third_party/skia 72542816cadb..801a9c16d81e (46 commits) (flutter/engine#8060) 3335764 Skip skp files in license check (flutter/engine#8050) 7f16789 Remove redundant thread checker in FML. (flutter/engine#8053) 840c523 Correct URL for Cirrus CI build status badge (flutter/engine#8054) 57c120a remove extra source files (flutter/engine#8052) 4773375 Used named conditionals for platform specific dependencies and suppress Android and Windows hooks on Mac. (flutter/engine#8051) 70a18b5 Add clang static analysis support to gn wrapper (flutter/engine#8047) b30f989 Improve elevation bounds for physical shape layers (flutter/engine#8044) e37bd27 Fix weak pointer use violations in shell and platform view. (flutter/engine#8046) dd80fc9 Add engine support for scrollwheel events (flutter/engine#7494)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the bounds calculation for shadows to be more correct rather than just a hard coded value of 20.0. The value of 20.0 was particularly problematic for larger elevations, which would introduce clipping of the shadow.