[flow] Switch to directional shadows #27124

untp · 2021-07-01T21:30:27Z

Shadows incorrectly offset, shifts incrementally. This issue fixed by Skia with Issue 10781: Add orthographic shadow support. We need to add SkShadowFlags::kDirectionalLight_ShadowFlag to DrawShadow.

Related Issues

fixes flutter/flutter#28463
fixes flutter/flutter#51237

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

flar · 2021-07-02T18:04:14Z

flow/layers/physical_shape_layer.cc

 const SkScalar kLightHeight = 600;
-const SkScalar kLightRadius = 800;
+const SkScalar kLightXOffset = 0;
+const SkScalar kLightYOffset = -450;


The code used to use an offset of -600 from the top of the bounds, but the value is now hard-coded as a -450 offset directionally. Doesn't that produce a different effect?

Or is this change intentional due to undesired results of the previous fixed offset?

I don't know. I just copied these values from canvaskit/util.dart. We should ask this to @yjbanov.

The values we use on the web side don't have much science to them. We just eyeballed the result and checked with the material team. In the 6 months since the change we've received zero complaints about how the shadows look.

flow/layers/physical_shape_layer.cc

flow/layers/physical_shape_layer_unittests.cc

untp · 2021-07-03T19:53:44Z

I investigated GetLocalBounds for compatibility with old implementation.
Getting the same results of the old implementation is impossible.
So we need to choose values that gives a result closest to the old implementation.
Here are the simplified equations from GetLocalBounds:

ambientBlur = 0.5 * zPlane.z / ctm.getMinScale()
if kDirectionalLight_ShadowFlag in flags:
  spotBlur = lightRadius * zPlane.z / ctm.getMinScale()
  spotScale = 1
  spotOffset.x = - lightPos.normalize().x * zPlane.z / ctm.getMinScale()
  spotOffset.y = - lightPos.normalize().y * zPlane.z / ctm.getMinScale()
else:
  spotBlur = lightRadius * (zPlane.z / (lightPos.z - zPlane.z)) / ctm.getMinScale()
  spotScale = lightPos.z / (lightPos.z - zPlane.z)
  spotOffset.x = - (zPlane.z / (lightPos.z - zPlane.z)) *
                       ((lightPos.x - ctm.getTranslateX()) / ctm.getMinScale())
  spotOffset.y = - (zPlane.z / (lightPos.z - zPlane.z)) *
                       ((lightPos.y - ctm.getTranslateY()) / ctm.getMinScale())

If these equations applied to the old implementation:
(assume ctm.getMinScale() equal to dpr)

ambientBlur = 0.5 * elevation
spotBlur = kLightRadius * elevation / (kLightHeight - elevation)
spotScale = kLightHeight / (kLightHeight - elevation)
spotOffset.x = - elevation / (kLightHeight - elevation) *
    (((bounds.left() + bounds.right()) / 2 - ctm.getTranslateX()) / ctm.getMinScale())
spotOffset.y = - elevation / (kLightHeight - elevation) *
    ((bounds.top() - kLightHeight - ctm.getTranslateY()) / ctm.getMinScale())

These 5 variables needs to be equal between old and new implementation but it is impossible.
spotScale can not be equal because 1 != kLightHeight / (kLightHeight - elevation).
If we assume kLightHeight - elevation equals to kLightHeight, variables become:
(Also assuming ctm.getTranslateX() == (bounds.left() + bounds.right()) / 2
and assuming ctm.getTranslateY() == bounds.top())

ambientBlur = 0.5 * elevation
spotBlur = kLightRadius / kLightHeight * elevation
spotScale = 1
spotOffset.x = 0
spotOffset.y = elevation / ctm.getMinScale()

Let's find out new implementation's values:

0.5 * zPlane.z / ctm.getMinScale() = 0.5 * elevation
lightRadius * zPlane.z / ctm.getMinScale() = kLightRadius / kLightHeight * elevation
- zPlane.z * lightPos.normalize().x / ctm.getMinScale() = 0
- zPlane.z * lightPos.normalize().y / ctm.getMinScale() = elevation / ctm.getMinScale()

After simplifying:

zPlane.z = elevation * ctm.getMinScale()
lightRadius = kLightRadius / kLightHeight
lightPos.normalize().x = 0
lightPos.normalize().y = - 1 / ctm.getMinScale()

ctm.getMinScale() mostly greater than 1 but can be smaller than 1 on low resolution devices.
And normalize() can't return smaller than -1; so, we need to assume ctm.getMinScale() is 1 only for lightPos.normalize().y.
Therefore we set lightPos as (0, -1, 0).
So function call should be like this:
(Using dpr as ctm.getMinScale(), because sometimes ctm.getMinScale() != dpr)

