[rotation_distortion] Use "delayed swap" solution to reduce rotation distortion#40730
Conversation
25d04f2 to
c25de0b
Compare
dnfield
left a comment
There was a problem hiding this comment.
This LGTM but would appreciate a look from one of the other reviewers who is more famliar/active with Objective C :)
|
I think this is an improvement but there's still a pretty big jump in the middle. What does this look like on a larger screen like the iPad pro? |
Yeah the ugly part is in the middle. Though when I try it on real device, it's almost unnoticeable, since it's moving. The iPad actually looks better due to closer width : height ratio. |
|
A crazy idea: is it possible to substitute a screenshot at the middle to avoid the "jump"? |
Not sure what kind of screenshot can make it better (before or after the rotation?). We could fade in and out a white screen in the middle though, but that makes it even more noticeable. Note that screenshot takes ~0.5-0.6 seconds, which is longer than the rotation duration (~0.4 seconds). So the only choice for screenshot is before the rotation happens. |
There was a problem hiding this comment.
There's examples of this https://github.com/flutter/engine/pull/37719/files#diff-9df3e6878a0f23383e1f2de991386935e435c5b77c863de75bd67a4ced38591dR200
See also our conversation in https://github.com/flutter/engine/pull/37719/files#r1046581397
I think you should try to use -mainScreenIfViewLoaded here instead of leaving the TODO, I don't particularly want to add more deprecated API usage, and it probably should be guarded with an ifLoaded.
There was a problem hiding this comment.
Nice! I did not think about just partial mocking mainScreenIfViewLoaded
|
I think you need to rebase onto #40732. |
d8c7935 to
f08fc61
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
|
@hellohuanlin I'm not sure why there's a |
|
@jmagman I'm curious to learn how did you navigate this website? I have logged in and didn't find anything. |
I'm in no way the expert, but I checked "Digests: Positive" at the top: I couldn't find a good description of how to use the Skia Gold client UI, just https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package%3Aflutter |
f08fc61 to
ba8828f
Compare
|
auto label is removed for flutter/engine, pr: 40730, due to - The status or check suite Linux linux_android_aot_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…otation distortion (flutter/engine#40730)
flutter/engine@b2d0738...20034a8 2023-04-17 godofredoc@google.com Reland "Migrate mac_host_engine to engine v2 builds." (flutter/engine#41279) 2023-04-17 41930132+hellohuanlin@users.noreply.github.com [rotation_distortion] Use "delayed swap" solution to reduce rotation distortion (flutter/engine#40730) 2023-04-17 akash.mercer@gmail.com null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() (flutter/engine#41082) 2023-04-17 5236035+fzyzcjy@users.noreply.github.com [Impeller] Make `DoMakeRasterSnapshot` output timeline event. (flutter/engine#41197) 2023-04-17 bdero@google.com [Impeller] Remove ContentContextOptions declarations from AnonymousContents (flutter/engine#41256) 2023-04-17 skia-flutter-autoroll@skia.org Roll Dart SDK from 786a70d8ef6b to a335e6724332 (1 revision) (flutter/engine#41278) 2023-04-17 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from atix5Ek_OOxH-uoPA... to Cy5LG4U2InaFLkJGz... (flutter/engine#41275) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from atix5Ek_OOxH to Cy5LG4U2InaF If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
i have this issue in my app right now in flutter 3.10.5 can anybody tell how di i delay the swap |
|
@FaisalMohammadi it should already be enabled in the latest release. It's not perfectly smooth, but significantly better than the previous behavior. |
|
Ok perfect, you mean then in flutter 3.13, because now i have 3.10.5 and i see huge streching in my widgets. |
|
I have updated the flutter sdk to the latest 3.13.2 and i am still facing this issue. like in this video i recorded i have also tested in real device its the same rotation-distortion.mp4 |
|
@FaisalMohammadi this seems like frame drop issue. are you doing anything heavy? Can you try it on an older version before this is landed, so we can compare? |
|
@FaisalMohammadi I meant older Flutter version when this PR is not landed. |


The "size interpolation" solution didn't go well (more context here). Then a new solution came to my mind, and I call it "delayed swap":
In the originally behavior, we swap the width/height immediately before the rotation, resulting in roughly ~4x distortion in the beginning. With "delayed swap" solution, we swap the width/height right in the middle of the rotation (i.e. delay the swap for half of the transition duration).
This new "delayed swap" solution gives us the same benefit as the "snapshot" solution:
And it fixes the drawback of "snapshot" solution:
Looks pretty good on flutter gallery:
delayed.swap.mp4
List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#16322
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.