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] specialize the geometry for drawRect #36914
Merged
Merged
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
Contributor
Author
|
The difference in UI time is probably from local engine usage... |
dnfield
reviewed
Oct 21, 2022
| GeometryResult RectGeometry::GetPositionUVBuffer(const ContentContext& renderer, | ||
| const Entity& entity, | ||
| RenderPass& pass) { | ||
| // TODO(jonahwilliams): support texture coordinates in rect geometry. |
Contributor
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.
nit: file a bug and explain what this would block/where it would go wrong.
Contributor
Author
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.
Its not really a useful API yet. I was actually thinking we should create a specific drawVertices subclass that adds these methods, since none of the other geometries will use it
dnfield
reviewed
Oct 21, 2022
Contributor
dnfield
left a comment
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 have mixed feelings about this.
- This will help in cases where someone does
canvas.drawRect, but not if they docanvas.drawPathwith a path that has a bunch of (or a single) rect in it. - This might make it harder to do optimizations that would batch multiple
canvas.drawFoocalls that are compatible into something that can be handled in a single draw call. I have a dream of making the Aiks canvas able to track the last entity it used and append to it when possible instead of always creating a new one. This means we'd need geometries to know how to append where possible. I think having this kind of specialized geometry won't affect that too much because rects are simple, but it may get harder if we start specializing other geometry types that are more complicated... - How common is canvas.drawRect compared to canvas.drawPath or canvas.drawRRect?
- How does this compare to an approach like I'm describing above, where all the drawRect calls get batched into a single entity/geometry?
Contributor
Author
|
dnfield
approved these changes
Oct 21, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Oct 22, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Avoid polyline generation and tessellation when we are given a rect via canvas.drawRect. This improves the raster time of a frame made of thousands of rects to about 1/3 of the original time.
Before
After
Demo code