SkShadowUtils::GetLocalBounds(
    ctm,
    path,
    SkPoint3::Make(0, 0, elevation * dpr),
    SkPoint3::Make(0, -1, 0),
    kLightRadius / kLightHeight,
    SkShadowFlags::kDirectionalLight_ShadowFlag,
    &shadow_bounds);

These are results of different rects calculated with old and new implementation:
(elevation = 1, dpr = 1, ctm = SkMatrix())

--------------------------------
rect: LTRB: -1, 0, 1, 1
old_impl: LTRB: -3.33723, -1.5, 3.33723, 4.3389
new_impl: LTRB: -3.33333, -1.5, 3.33333, 4.33333
--------------------------------
rect: LTRB: -10, 0, 10, 10
old_impl: LTRB: -12.3523, -1.5, 12.3523, 13.3539
new_impl: LTRB: -12.3333, -1.5, 12.3333, 13.3333
--------------------------------
rect: LTRB: -100, 0, 100, 100
old_impl: LTRB: -102.502, -1.5, 102.502, 103.504
new_impl: LTRB: -102.333, -1.5, 102.333, 103.333
--------------------------------
rect: LTRB: -1, 0, 1, 2
old_impl: LTRB: -3.33723, -1.5, 3.33723, 5.34057
new_impl: LTRB: -3.33333, -1.5, 3.33333, 5.33333
--------------------------------
rect: LTRB: -10, 0, 10, 20
old_impl: LTRB: -12.3523, -1.5, 12.3523, 23.3706
new_impl: LTRB: -12.3333, -1.5, 12.3333, 23.3333
--------------------------------
rect: LTRB: -100, 0, 100, 200
old_impl: LTRB: -102.502, -1.5, 102.502, 203.671
new_impl: LTRB: -102.333, -1.5, 102.333, 203.333
--------------------------------
rect: LTRB: 0, 0, 1, 1
old_impl: LTRB: -2.33639, -1.5, 3.33639, 4.3389
new_impl: LTRB: -2.33333, -1.5, 3.33333, 4.33333
--------------------------------
rect: LTRB: 0, 0, 10, 10
old_impl: LTRB: -2.34391, -1.5, 12.3439, 13.3539
new_impl: LTRB: -2.33333, -1.5, 12.3333, 13.3333
--------------------------------
rect: LTRB: 0, 0, 100, 100
old_impl: LTRB: -2.41903, -1.5, 102.419, 103.504
new_impl: LTRB: -2.33333, -1.5, 102.333, 103.333
--------------------------------
rect: LTRB: 1, 1, 2, 2
old_impl: LTRB: -1.33639, -0.5, 4.33639, 5.3389
new_impl: LTRB: -1.33333, -0.5, 4.33333, 5.33333
--------------------------------
rect: LTRB: 10, 10, 20, 20
old_impl: LTRB: 7.65609, 8.5, 22.3439, 23.3539
new_impl: LTRB: 7.66667, 8.5, 22.3333, 23.3333
--------------------------------
rect: LTRB: 100, 100, 200, 200
old_impl: LTRB: 97.581, 98.5, 202.419, 203.504
new_impl: LTRB: 97.6667, 98.5, 202.333, 203.333
--------------------------------

flar · 2021-07-03T20:39:26Z

I investigated GetLocalBounds for compatibility with old implementation.

I think that may be a mistake in tactics. This assumes that the old implementation was correct. In particular, when I was using the PhysicalShapeLayer to compute bounds for my DisplayList tests I ran into problems with it not enclosing all of the pixels that were rendered (the DL tests do OOB testing for rendering vs DL.bounds()). I ended up ignoring it and using GetLocalBounds in the shadow utils instead for DL bounds computations.

The goal here will be to make sure that the bounds computed encompass the rendering, not that they are compatible with the prior implementation of something. It would be nice if they were no looser than the old implementation as well, for performance.

With respect to rendering, we need to decide what the goal is here. Some of the comments linked above talked about not matching the Material spec, and I think we do need to ensure our Material widgets match the material spec.

We also need to consider that we have Cupertino widgets that may also use these shadows and need to be compatible with the Cupertino spec.

If they are dragging us in 2 different directions with how we interpret various values like elevation and light direction, then maybe we need to add some extra parameters to our shadow parameters, but hopefully they have similar enough shadow specs that we can meet them with the existing parameters.

untp · 2021-07-03T21:25:47Z

I think, people want to see same shadows after this fix. Because this PR edits code that 4 years old. Material specification is not strict, it doesn't provide values, it just say combine shadow from key and ambient lights.

