This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[Impeller] Remove polymorphism from clip recording. #55784
Closed
Conversation
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
jonahwilliams
commented
Oct 9, 2024
| } | ||
|
|
||
| void Canvas::ClipGeometry(std::unique_ptr<Geometry> geometry, | ||
| void Canvas::ClipGeometry(const Geometry& geometry, |
Contributor
Author
There was a problem hiding this comment.
I combined AddClipEntityToCurrentPass into this method, since its all only called in one place.
jonahwilliams
commented
Oct 9, 2024
|
|
||
| void AddClipEntityToCurrentPass(Entity& entity); | ||
|
|
||
| void RestoreClip(); |
Contributor
Author
There was a problem hiding this comment.
This one was unused.
jonahwilliams
commented
Oct 9, 2024
| /// depth should be the depth of the entity that is currently being drawn, and | ||
| /// restore_coverage should be its coverage. If restore_coverage is | ||
| /// std::nullopt, the render pass coverage is used instead. | ||
| bool RenderClipRestore(const ContentContext& renderer, |
Contributor
Author
There was a problem hiding this comment.
We don't ever need to create a restore entity anymore.
Contributor
Author
There was a problem hiding this comment.
Either its a save restore in which case we do nothing (since we keep incrementing depth) or a saveLayer in which case we don't need to restore since we're ending the pass.
jonahwilliams
commented
Oct 9, 2024
| return false; | ||
| } | ||
|
|
||
| Contents::ClipCoverage Contents::GetClipCoverage( |
jonahwilliams
commented
Oct 9, 2024
| using ColorFilterProc = std::function<Color(Color)>; | ||
|
|
||
| struct ClipCoverage { | ||
| enum class Type { kNoChange, kAppend, kRestore }; |
Contributor
Author
There was a problem hiding this comment.
This enum was no longer used, moved struct to clip contents
jonahwilliams
commented
Oct 9, 2024
| /// During rendering, coverage coordinates count pixels from the top | ||
| /// left corner of the framebuffer. | ||
| /// | ||
| virtual ClipCoverage GetClipCoverage( |
Contributor
Author
There was a problem hiding this comment.
Unused, not virtual on ClipContents anymore.
jonahwilliams
commented
Oct 9, 2024
impeller/entity/contents/filters/gaussian_blur_filter_contents.cc
Outdated
Show resolved
Hide resolved
jonahwilliams
commented
Oct 9, 2024
| return contents_->GetCoverage(*this); | ||
| } | ||
|
|
||
| Contents::ClipCoverage Entity::GetClipCoverage( |
jonahwilliams
commented
Oct 9, 2024
| Entity& entity, | ||
| size_t clip_height_floor, | ||
| Point global_pass_position) { | ||
| EntityPassClipStack::ClipStateResult EntityPassClipStack::RecordRestore( |
Contributor
Author
There was a problem hiding this comment.
Rather that work generic over entities, there is now a RecordClip and RecordRestore operation.
added 7 commits
October 9, 2024 14:13
auto-submit bot
pushed a commit
that referenced
this pull request
Oct 22, 2024
Now that we don't have the entity pass structure, clips can be treated as a kind of structured data and not a generic entity. Doing so lets us simplify some of the clip management and remove some usage of raw pointers. This change also removes the ClipRestoreContents, which are only ever created and then immediately rendered - we can instead replace this with a function. The ClipCoverageStack now has explicit Clip/Restore methods that internally apply the same logic, but without involving the entity object. Redo of #55784
nick9822
pushed a commit
to nick9822/flutter
that referenced
this pull request
Dec 18, 2024
Now that we don't have the entity pass structure, clips can be treated as a kind of structured data and not a generic entity. Doing so lets us simplify some of the clip management and remove some usage of raw pointers. This change also removes the ClipRestoreContents, which are only ever created and then immediately rendered - we can instead replace this with a function. The ClipCoverageStack now has explicit Clip/Restore methods that internally apply the same logic, but without involving the entity object. Redo of flutter/engine#55784
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Now that we don't have the entity pass structure, clips can be treated as a kind of structured data and not a generic entity. Doing so lets us simplify some of the clip management and remove some usage of raw pointers. This change also removes the ClipRestoreContents, which are only ever created and then immediately rendered - we can instead replace this with a function.
The ClipCoverageStack now has explicit Clip/Restore methods that internally apply the same logic, but without involving the entity object.