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

[Impeller] Assign subpass depth on restore rather than creation. #50626

Merged
merged 6 commits into from
Feb 17, 2024

Conversation

bdero
Copy link
Member

@bdero bdero commented Feb 13, 2024

The subpass depth is used for drawing the texture to the parent pass. So it needs a depth > all of the clips contained within the subpass.

Also correct the way we're assigning the depth value in shaders.

@bdero bdero requested a review from jonahwilliams February 13, 2024 23:30
@bdero bdero self-assigned this Feb 13, 2024
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

@bdero bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 14, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 14, 2024
Copy link
Contributor

auto-submit bot commented Feb 14, 2024

auto label is removed for flutter/engine/50626, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@bdero bdero force-pushed the bdero/fix-savelayer-depth branch 2 times, most recently from 92afb30 to 7ba1a30 Compare February 14, 2024 10:57
@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 #50626 at sha 7ba1a30

@flutter-dashboard
Copy link

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

Changes reported for pull request #50626 at sha af0bff2

The subpass depth is used for drawing the texture to the parent pass.
So it needs a depth > all of the clips contained within the subpass.

Also correct the way we're assigning the depth value in shaders.
@bdero bdero force-pushed the bdero/fix-savelayer-depth branch from 393ade6 to 8860b27 Compare February 16, 2024 03:53
@bdero
Copy link
Member Author

bdero commented Feb 16, 2024

This is a bit wacky, but the pipelines seem to be getting an alternative depth write compare options on all the backends on just CI somehow. It's happening for all 3 backends so the issue is going to be a high level thing. I'll probably figure it out after a couple more print debugging iterations.

Might scatter around a few more sensible debug time checks while I'm at it as well since there's some potential for subtle mistakes that are hard to debug.

@bdero bdero marked this pull request as draft February 16, 2024 23:53
@bdero
Copy link
Member Author

bdero commented Feb 16, 2024

(errors on CI right now are just due to my print debugging)

@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@bdero bdero marked this pull request as ready for review February 17, 2024 02:38
@bdero
Copy link
Member Author

bdero commented Feb 17, 2024

I had missed a couple of spots when initially distributing the depth to the shaders, and the previous behavior of passing the depth through the ortho projection was covering it up.

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.

Still LGTM

@bdero bdero added autosubmit Merge PR when tree becomes green via auto submit App and removed will affect goldens labels Feb 17, 2024
@auto-submit auto-submit bot merged commit 0a7fcfc into flutter:main Feb 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 17, 2024
…143652)

flutter/engine@e51d4f1...c807aea

2024-02-17 [email protected] Roll Skia from bb61c2b4614e to 0d2dbf53aef6 (3 revisions) (flutter/engine#50744)
2024-02-17 [email protected] [Impeller] add additional setup method that caches more pipelines, warms internal shader code (flutter/engine#50521)
2024-02-17 [email protected] [Impeller] Assign subpass depth on restore rather than creation. (flutter/engine#50626)
2024-02-17 [email protected] Roll Dart SDK from fa66195a3814 to 6d659f880394 (1 revision) (flutter/engine#50739)

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],[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://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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants