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

[Impeller] Gaussian blur: Remove the current blur style implementation. #45520

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

bdero
Copy link
Member

@bdero bdero commented Sep 6, 2023

Resolves flutter/flutter#122658.
Resolves remaining blur issues visible in: flutter/flutter#132839

The current blur style implementation hasn't been working since we added the downscaling optimization, and it will continue not working after we switch to flutter/flutter#131580.

In it's current state, it at best causes awful looking undesirable results that users are forced to work around anyhow. We should just remove it in the meantime.

Before:
image

After:
image

@bdero bdero self-assigned this Sep 6, 2023
@flutter-dashboard
Copy link

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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RIP in Pieces, LGTM

I'm currently working on a patch that will jam the two filter contents together, this change will significantly simplify that.

Do we have an issue tracking relanding/fixing the blur styles?

@bdero bdero force-pushed the bdero/remove-blur-style-impl branch from 8917927 to ac2117f Compare September 6, 2023 23:24
@bdero
Copy link
Member Author

bdero commented Sep 6, 2023

SGTM. I went ahead and split the reimplementation for flutter/flutter#122658 out into a separate issue here: flutter/flutter#134178

@bdero
Copy link
Member Author

bdero commented Sep 6, 2023

I need to remember to delete the gen directory when testing shader removals locally.

@bdero bdero force-pushed the bdero/remove-blur-style-impl branch from ac2117f to 6d6d67c Compare September 6, 2023 23:43
@bdero bdero force-pushed the bdero/remove-blur-style-impl branch from 6d6d67c to 9bb5e23 Compare September 7, 2023 00:10
@bdero bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2023
@auto-submit auto-submit bot merged commit 02ea040 into flutter:main Sep 7, 2023
@zanderso
Copy link
Member

zanderso commented Sep 7, 2023

Looks like this patch broke impeller-cmake-example.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2023
bdero added a commit that referenced this pull request Sep 7, 2023
…orms. (#45524)

Follow-up for #45520.

Missed a few things... These were getting defaulted thanks to impeller's
header gen but don't have any purpose now that we've removed the old
blur style impl.
@bdero
Copy link
Member Author

bdero commented Sep 7, 2023

Yup, already on it. ;)

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 7, 2023
…sions) (#134188)

Manual roll requested by [email protected]

flutter/engine@2c69d05...75437a3

2023-09-07 [email protected] Revert "Enforce the rule of calling `FlutterView.Render`" (flutter/engine#45525)
2023-09-07 [email protected] Roll Skia from 2b76d1113497 to 9e86d3f6239a (1 revision) (flutter/engine#45521)
2023-09-07 [email protected] [Impeller] Gaussian blur: Remove the current blur style implementation. (flutter/engine#45520)
2023-09-06 [email protected] Roll Skia from 9c9757c5d17d to 2b76d1113497 (2 revisions) (flutter/engine#45518)
2023-09-06 [email protected] Roll Skia from 0039caadd635 to 9c9757c5d17d (1 revision) (flutter/engine#45516)
2023-09-06 [email protected] Roll ANGLE from 00daa451320c to 60b56591dee5 (1 revision) (flutter/engine#45517)
2023-09-06 [email protected] Roll Fuchsia Linux SDK from 8dgICHnG28wNHzoz3... to SCoDb2m_zQDLrMhwT... (flutter/engine#45514)
2023-09-06 [email protected] Roll Skia from a274609c442c to 0039caadd635 (2 revisions) (flutter/engine#45513)
2023-09-06 [email protected] Add macOS support for plugin value publishing (flutter/engine#45502)
2023-09-06 [email protected] Roll Skia from e5ed4ffaaaa4 to a274609c442c (2 revisions) (flutter/engine#45510)
2023-09-06 [email protected] Roll Fuchsia with license script fix (flutter/engine#45498)
2023-09-06 [email protected] Enforce the rule of calling `FlutterView.Render` (flutter/engine#45300)
2023-09-06 [email protected] Roll ANGLE from 7b0bb0f6e785 to 00daa451320c (1 revision) (flutter/engine#45507)
2023-09-06 [email protected] Roll Skia from 59e54ccf25a4 to e5ed4ffaaaa4 (4 revisions) (flutter/engine#45506)
2023-09-06 [email protected] Roll Fuchsia Mac SDK from dFe-t1SosqZwU5lZR... to hHwU6r12A0sy5Bq-0... (flutter/engine#45505)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from z9uQ0mXwjKFQ to SCoDb2m_zQDL
  fuchsia/sdk/core/mac-amd64 from dFe-t1SosqZw to hHwU6r12A0sy

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] 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Impeller] Gaussian blur: Remove the blur style logic from the gaussian blur shader.
3 participants