-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Move primitive type to pipeline descriptor #37315
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Could just convert it to MTLPrimitiveType here. |
||
|
||
FML_DCHECK(command.index_count * | ||
(command.index_type == IndexType::k16bit ? 2 : 4) == | ||
command.index_buffer.range.length); | ||
|
@@ -503,7 +505,7 @@ static bool Bind(PassBindingsCache& pass, | |
VALIDATION_LOG << "iOS Simulator does not support instanced rendering."; | ||
return false; | ||
#endif | ||
[encoder drawIndexedPrimitives:ToMTLPrimitiveType(command.primitive_type) | ||
[encoder drawIndexedPrimitives:ToMTLPrimitiveType(primitive_type) | ||
indexCount:command.index_count | ||
indexType:ToMTLIndexType(command.index_type) | ||
indexBuffer:mtl_index_buffer | ||
|
@@ -512,7 +514,7 @@ static bool Bind(PassBindingsCache& pass, | |
baseVertex:command.base_vertex | ||
baseInstance:0u]; | ||
} else { | ||
[encoder drawIndexedPrimitives:ToMTLPrimitiveType(command.primitive_type) | ||
[encoder drawIndexedPrimitives:ToMTLPrimitiveType(primitive_type) | ||
indexCount:command.index_count | ||
indexType:ToMTLIndexType(command.index_type) | ||
indexBuffer:mtl_index_buffer | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -311,4 +311,19 @@ constexpr vk::IndexType ToVKIndexType(IndexType index_type) { | |
} | ||
} | ||
|
||
constexpr vk::PrimitiveTopology ToVKPrimitiveTopology(PrimitiveType primitive) { | ||
switch (primitive) { | ||
case PrimitiveType::kTriangle: | ||
return vk::PrimitiveTopology::eTriangleList; | ||
case PrimitiveType::kTriangleStrip: | ||
return vk::PrimitiveTopology::eTriangleStrip; | ||
case PrimitiveType::kLine: | ||
return vk::PrimitiveTopology::eLineList; | ||
case PrimitiveType::kLineStrip: | ||
return vk::PrimitiveTopology::eLineStrip; | ||
case PrimitiveType::kPoint: | ||
return vk::PrimitiveTopology::ePointList; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call! done: #37324 |
||
|
||
} // namespace impeller |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,10 @@ class PipelineDescriptor final : public Comparable<PipelineDescriptor> { | |
|
||
WindingOrder GetWindingOrder() const; | ||
|
||
void SetPrimitiveType(PrimitiveType type); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The setters return a reference to the descriptor so we can chain them. Though I don't see that pattern followed a lot. |
||
|
||
PrimitiveType GetPrimitiveType() const; | ||
|
||
private: | ||
std::string label_; | ||
SampleCount sample_count_ = SampleCount::kCount1; | ||
|
@@ -131,6 +135,7 @@ class PipelineDescriptor final : public Comparable<PipelineDescriptor> { | |
front_stencil_attachment_descriptor_; | ||
std::optional<StencilAttachmentDescriptor> | ||
back_stencil_attachment_descriptor_; | ||
PrimitiveType primitive_type_ = PrimitiveType::kTriangle; | ||
}; | ||
|
||
using PipelineMap = std::unordered_map< | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.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.
Got it.