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

Conversation

bdero
Copy link
Member

@bdero bdero commented Jul 6, 2023

Prior to this patch, DrawPicture was untested and a no-op.

We needed a new routine to absorb Elements of a cloned pass into another pass, since appending a Picture as a subpass would be incorrect behavior for drawing a picture (since subpasses start with a blank image, which gets drawn to and then composited with the parent pass via a separate blending operation).

@bdero bdero force-pushed the bdero/drawpicture branch from a1ecace to 6d59030 Compare July 6, 2023 20:26
@chinmaygarde chinmaygarde changed the title [Impeller] Fix DrawPicture [Impeller] Fix DrawPicture. Jul 6, 2023
return true;
});
return;
pass->AddSubpassInline(std::move(pass));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pass->AddSubpassInline(std::move(pass));
GetCurrentPass()->AddSubpassInline(std::move(pass));

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, done.

Comment on lines 405 to 406
auto save_count = GetSaveCount();
Save();
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this below the early return below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Uncaught yb linter issue

return true;
}

void EntityPass::IterateAllElements(
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving to separate patch, unused

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@bdero bdero requested a review from dnfield July 6, 2023 21:28
@bdero bdero force-pushed the bdero/drawpicture branch from 6d59030 to f6c759c Compare July 6, 2023 21:29
@bdero bdero force-pushed the bdero/drawpicture branch from f6c759c to 3907d4e Compare July 6, 2023 21:32
@bdero bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 6, 2023
@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 #43446 at sha 3907d4e

@Hixie Hixie removed the impeller label Jul 6, 2023
@auto-submit auto-submit bot merged commit c29c132 into flutter:main Jul 6, 2023
@chinmaygarde
Copy link
Member

Is this part of the fixes for flutter/flutter#130078?

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 6, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 7, 2023
…130107)

flutter/engine@48bf7ac...491f317

2023-07-06 chinmaygarde@google.com Account for updated Impeller label. (flutter/engine#43450)
2023-07-06 bdero@google.com [Impeller] Fix DrawPicture. (flutter/engine#43446)

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 aaclarke@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://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
@dnfield
Copy link
Contributor

dnfield commented Jul 7, 2023

No this is me nerd-sniping @bdero into fixing this the right way haha

@dnfield
Copy link
Contributor

dnfield commented Jul 7, 2023

I filed flutter/flutter#130142 to track the work I'm referring to.

kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
Prior to this patch, `DrawPicture` was untested and a no-op.

We needed a new routine to absorb Elements of a cloned pass into another pass, since appending a `Picture` as a subpass would be incorrect behavior for drawing a picture (since subpasses start with a blank image, which gets drawn to and then composited with the parent pass via a separate blending operation).
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
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants