[Impeller] moved blur to unrotated local space, started respecting respect_ctm flag
#53377
Conversation
…b entity scale when rendering the 2-pass blur input.
|
|
||
| namespace { | ||
| Vector2 ExtractScale(const Matrix& matrix) { | ||
| Vector2 entity_scale_x = matrix * Vector2(1.0, 0.0); |
There was a problem hiding this comment.
Matrix::Decompose here?
There was a problem hiding this comment.
I looked at that and it was way more calculations for things we just throw away. It's doing things like calculating the determinant. It returns an optional result probably for this reason. This one is guaranteed to have a result too.
There was a problem hiding this comment.
These matrix mults are just recomputing the basis vectors, which could just be pulled out of the matrix instead: return Vector2(matrix.GetBasisX().Length(), matrix.GetBasisY().Length())
Matrix::GetScale does the same thing, but includes the Z basis and returns a Vector3. Adding a Matrix::GetScaleXY seems like a good idea since we're doing this in a few places now.
|
Notes from the goldens:
|
|
Here's what the tests look like in playgrounds: |
|
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. |
|
Disprespecting the CTM makes things draw differently depending on the dpi of the device. I'll file a separate issue for that: I had a discussion with brandon about this a couple days ago, but here is a clear case where we need to know the dpi to render the right thing. |
|
All the goldens look good. I just approved them. I'll tag brandon on this PR in case he wants to take a look before I merge. Everything is looking good from my end though. This may change benchmarks again, but the will be in line with what we had before the "source space" pr. |
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
Belated LGTM. :)
@gaaclarke confirmed that the behavior of this patch matches Skia's in flutter/flutter#150272.
Neither Impeller nor Skia are aware of the device pixel ratio, and the remaining problem with text shadows changing with different DPRs is due to DisplayListParagraphPainter::drawTextShadow using respect_ctm=false without accounting for DPR.
…pecting `respect_ctm` flag (flutter/engine#53377)
…pecting `respect_ctm` flag (flutter/engine#53377)
…150290) flutter/engine@8167dff...9779c27 2024-06-15 [email protected] Manual roll of Dart SDK from e90b0a53e058 to dca20ab646c5 (flutter/engine#53410) 2024-06-15 [email protected] Update "Vulnerability scanning" to add the actions: read permission. (flutter/engine#53409) 2024-06-14 [email protected] Roll Skia from 136c7cc18d1e to 6f6b45e1faea (3 revisions) (flutter/engine#53408) 2024-06-14 [email protected] [Impeller] moved blur to unrotated local space, started respecting `respect_ctm` flag (flutter/engine#53377) 2024-06-14 [email protected] Roll Skia from 5560041fcfc0 to 136c7cc18d1e (4 revisions) (flutter/engine#53406) 2024-06-14 [email protected] Roll web_ui dependencies to enable the next roll of the Dart SDK (flutter/engine#53399) 2024-06-14 [email protected] Hack to prevent Safari from being backgrounded during unit tests. (flutter/engine#53402) 2024-06-14 [email protected] Manual roll Dart SDK from cabe65966815 to e90b0a53e058 (1 revision) (flutter/engine#53404) 2024-06-14 [email protected] Roll Skia from de1a50046289 to 5560041fcfc0 (1 revision) (flutter/engine#53393) 2024-06-14 [email protected] Manual roll Dart SDK from 115a9a2b26cd to cabe65966815 (1 revision) (flutter/engine#53390) 2024-06-14 [email protected] Roll Skia from 3bebb0602780 to de1a50046289 (3 revisions) (flutter/engine#53388) 2024-06-14 [email protected] Roll Skia from d248a9f99ff5 to 3bebb0602780 (1 revision) (flutter/engine#53387) 2024-06-14 [email protected] Roll Fuchsia Linux SDK from pGxbL7JoNb3yAYFw4... to BkYg1UB1jdbAZv3gA... (flutter/engine#53386) 2024-06-14 [email protected] [Impeller] restore accidentally deleted Cap/Join benchmarks (flutter/engine#53385) 2024-06-13 [email protected] Add a `FlutterEngineRule` (JUnit `TestRule`) and use it in `FlutterRendererTest` (flutter/engine#53361) 2024-06-13 [email protected] Roll Skia from b21429b0ea78 to d248a9f99ff5 (2 revisions) (flutter/engine#53383) 2024-06-13 [email protected] Roll Skia from 40bdee9eedd6 to b21429b0ea78 (3 revisions) (flutter/engine#53382) 2024-06-13 [email protected] [DisplayList] cull clip operations that can be trivially and safely ignored (flutter/engine#53367) 2024-06-13 [email protected] Roll Skia from 0f47a9333edb to 40bdee9eedd6 (2 revisions) (flutter/engine#53381) 2024-06-13 [email protected] Roll Dart SDK from aa2265e5a192 to 115a9a2b26cd (5 revisions) (flutter/engine#53380) 2024-06-13 [email protected] Roll Skia from bf5a0e0dbf41 to 0f47a9333edb (2 revisions) (flutter/engine#53378) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from pGxbL7JoNb3y to BkYg1UB1jdbA 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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
Resolves flutter/flutter#142014
Resolves flutter/flutter#150114
A previous patch made the blur operate in source space. This doesn't work for text since it jumbles up the characters. We probably want the text crisp when scaled anyways so it would make sense not to render those is source space. Instead I moved the blur to "scaled source space" or "unrotated local space", if you will.
This is more closely what we had previously but it handles rotation correctly.
Also, brandon fixed a problem where we weren't minding a flag on how the ctm should be used when rendering mask blurs. That was important to this fix too.
new golden results before patch
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.