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

Conversation

jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Aug 1, 2024

@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 #54293 at sha b091bfb

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54293 at sha 8019b83

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54293 at sha 030afbe

@jonahwilliams
Copy link
Contributor Author

I'm looking at an issue with SiblingSaveLayerBoundsAreRespected . It looks like the saveLayer bounds are always treated as tight with aiks, but are advisory in display list - leading to the saveLayer expanding instead of clipping. Since this is the current behavior this PR isn't breaking anything there, but perhaps I should disable/remove the test as it doesn't actually demonstrate what the expected rendering is?

@jonahwilliams jonahwilliams requested review from bdero and flar August 6, 2024 19:57
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54293 at sha 397ca9b

@flar
Copy link
Contributor

flar commented Aug 6, 2024

I'm looking at an issue with SiblingSaveLayerBoundsAreRespected . It looks like the saveLayer bounds are always treated as tight with aiks, but are advisory in display list - leading to the saveLayer expanding instead of clipping. Since this is the current behavior this PR isn't breaking anything there, but perhaps I should disable/remove the test as it doesn't actually demonstrate what the expected rendering is?

The intent is to include the tight bounds with the user bounds as a clip:

if (layer_op->options.bounds_from_caller()) {

TEST_P(AiksTest, SiblingSaveLayerBoundsAreRespected) {
DisplayListBuilder builder;
DlPaint paint;
SkRect rect = SkRect::MakeXYWH(0, 0, 1000, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember where it caused a problem but in some tool I've used to look at goldens a transparent background with black stuff drawn on it just looks like a giant field of black, so I usually paint a background color in my tests...

@jonahwilliams
Copy link
Contributor Author

Ahh found the bug!

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54293 at sha 776dc12

@jonahwilliams
Copy link
Contributor Author

The rrect changes are a bit odd too, but I think the Skia RRect constructor must be different from the impeller version. Otherwise ready for review, I've left the untriaged goldens.

@jonahwilliams jonahwilliams requested a review from flar August 7, 2024 00:14
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54293 at sha 4bb55da

@jonahwilliams
Copy link
Contributor Author

Friendly ping @flar :)

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2024
@auto-submit auto-submit bot merged commit 0a8de3d into flutter:main Aug 8, 2024
31 checks passed
@jonahwilliams jonahwilliams deleted the convert_more_tests1232 branch August 8, 2024 20:29
@flar
Copy link
Contributor

flar commented Aug 8, 2024

Sorry, I was still looking at why the goldens changed. I don't trust that.

@flar
Copy link
Contributor

flar commented Aug 8, 2024

Is there any way to load up the changed goldens again?

@chinmaygarde
Copy link
Member

Hmm, not that I know of. Perhaps you can create a revert PR (but not submit it) to show the reverse change to introspect.

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 8, 2024
…153132)

flutter/engine@ef820aa...0520889

2024-08-08 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Detect animated WebP images (flutter/engine#54418)
2024-08-08 skia-flutter-autoroll@skia.org Roll Dart SDK from 067c7cfcbc8c to ff0404c72fc5 (1 revision) (flutter/engine#54455)
2024-08-08 jonahwilliams@google.com [Impeller] move aiks text tests to DL. (flutter/engine#54293)
2024-08-08 skia-flutter-autoroll@skia.org Roll Skia from 0c6dd1e6ff8e to 4cff580721cf (20 revisions) (flutter/engine#54454)
2024-08-08 robert.ancell@canonical.com Add a precision to the fragment shader (flutter/engine#54109)

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 jsimmons@google.com,rmistry@google.com,zra@google.com 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
DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024
…lutter#153132)

flutter/engine@ef820aa...0520889

2024-08-08 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Detect animated WebP images (flutter/engine#54418)
2024-08-08 skia-flutter-autoroll@skia.org Roll Dart SDK from 067c7cfcbc8c to ff0404c72fc5 (1 revision) (flutter/engine#54455)
2024-08-08 jonahwilliams@google.com [Impeller] move aiks text tests to DL. (flutter/engine#54293)
2024-08-08 skia-flutter-autoroll@skia.org Roll Skia from 0c6dd1e6ff8e to 4cff580721cf (20 revisions) (flutter/engine#54454)
2024-08-08 robert.ancell@canonical.com Add a precision to the fragment shader (flutter/engine#54109)

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 jsimmons@google.com,rmistry@google.com,zra@google.com 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
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…lutter#153132)

flutter/engine@ef820aa...0520889

2024-08-08 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Detect animated WebP images (flutter/engine#54418)
2024-08-08 skia-flutter-autoroll@skia.org Roll Dart SDK from 067c7cfcbc8c to ff0404c72fc5 (1 revision) (flutter/engine#54455)
2024-08-08 jonahwilliams@google.com [Impeller] move aiks text tests to DL. (flutter/engine#54293)
2024-08-08 skia-flutter-autoroll@skia.org Roll Skia from 0c6dd1e6ff8e to 4cff580721cf (20 revisions) (flutter/engine#54454)
2024-08-08 robert.ancell@canonical.com Add a precision to the fragment shader (flutter/engine#54109)

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 jsimmons@google.com,rmistry@google.com,zra@google.com 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants