-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix HiDPI platform view transforms in CanvasKit #23244
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| final Matrix4 scaleMatrix = | ||
| Matrix4.diagonal3Values(inverseScale, inverseScale, 1); | ||
| headTransform.multiply(scaleMatrix); | ||
| headTransform = scaleMatrix.multiplied(headTransform); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we initialize headTransform to Matrix4.diagonal3Values(inverseScale, inverseScale, 1) instead of applying the scaled transform at the end? This should eliminate 2 extra matrix allocations (Matrix4.identity() and scaleMatrix.multiplied).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I fully understand what you mean. Please feel free to take over this PR and commit changes to my branch. I must confess, I merely applied the fix applied in #22945 without deep understanding of how this all fits together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The headTransform was IDENTITY x T1 x T2 x ... x Tk x inverseScale. After this change it becomes inverseScale x IDENTITY x T1 x T2 x ... x Tk, which means we can remove IDENTITY and start with inverseScale. But the devil is probably in the detail. For example, if there's a clipping transform we reset it back to IDENTITY. I'm wondering if applying the scale at the end works in the presence of clips at all. If I'm reading the code correctly, it seems it doesn't?
Example: T1 > CLIP1 > T2 becomes T1 > CLIP1 > inverseScale > T2, but should probably be inverseScale > T1 > CLIP1 > T2. IIUC the inverseScale is there to undo the top-level transform that the flutter framework injects in https://github.com/flutter/flutter/blob/47bad77287effb3467f27937884036f51da6e325/packages/flutter/lib/src/rendering/view.dart#L144, so in order to undo that we should apply the inverse at the top level too.
I can take it over if you'd like, but also happy to provide guidance. I think we also need @hterkelsen's thoughts on this to make sure we're not breaking it altogether :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please take it over. Feel free to close this and create a separate PR or just push to my branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. I'll start a new PR. Thanks for discovering the bug!
|
Closing the PR to start a new one. |
Description
Completing the fix started in #22945
/cc @hterkelsen @yjbanov
Related Issues
Fixes flutter/flutter#61290
Tests
I added the following tests: TBD
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.