Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Partial ShaderMask for Web DOM#21756

Closed
anirudhb wants to merge 4 commits intoflutter:masterfrom
anirudhb:shadermask_web
Closed

Partial ShaderMask for Web DOM#21756
anirudhb wants to merge 4 commits intoflutter:masterfrom
anirudhb:shadermask_web

Conversation

@anirudhb
Copy link
Contributor

@anirudhb anirudhb commented Oct 10, 2020 •
edited
Loading

Description

Adds a partial ShaderMask implementation on Flutter Web with the DOM backend.

What's implemented:

  • ui.Gradient shaders
    • Linear gradients
    • Radial gradients
    • Sweep gradients
    • Conical gradients
  • ui.ImageShader shaders

All blend modes should work.

Related Issues

flutter/flutter#44152

Tests

I added the following tests:

  • None, this is not breaking behavior.

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.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 10, 2020

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.

@google-cla google-cla bot added the cla: yes label Oct 10, 2020
@anirudhb
Copy link
Contributor Author

anirudhb commented Oct 11, 2020

I should be able to add support for the rest of the color modes using SVG filters as well, so I'll work on that too. Regarding the rest of the shaders, image shaders should be possible with background-repeat but sweep gradients might be hard to implement (not sure how to implement those.)

@anirudhb anirudhb marked this pull request as draft October 11, 2020 04:57
@anirudhb anirudhb marked this pull request as ready for review October 11, 2020 23:24
@zanderso zanderso requested a review from yjbanov October 13, 2020 16:57
@yjbanov yjbanov requested a review from ferhatb October 14, 2020 21:26
yjbanov
yjbanov reviewed Oct 14, 2020
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Thanks for contributing the web implementation! I have some preliminary comments. @ferhatb will probably have more. We can't land it without tests. For this functionality specifically, we'll need screenshot tests. I think we can cover all of the functionality with a single screenshot (example).

lib/web_ui/lib/src/engine/html/shader_mask.dart

/// The dedicated child container element that's separate from the
/// [rootElement] is used to compensate for the coordinate system shift
/// introduced by the [rootElement] translation.
Copy link
Contributor

@yjbanov yjbanov Oct 14, 2020

Choose a reason for hiding this comment

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

Unless I'm missing something, this layer does not translate its root element, so I think you don't need a separate child container.

lib/web_ui/lib/src/engine/html/shader_mask.dart Outdated
_maskFilterIdCounter += 1;
return '<svg width="0" height="0" xmlns:xlink="http://www.w3.org/1999/xlink">'
'<filter id="_fmf$_maskFilterIdCounter" '
'filterUnits="objectBoundingBox">'
Copy link
Contributor

@yjbanov yjbanov Oct 14, 2020

Choose a reason for hiding this comment

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

objectBoundingBox is the default. We don't need to specify it. Will save a bit of CPU/memory/network.

(applies here and in other filters)

Copy link
Contributor

@ferhatb ferhatb Oct 15, 2020

Choose a reason for hiding this comment

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

Please test in Firefox. There are some cross browser bugs around filterUnits

ferhatb
ferhatb suggested changes Oct 15, 2020
View reviewed changes
lib/web_ui/lib/src/engine/html/shader_mask.dart
@@ -0,0 +1,376 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
Copy link
Contributor

@ferhatb ferhatb Oct 15, 2020

Choose a reason for hiding this comment

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

Needs golden test to verify it renders on both Chrome and WebKit.

@chinmaygarde chinmaygarde added the platform-web Code specifically for the web engine label Oct 22, 2020
@mzdm mzdm mentioned this pull request Oct 30, 2020
Closed
@ferhatb
Copy link
Contributor

ferhatb commented Oct 30, 2020

Hi @anirudhb are you still working on this? Anyway we can help?

@anirudhb
Copy link
Contributor Author

anirudhb commented Oct 30, 2020 •
edited
Loading

Just updated the PR. I'm currently writing golden tests, but not sure what the process is to generate/submit golden files (so that the test does not fail.)

Edit: It seems my most recent commit somehow messed up Firefox's rendering. Looking into and fixing. Although afaik the lack of filterUnits should not affect rendering.

@ferhatb
Copy link
Contributor

ferhatb commented Nov 5, 2020

Just updated the PR. I'm currently writing golden tests, but not sure what the process is to generate/submit golden files (so that the test does not fail.)

Edit: It seems my most recent commit somehow messed up Firefox's rendering. Looking into and fixing. Although afaik the lack of filterUnits should not affect rendering.

Once you have the test

Just updated the PR. I'm currently writing golden tests, but not sure what the process is to generate/submit golden files (so that the test does not fail.)

Edit: It seems my most recent commit somehow messed up Firefox's rendering. Looking into and fixing. Although afaik the lack of filterUnits should not affect rendering.

Great! Once you have firefox problem solved and add a test we can gen goldens.

  • Please look at gradient_golden_test.dart and create a shadermask_golden_test.dart that is similar.
  • engine/src/flutter/lib/web_ui>. dev/felt test test/engine/golden_tests/shadermask_golden_test.dart
  • then if you have a mac , repeat the above with: --browser=ios-safari
  • Under the .dart_tool/golden/web directory you should now see the golden files
  • Add those files to flutter/goldens repo in a PR and send out for review.
  • Finally take the commit hash of that golden repo commit and update golden_locks.yaml with new commit hash in this PR.
  • If you don't have a mac to generate safari goldens, no worries, add the test, let us know and we'll generate goldens PR for you.

@yjbanov
Copy link
Contributor

yjbanov commented Nov 6, 2020

  • engine/src/flutter/lib/web_ui>. dev/felt test test/engine/golden_tests/shadermask_golden_test.dart
  • then if you have a mac , repeat the above with: --browser=ios-safari
  • Under the .dart_tool/golden/web directory you should now see the golden files

In order for felt to generate the new goldens you also need to pass --update-screenshot-goldens (or pass write: true to matchGoldenFile). If you don't have access to a Mac or Linux, you can just write the test, mark it as skip: true, and post the screenshots generated on whatever computer you have here for manual review. We can enable them on the CI ourselves in a separate PR.

@yjbanov
Copy link
Contributor

yjbanov commented Jan 6, 2021

@anirudhb do you plan to continue working on this PR?

@anirudhb
Copy link
Contributor Author

anirudhb commented Jan 7, 2021

I'll take a look at it again this weekend.

@anirudhb anirudhb marked this pull request as draft January 20, 2021 18:51
@ferhatb
Copy link
Contributor

ferhatb commented Jan 28, 2021

@anirudhb , please take a look at #24029 it contains a refactor of gradient code and your changes to support full set of gradients. Let me know if you have time to write the screenshot tests and add changes. If not we can add tests and land it.

@anirudhb
Copy link
Contributor Author

anirudhb commented Jan 30, 2021 •
edited
Loading

I don't think I'll have time to fix up this PR, but feel free to make it usable for merge! (You can edit this branch if necessary)

@anirudhb anirudhb marked this pull request as ready for review January 30, 2021 03:43
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jan 30, 2021

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.

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.

@thecalamiity
Copy link

thecalamiity commented May 4, 2021

@anirudhb Looks the issue was closed. Is this PR still valid?

@ferhatb
Copy link
Contributor

ferhatb commented May 5, 2021

Closing this PR shadermasks have landed in a separate PR.

@ferhatb ferhatb closed this May 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@yjbanov yjbanov yjbanov left review comments

@ferhatb ferhatb ferhatb requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes platform-web Code specifically for the web engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@anirudhb @ferhatb @yjbanov @thecalamiity @chinmaygarde @googlebot