This repository was archived by the owner on Feb 25, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] round up subpass coverage when it is close to (and smaller) than root pass size. #49925
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
3434aff
[Impeller] dont provide coverage hint for subpass coverage.
edc86d3
more testing.
42a37e0
round up
19bbfad
Add unit test.
83d5360
Merge branch 'main' of github.com:flutter/engine into page_transition…
198b5cb
++
b721b56
dont round past bounds hint.
7e537c7
++
29ace61
++
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs more changes to make it safe.
I think the clear color optimization will always work out to be safe even thought it uses the root pass size, since it should fill up to the next largest clip anyhow.
Something that's not safe is the SaveLayer bounds hint. We simply use the bounds hint to limit the subpass, relying on the root pass to clip content outside of the bounds. So when a bounds hint is supplied we need to insert a clip to keep the behavior consistent with Skia.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case I'm concerned about, the bounds hint is actually the same size as the root pass size. So it sounds like I can handle this by only rounding up if I don't round up past the bounds hint, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a golden case that creates a SaveLayer with the bounds limit supplied and tries to draw stuff outside all the edges of the subpass.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, it's 2024 and GitHub isn't ajax yet, so I didn't see your last message.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bounds hint is in local space, but the output of
GetSubpassCoverage(used to determine the size of the texture) is in screen space. So I think you could safely check this for your case bysubpass.bounds_limit_->TransformBounds(subpass.transform_).GetSize() >= subpass_size.GetSubpassCoveragedoes this to work out the screen space bounds for clamping the texture, so to avoid float issues I'd make sure both places use the exact same rectangle (by sharing the same code path or computed output).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if you happen to know that the bounds hint will never be supplied for the zoom page transition layers, just check that it's nullopt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a bounds hint, its just the same as the root pass size. I'll need to check if its smaller than the root pass size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done...ish