[Impeller] Move primitive type to pipeline descriptor #37315
Merged
Conversation
This file contains 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 is the correct abstraction for Vulkan as we need this during primitive assembly. We also know this ahead of time so its useful to just wire it in. Fixes flutter/flutter#106379
dnfield
approved these changes
Nov 4, 2022
iskakaushik
added
the
autosubmit
chinmaygarde
reviewed
Nov 4, 2022
| @@ -115,6 +115,10 @@ class PipelineDescriptor final : public Comparable<PipelineDescriptor> { | |||
|
|
|||
| WindingOrder GetWindingOrder() const; | |||
|
|
|||
| void SetPrimitiveType(PrimitiveType type); | |||
There was a problem hiding this comment.
The setters return a reference to the descriptor so we can chain them. Though I don't see that pattern followed a lot.
| case PrimitiveType::kPoint: | ||
| return vk::PrimitiveTopology::ePointList; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we add FML_UNREACHABLE just to better guarantee exhaustive switches? The functions earlier in this TU do this as do all the other backends. For consistency.
There was a problem hiding this comment.
good call! done: #37324
| @@ -494,6 +494,8 @@ static bool Bind(PassBindingsCache& pass, | |||
| return false; | |||
| } | |||
|
|
|||
| const PrimitiveType primitive_type = pipeline_desc.GetPrimitiveType(); | |||
There was a problem hiding this comment.
Nit: Could just convert it to MTLPrimitiveType here.
| @@ -86,7 +86,6 @@ bool ClipContents::Render(const ContentContext& renderer, | |||
| { | |||
| cmd.label = "Difference Clip (Increment)"; | |||
|
|
|||
| cmd.primitive_type = PrimitiveType::kTriangleStrip; | |||
There was a problem hiding this comment.
How did we manage to get rid of the strip here without reworking the vertex buffer? Was it just incorrect?
There was a problem hiding this comment.
@chinmaygarde it was always overwritten, see line: 123. I think there was a refactor at some point to use the geometry_result but this line never got removed.
iskakaushik
changed the title
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 4, 2022
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Nov 4, 2022
…114710) * 9584880f4 Use iOS 16 APIs to rotate orientation (flutter/engine#37302) * 2ac9c73f6 Move primitive type to pipeline descriptor (flutter/engine#37315) * 6950689ed Roll Skia from 7a45d3123f8b to dc49f35e1ac6 (6 revisions) (flutter/engine#37319)
iskakaushik
added a commit
to iskakaushik/engine
that referenced
this pull request
Nov 9, 2022
This was wrongly removed in flutter#37315 where I assumed that this was overwritten by geometry_result a few lines below, but there is indeed a pipeline created earlier in line:100. Fixes: flutter/flutter#114870
schwa423
pushed a commit
to schwa423/engine
that referenced
this pull request
Nov 16, 2022
This is the correct abstraction for Vulkan as we need this during primitive assembly. We also know this ahead of time so its useful to just wire it in. Fixes flutter/flutter#106379
shogohida
pushed a commit
to shogohida/flutter
that referenced
this pull request
Dec 7, 2022
…lutter#114710) * 9584880f4 Use iOS 16 APIs to rotate orientation (flutter/engine#37302) * 2ac9c73f6 Move primitive type to pipeline descriptor (flutter/engine#37315) * 6950689ed Roll Skia from 7a45d3123f8b to dc49f35e1ac6 (6 revisions) (flutter/engine#37319)
gspencergoog
pushed a commit
to gspencergoog/flutter
that referenced
this pull request
Jan 19, 2023
…lutter#114710) * 9584880f4 Use iOS 16 APIs to rotate orientation (flutter/engine#37302) * 2ac9c73f6 Move primitive type to pipeline descriptor (flutter/engine#37315) * 6950689ed Roll Skia from 7a45d3123f8b to dc49f35e1ac6 (6 revisions) (flutter/engine#37319)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is the correct abstraction for Vulkan as we need this during primitive assembly. We also know this ahead of time so its useful to just wire it in.
Fixes flutter/flutter#106379