Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use full 4x4 matrix for mapRect operation in Transform layer #130303

Open
dnfield opened this issue Jul 11, 2023 · 7 comments
Open

Use full 4x4 matrix for mapRect operation in Transform layer #130303

dnfield opened this issue Jul 11, 2023 · 7 comments
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@dnfield
Copy link
Contributor

dnfield commented Jul 11, 2023

After fixing #82961, we drop down to a 3x3 matrix here: https://github.com/flutter/engine/blob/b9b1b63c2ba99c149f2846ac1c5430084f2ec346/flow/layers/transform_layer.cc#L49

We should do the rect mapping using the full 4x4 transform. Skia has an implementation for this in SkMatrixPriv.

@flar fyi

@dnfield dnfield added engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list labels Jul 11, 2023
@flar
Copy link
Contributor

flar commented Jul 11, 2023

Ah, I had actually traced that through and found a call to turn it into an M33, but just realized that my IDE is forwarding me to the wrong code. I'll look for the real map rect and splice it in...

@flar
Copy link
Contributor

flar commented Jul 11, 2023

Looking at the code in SkMatrixPriv (implemented in SkM44.cpp), it isn't doing anything different from SkMatrix::mapRect except that it handles cases that break the Z plane of the camera more directly whereas SkMatrix uses path clipping instead.

This change would be an optimization, not a correctness fix. We should look at it if we don't implement our own, but there doesn't appear to be a bug here for now.

@dnfield
Copy link
Contributor Author

dnfield commented Jul 12, 2023

Would you mind adding a comment to that effect to our usage of it?

@flar
Copy link
Contributor

flar commented Jul 12, 2023

Would you mind adding a comment to that effect to our usage of it?

See flutter/engine#43608

auto-submit bot pushed a commit to flutter/engine that referenced this issue Jul 12, 2023
kjlubick pushed a commit to kjlubick/engine that referenced this issue Jul 14, 2023
@chinmaygarde chinmaygarde added team-engine Owned by Engine team triaged-engine Triaged by Engine team labels Jul 17, 2023
@flutter-triage-bot flutter-triage-bot bot added the Bot is counting down the days until it unassigns the issue label Dec 11, 2023
@flutter-triage-bot
Copy link

This issue is assigned to @flar but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

@flutter-triage-bot flutter-triage-bot bot removed the triaged-engine Triaged by Engine team label Feb 19, 2024
@flutter-triage-bot
Copy link

This issue was assigned to @flar but has had no status updates in a long time. To remove any ambiguity about whether the issue is being worked on, the assignee was removed.

@flutter-triage-bot flutter-triage-bot bot removed the Bot is counting down the days until it unassigns the issue label Feb 19, 2024
@flar
Copy link
Contributor

flar commented Feb 19, 2024

This issue will likely be fixed when we move the DisplayList to use the Impeller classes for basic geometry. That work is on the radar, but not yet in progress.

@jonahwilliams jonahwilliams added the triaged-engine Triaged by Engine team label Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

No branches or pull requests

4 participants