[Impeller] Try replacing `*desc` with value/is_valid checks. (#48194)
Some discussion here:
https://discord.com/channels/608014603317936148/1175215129135153202.
In short, `*<std::optional>` is [undefined
behavior](https://en.cppreference.com/w/cpp/utility/optional/operator*).
After talking to @bdero we are considering that this should never
happen.
I thought of a few different approaches here, none of them are great.
However given that this class is used quite minimally, this seems to
jive with similar patterns we're using in scene/scene_context.
---
Feel free to push back or suggest alternatives, mostly proposing this to
get brain juices flowing.
diff --git a/impeller/scene/scene_context.cc b/impeller/scene/scene_context.cc
index 66c8b07..baece93 100644
--- a/impeller/scene/scene_context.cc
+++ b/impeller/scene/scene_context.cc
@@ -40,11 +40,24 @@
return;
}
- pipelines_[{PipelineKey{GeometryType::kUnskinned, MaterialType::kUnlit}}] =
+ auto unskinned_variant =
MakePipelineVariants<UnskinnedVertexShader, UnlitFragmentShader>(
*context_);
- pipelines_[{PipelineKey{GeometryType::kSkinned, MaterialType::kUnlit}}] =
+ if (!unskinned_variant) {
+ FML_LOG(ERROR) << "Could not create unskinned pipeline variant.";
+ return;
+ }
+ pipelines_[{PipelineKey{GeometryType::kUnskinned, MaterialType::kUnlit}}] =
+ std::move(unskinned_variant);
+
+ auto skinned_variant =
MakePipelineVariants<SkinnedVertexShader, UnlitFragmentShader>(*context_);
+ if (!skinned_variant) {
+ FML_LOG(ERROR) << "Could not create skinned pipeline variant.";
+ return;
+ }
+ pipelines_[{PipelineKey{GeometryType::kSkinned, MaterialType::kUnlit}}] =
+ std::move(skinned_variant);
{
impeller::TextureDescriptor texture_descriptor;
diff --git a/impeller/scene/scene_context.h b/impeller/scene/scene_context.h
index 615ec4a..f85663b 100644
--- a/impeller/scene/scene_context.h
+++ b/impeller/scene/scene_context.h
@@ -67,9 +67,14 @@
public:
explicit PipelineVariantsT(Context& context) {
auto desc = PipelineT::Builder::MakeDefaultPipelineDescriptor(context);
+ if (!desc.has_value()) {
+ is_valid_ = false;
+ return;
+ }
// Apply default ContentContextOptions to the descriptor.
SceneContextOptions{}.ApplyToPipelineDescriptor(
- *context.GetCapabilities(), *desc);
+ /*capabilities=*/*context.GetCapabilities(),
+ /*desc=*/desc.value());
variants_[{}] = std::make_unique<PipelineT>(context, desc);
};
@@ -99,7 +104,10 @@
return variant_pipeline;
}
+ bool IsValid() const { return is_valid_; }
+
private:
+ bool is_valid_ = true;
std::unordered_map<SceneContextOptions,
std::unique_ptr<PipelineT>,
SceneContextOptions::Hash,
@@ -108,10 +116,19 @@
};
template <typename VertexShader, typename FragmentShader>
+ /// Creates a PipelineVariantsT for the given vertex and fragment shaders.
+ ///
+ /// If a pipeline could not be created, returns nullptr.
std::unique_ptr<PipelineVariants> MakePipelineVariants(Context& context) {
+ auto pipeline =
+ PipelineVariantsT<RenderPipelineT<VertexShader, FragmentShader>>(
+ context);
+ if (!pipeline.IsValid()) {
+ return nullptr;
+ }
return std::make_unique<
PipelineVariantsT<RenderPipelineT<VertexShader, FragmentShader>>>(
- context);
+ std::move(pipeline));
}
std::unordered_map<PipelineKey,
