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

Add optional offset parameter to ImageFilterLayer #36863

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

flar
Copy link
Contributor

@flar flar commented Oct 19, 2022

Enables a fix in the framework that would address flutter/flutter#111855

An offset parameter is added to the ImageFilterLayer so that the accumulated offset in _ImageFilterRenderObject.paint can paint the children with a zero offset.

This code is backwards compatible and will have no effect until the associated framework PR (flutter/flutter#113673) is merged. This PR must merge before that PR can merge.

The fix has been verified locally using both PRs in conjunction.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Oct 19, 2022
@flar flar changed the title add optional offset parameter to ImageFilterLayer Add optional offset parameter to ImageFilterLayer Oct 19, 2022
@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 1.
View them at https://flutter-engine-gold.skia.org/cl/github/36863

@flutter-dashboard
Copy link

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.

Changes reported for pull request #36863 at sha 67b74cc

@flar
Copy link
Contributor Author

flar commented Oct 19, 2022

The html golden failures are a result of the fact that the ImageFilter.matrix element puts the matrix into the transform property of the element which is also where I was trying to put the offset. The offset then canceled out the transform itself. More work is required there to get that to work.

Any suggestions @goderbauer?

@jonahwilliams
Copy link
Member

@yjbanov maybe?

@flar
Copy link
Contributor Author

flar commented Oct 20, 2022

transform-origin doesn't help because that is a translate(origin), do the transform, translate(-origin) kind of property.

It would be easy to accomplish if pushImageFilter could create multiple layers - a transform and an IF layer, or if PersistedIF could output multiple elements - one with the translate and the other with the filter. But, there appears to be a 1:1 assumption in both of these situations.

Perhaps the IFs could be responsible for incorporating the translation into their transform property - most of them would just produce a translate transform as their transform property, but MatrixIF would prefix its own transform with the translate...?

@yjbanov
Copy link
Contributor

yjbanov commented Nov 3, 2022

We have similar issues in other places, where the transform of an element intended to apply only a visual effect ends up dragging children along with it. To fix that we split that parent element into two - parent element and container element - and then the container element compensates by transforming the children back to where they need to be. Here are some examples:

Would this work for image filter as well?

@flar
Copy link
Contributor Author

flar commented Nov 3, 2022

Would this work for image filter as well?

I think that might work, but now I'm thinking that maybe we should handle this at a higher level and have the IFRenderObject interpose an actual TransformLayer into the tree (when offset is not zero) - that would avoid having to modify the engine at all...

@jonahwilliams
Copy link
Member

Fitting the Image filter into the existing render object is going to be a lot simpler if it accepts an offset. It would also allow the layer to act as a repaint boundary without users needing to protect both sides with their own repaint boundary widgets

@flar
Copy link
Contributor Author

flar commented Nov 4, 2022

Fitting the Image filter into the existing render object is going to be a lot simpler if it accepts an offset. It would also allow the layer to act as a repaint boundary without users needing to protect both sides with their own repaint boundary widgets

Is this because the ROs typically only track a single Layer as well? That would complicate things to add 2 layers as output of a single RO...

@jonahwilliams
Copy link
Member

Yes most ROs only create 0 or 1 layers. It is possible to create a RO with more that one layer if you want to manage it yourself via a LayerHandle object. Its sort of a pain, and I'd really prefer if we could make the html version of this API do the right thing.

@skia-gold
Copy link

Gold has detected about 69 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/36863

@flar
Copy link
Contributor Author

flar commented Nov 18, 2022

I can't seem to access the gold link. The page comes up but shows no results and reports a network error...

@mdebbar
Copy link
Contributor

mdebbar commented Nov 21, 2022

@flar I already explained this in Chat but I'll repeat it here for more visibility:

This is a known issue on the Gold dashboard. When there are no results to show on the page, it throws an error.

If you look at the top of the page, only the "Untriaged" checkbox is checked. That means the page is trying to display untriaged images, but it found none. So that's actually a good thing. It means all images were triaged and the PR is good from a Gold perspective.

If you want to double-check and make sure there's actually nothing wrong on your end, you can check the "Positive" checkbox. This should load the positive auto-triaged images onto the page.

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.

LGTM

@flar flar added autosubmit Merge PR when tree becomes green via auto submit App and removed will affect goldens labels Nov 21, 2022
@auto-submit auto-submit bot merged commit 21ce09a into flutter:main Nov 21, 2022
@flar
Copy link
Contributor Author

flar commented Nov 21, 2022

@flar I already explained this in Chat but I'll repeat it here for more visibility:

This is a known issue on the Gold dashboard. When there are no results to show on the page, it throws an error.

If you look at the top of the page, only the "Untriaged" checkbox is checked. That means the page is trying to display untriaged images, but it found none. So that's actually a good thing. It means all images were triaged and the PR is good from a Gold perspective.

If you want to double-check and make sure there's actually nothing wrong on your end, you can check the "Positive" checkbox. This should load the positive auto-triaged images onto the page.

Per that explanation, I removed the "will affect goldens" keyword that the bot automatically added.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 21, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 21, 2022
…115777)

* 09b82e0f0 Roll Dart SDK from c3f1b3642181 to 68291f382fb6 (7 revisions) (flutter/engine#37808)

* 49db6092a [Impeller] Present Impeller contents in a transaction when there is a platform view (flutter/engine#37809)

* 21ce09a77 Add optional offset parameter to ImageFilterLayer (flutter/engine#36863)

* cc0ad6690 Roll Skia from f7b5a76b6f35 to b5b35f8dc919 (5 revisions) (flutter/engine#37810)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
…lutter#115777)

* 09b82e0f0 Roll Dart SDK from c3f1b3642181 to 68291f382fb6 (7 revisions) (flutter/engine#37808)

* 49db6092a [Impeller] Present Impeller contents in a transaction when there is a platform view (flutter/engine#37809)

* 21ce09a77 Add optional offset parameter to ImageFilterLayer (flutter/engine#36863)

* cc0ad6690 Roll Skia from f7b5a76b6f35 to b5b35f8dc919 (5 revisions) (flutter/engine#37810)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#115777)

* 09b82e0f0 Roll Dart SDK from c3f1b3642181 to 68291f382fb6 (7 revisions) (flutter/engine#37808)

* 49db6092a [Impeller] Present Impeller contents in a transaction when there is a platform view (flutter/engine#37809)

* 21ce09a77 Add optional offset parameter to ImageFilterLayer (flutter/engine#36863)

* cc0ad6690 Roll Skia from f7b5a76b6f35 to b5b35f8dc919 (5 revisions) (flutter/engine#37810)
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 platform-web Code specifically for the web engine
Projects
None yet
5 participants