I don't know what is the goal. I just fixed the above issues. Now shadows doesn't depend on ctm.

Maybe we need to invite someone from material design team.

Also here are some screenshots

Old implementation

New implementation

jvanverth · 2021-07-14T20:21:22Z

So the reason the resulting direction is unintuitive is that this case has exposed a bug in the directional lighting code. Rather than

spotOffset.x = - lightPos.normalize().x * zPlane.z / ctm.getMinScale()
spotOffset.y = - lightPos.normalize().y * zPlane.z / ctm.getMinScale()

it should be

spotOffset.x = - lightPos.x / lightPos.z * zPlane.z / ctm.getMinScale()
spotOffset.y = - lightPos.y / lightPos.z * zPlane.z / ctm.getMinScale()

(This also makes me think that the blur could be

spotBlur = lightRadius / lightPos.z * zPlane.z / ctm.getMinScale()

though either way it's sort of a hack. A true directional light wouldn't normally affect blur, since it's infinitely far away.)

In any case, with those changes lightPos should end up being (0, -sqrt(2)/2, sqrt(2)/2, which makes more sense as a light direction.

jvanverth · 2021-07-14T20:30:43Z

Oh, and once I land this fix it probably means that the Web values would have to change, depending on how sharp the lighting angle is.

flar · 2021-07-14T20:34:24Z

In any case, with those changes lightPos should end up being (0, -sqrt(2)/2, sqrt(2)/2, which makes more sense as a light direction.

Wouldn't (0, -1, 1) work as well since the values are "Z-normalized"?

jvanverth · 2021-07-14T20:43:18Z

Yeah, any vector pointing in that direction would work, I just normalized it out of habit.

yjbanov · 2021-07-14T21:07:28Z

Oh, and once I land this fix it probably means that the Web values would have to change, depending on how sharp the lighting angle is.

We can address the web side in a separate PR. We roll Skia with CanvasKit releases, so this won't take effect immediately. Thanks for the heads up though.

jvanverth · 2021-07-14T21:30:10Z

We can address the web side in a separate PR. We roll Skia with CanvasKit releases, so this won't take effect immediately. Thanks for the heads up though.

Sounds good, I'll land the fix tomorrow morning.

flar · 2021-07-14T21:31:14Z

When the Skia fix lands and this PR will have to be rebased to catch it, I think that may also fix the odd error in the Linux Android Scenarios as well.

flar · 2021-07-16T18:51:04Z

Just a status update. I'm on the CC thread for the Skia fix to the directional shadow math and it is actively being worked on. The delays are minor and related to making sure it passes all tests and golden image comparisons.

jvanverth · 2021-07-16T20:21:38Z

The fix has landed: https://skia-review.googlesource.com/c/skia/+/428978

flar · 2021-07-16T20:28:39Z

Try rebasing to >= c120a50 to check with the fix in place.

untp · 2021-07-16T20:31:44Z

I already rebased with previous commits. My branch's DEPS' skia_revision is 94fda947.

flar

LGTM

This is a reland of 6789b82 Original change's description: > Fix directional shadows. > > The xy offset calculation for drawShadow was not quite correct. Rather > than normalizing the light vector and using the xy values of that as the > base offset value, we should scale the light vector by 1/z. > > See flutter/engine#27124 (comment) > for more detail. > > Bug: skia:10781 > Change-Id: Ib69a313cb96a532f8d89644e3d69f666a184e897 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/428880 > Reviewed-by: Brian Salomon <[email protected]> > Commit-Queue: Jim Van Verth <[email protected]> Bug: skia:10781 Change-Id: Ib58d374aa03d0144512e5ded6ccd572c74783607 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/428978 Reviewed-by: Brian Salomon <[email protected]> Commit-Queue: Jim Van Verth <[email protected]>

…86596)

untp added 5 commits July 1, 2021 19:48

Update physical_shape_layer.cc

3f46f17

Update physical_shape_layer.h

45e761f

Update physical_shape_layer_unittests.cc

3559cb8

Update checkerboard_layertree_unittests.cc

b7fbbaf

Update physical_shape_layer.cc

6f245dd

google-cla bot added the cla: yes label Jul 1, 2021

untp added 3 commits July 2, 2021 00:46

Format physical_shape_layer.cc

c03a2f5

Format physical_shape_layer_unittests.cc

a8b7fe6

Format checkerboard_layertree_unittests.cc

daec720

untp mentioned this pull request Jul 1, 2021

Shadow gets weird when card is too long flutter/flutter#28463

Closed

untp added 4 commits July 2, 2021 01:31

Create restart-checks

176b32f

Delete restart-checks

d1cb536

Style physical_shape_layer.cc

c716332

Merge branch 'flutter:master' into untp-patch-1

7424300

zanderso requested review from chinmaygarde and flar July 2, 2021 15:34

flar requested changes Jul 2, 2021

View reviewed changes

flar requested a review from yjbanov July 2, 2021 19:21

untp added 7 commits July 3, 2021 22:54

Merge branch 'flutter:master' into untp-patch-1

bd5e29d

Update physical_shape_layer.cc

ec27cb7

Update physical_shape_layer_unittests.cc

1e61a4a

Format physical_shape_layer.cc

5c792d6

Format physical_shape_layer_unittests.cc

255b86b

Update display_list_utils.cc

b5587f9

Update display_list_utils.cc

52ab343

untp added 2 commits July 4, 2021 00:52

Fix display_list_utils.cc

9c4e273

Format display_list_utils.cc

80f07f9

untp added 2 commits July 16, 2021 21:37

Merge branch 'flutter:master' into untp-patch-1

00e8eb9

Update physical_shape_layer.cc

6e89050

flar approved these changes Jul 16, 2021

View reviewed changes

flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 16, 2021

fluttergithubbot merged commit 334fa80 into flutter:master Jul 17, 2021

engine-flutter-autoroll mentioned this pull request Jul 17, 2021

Roll Engine from 14a9e9a50e17 to 334fa806add1 (1 revision) flutter/flutter#86596

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 17, 2021

334fa80 [flow] Switch to directional shadows (flutter/engine#27124)

084b97d

flar mentioned this pull request Jul 17, 2021

add test for scaled playback of DL and SkPicture #27503

Merged

untp deleted the untp-patch-1 branch July 17, 2021 09:44

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Jul 19, 2021

334fa80 [flow] Switch to directional shadows (flutter/engine#27124) (#…

eb64852

…86596)

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

[flow] Switch to directional shadows (flutter#27124)

34a0e4c

flar mentioned this pull request Aug 27, 2021

Consider adding a custom recording format flutter/flutter#53501

Closed

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

[flow] Switch to directional shadows (flutter#27124)

3b1e65c

flar mentioned this pull request Sep 16, 2021

Shadows flicker when overlapping flutter/flutter#82998

Closed

HansMuller mentioned this pull request Oct 6, 2021

Translucent FloatingActionButton get's a persistent highlight when clicked/touched flutter/flutter#90753

Open

HadyMash mentioned this pull request Oct 18, 2021

Widgets in cacheExtent are not painted properly flutter/flutter#92036

Open

justinmc mentioned this pull request Dec 29, 2021

Dots appear under FAB with elevation without a background color flutter/flutter#90254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flow] Switch to directional shadows #27124

[flow] Switch to directional shadows #27124

untp commented Jul 1, 2021 •

edited

Loading

flar Jul 2, 2021

flar Jul 2, 2021

untp Jul 2, 2021

yjbanov Jul 6, 2021

untp commented Jul 3, 2021

flar commented Jul 3, 2021

untp commented Jul 3, 2021

jvanverth commented Jul 14, 2021 •

edited

Loading

jvanverth commented Jul 14, 2021

flar commented Jul 14, 2021

jvanverth commented Jul 14, 2021

yjbanov commented Jul 14, 2021

jvanverth commented Jul 14, 2021

flar commented Jul 14, 2021

flar commented Jul 16, 2021

jvanverth commented Jul 16, 2021

flar commented Jul 16, 2021

untp commented Jul 16, 2021

flar left a comment

[flow] Switch to directional shadows #27124

[flow] Switch to directional shadows #27124

Conversation

untp commented Jul 1, 2021 • edited Loading

Related Issues

Pre-launch Checklist

flar Jul 2, 2021

Choose a reason for hiding this comment

flar Jul 2, 2021

Choose a reason for hiding this comment

untp Jul 2, 2021

Choose a reason for hiding this comment

yjbanov Jul 6, 2021

Choose a reason for hiding this comment

untp commented Jul 3, 2021

flar commented Jul 3, 2021

untp commented Jul 3, 2021

jvanverth commented Jul 14, 2021 • edited Loading

jvanverth commented Jul 14, 2021

flar commented Jul 14, 2021

jvanverth commented Jul 14, 2021

yjbanov commented Jul 14, 2021

jvanverth commented Jul 14, 2021

flar commented Jul 14, 2021

flar commented Jul 16, 2021

jvanverth commented Jul 16, 2021

flar commented Jul 16, 2021

untp commented Jul 16, 2021

flar left a comment

Choose a reason for hiding this comment

untp commented Jul 1, 2021 •

edited

Loading

jvanverth commented Jul 14, 2021 •

edited

Loading