Don't ignore draw errors (#13240)

# Objective - It's possible to have errors in a draw command, but these errors are ignored ## Solution - Return a result with the error ## Changelog Renamed `RenderCommandResult::Failure` to `RenderCommandResult::Skip` Added a `reason` string parameter to `RenderCommandResult::Failure` ## Migration Guide If you were using `RenderCommandResult::Failure` to just ignore an error and retry later, use `RenderCommandResult::Skip` instead. This wasn't intentional, but this PR should also help with #12660 since we can turn a few unwraps into error messages now. --------- Co-authored-by: Charlotte McElwain <[email protected]>
bevyengine · Jul 22, 2024 · 3faca1e · 3faca1e
1 parent ee88d79
commit 3faca1e
Show file tree

Hide file tree

Showing 19 changed files with 170 additions and 82 deletions.
diff --git a/crates/bevy_core_pipeline/src/core_2d/main_transparent_pass_2d_node.rs b/crates/bevy_core_pipeline/src/core_2d/main_transparent_pass_2d_node.rs
@@ -58,7 +58,7 @@ impl ViewNode for MainTransparentPass2dNode {
             }
 
             if !transparent_phase.items.is_empty() {
-                transparent_phase.render(&mut render_pass, world, view_entity);
+                transparent_phase.render(&mut render_pass, world, view_entity)?;
             }
 
             pass_span.end(&mut render_pass);

diff --git a/crates/bevy_core_pipeline/src/core_3d/main_opaque_pass_3d_node.rs b/crates/bevy_core_pipeline/src/core_3d/main_opaque_pass_3d_node.rs
@@ -12,6 +12,7 @@ use bevy_render::{
     renderer::RenderContext,
     view::{ViewDepthTexture, ViewTarget, ViewUniformOffset},
 };
+use bevy_utils::tracing::error;
 #[cfg(feature = "trace")]
 use bevy_utils::tracing::info_span;
 
@@ -95,14 +96,18 @@ impl ViewNode for MainOpaquePass3dNode {
             if !opaque_phase.is_empty() {
                 #[cfg(feature = "trace")]
                 let _opaque_main_pass_3d_span = info_span!("opaque_main_pass_3d").entered();
-                opaque_phase.render(&mut render_pass, world, view_entity);
+                if let Err(err) = opaque_phase.render(&mut render_pass, world, view_entity) {
+                    error!("Error encountered while rendering the opaque phase {err:?}");
+                }
             }
 
             // Alpha draws
             if !alpha_mask_phase.is_empty() {
                 #[cfg(feature = "trace")]
                 let _alpha_mask_main_pass_3d_span = info_span!("alpha_mask_main_pass_3d").entered();
-                alpha_mask_phase.render(&mut render_pass, world, view_entity);
+                if let Err(err) = alpha_mask_phase.render(&mut render_pass, world, view_entity) {
+                    error!("Error encountered while rendering the alpha mask phase {err:?}");
+                }
             }
 
             // Skybox draw using a fullscreen triangle

diff --git a/crates/bevy_core_pipeline/src/core_3d/main_transmissive_pass_3d_node.rs b/crates/bevy_core_pipeline/src/core_3d/main_transmissive_pass_3d_node.rs
@@ -9,6 +9,7 @@ use bevy_render::{
     renderer::RenderContext,
     view::{ViewDepthTexture, ViewTarget},
 };
+use bevy_utils::tracing::error;
 #[cfg(feature = "trace")]
 use bevy_utils::tracing::info_span;
 use std::ops::Range;
@@ -98,7 +99,11 @@ impl ViewNode for MainTransmissivePass3dNode {
                     }
 
                     // render items in range
-                    transmissive_phase.render_range(&mut render_pass, world, view_entity, range);
+                    if let Err(err) =
+                        transmissive_phase.render_range(&mut render_pass, world, view_entity, range)
+                    {
+                        error!("Error encountered while rendering the transmissive phase {err:?}");
+                    }
                 }
             } else {
                 let mut render_pass =
@@ -108,7 +113,9 @@ impl ViewNode for MainTransmissivePass3dNode {
                     render_pass.set_camera_viewport(viewport);
                 }
 
-                transmissive_phase.render(&mut render_pass, world, view_entity);
+                if let Err(err) = transmissive_phase.render(&mut render_pass, world, view_entity) {
+                    error!("Error encountered while rendering the transmissive phase {err:?}");
+                }
             }
         }
 

diff --git a/crates/bevy_core_pipeline/src/core_3d/main_transparent_pass_3d_node.rs b/crates/bevy_core_pipeline/src/core_3d/main_transparent_pass_3d_node.rs
@@ -9,6 +9,7 @@ use bevy_render::{
     renderer::RenderContext,
     view::{ViewDepthTexture, ViewTarget},
 };
+use bevy_utils::tracing::error;
 #[cfg(feature = "trace")]
 use bevy_utils::tracing::info_span;
 
@@ -70,7 +71,9 @@ impl ViewNode for MainTransparentPass3dNode {
                 render_pass.set_camera_viewport(viewport);
             }
 
-            transparent_phase.render(&mut render_pass, world, view_entity);
+            if let Err(err) = transparent_phase.render(&mut render_pass, world, view_entity) {
+                error!("Error encountered while rendering the transparent phase {err:?}");
+            }
 
             pass_span.end(&mut render_pass);
         }

diff --git a/crates/bevy_core_pipeline/src/deferred/node.rs b/crates/bevy_core_pipeline/src/deferred/node.rs
@@ -11,6 +11,7 @@ use bevy_render::{
     renderer::RenderContext,
     view::ViewDepthTexture,
 };
+use bevy_utils::tracing::error;
 #[cfg(feature = "trace")]
 use bevy_utils::tracing::info_span;
 
@@ -149,14 +150,23 @@ impl ViewNode for DeferredGBufferPrepassNode {
             {
                 #[cfg(feature = "trace")]
                 let _opaque_prepass_span = info_span!("opaque_deferred_prepass").entered();
-                opaque_deferred_phase.render(&mut render_pass, world, view_entity);
+                if let Err(err) = opaque_deferred_phase.render(&mut render_pass, world, view_entity)
+                {
+                    error!("Error encountered while rendering the opaque deferred phase {err:?}");
+                }
             }
 
             // Alpha masked draws
             if !alpha_mask_deferred_phase.is_empty() {
                 #[cfg(feature = "trace")]
                 let _alpha_mask_deferred_span = info_span!("alpha_mask_deferred_prepass").entered();
-                alpha_mask_deferred_phase.render(&mut render_pass, world, view_entity);
+                if let Err(err) =
+                    alpha_mask_deferred_phase.render(&mut render_pass, world, view_entity)
+                {
+                    error!(
+                        "Error encountered while rendering the alpha mask deferred phase {err:?}"
+                    );
+                }
             }
 
             drop(render_pass);

diff --git a/crates/bevy_core_pipeline/src/prepass/node.rs b/crates/bevy_core_pipeline/src/prepass/node.rs
@@ -9,6 +9,7 @@ use bevy_render::{
     renderer::RenderContext,
     view::{ViewDepthTexture, ViewUniformOffset},
 };
+use bevy_utils::tracing::error;
 #[cfg(feature = "trace")]
 use bevy_utils::tracing::info_span;
 
@@ -125,14 +126,23 @@ impl ViewNode for PrepassNode {
             {
                 #[cfg(feature = "trace")]
                 let _opaque_prepass_span = info_span!("opaque_prepass").entered();
-                opaque_prepass_phase.render(&mut render_pass, world, view_entity);
+                if let Err(err) = opaque_prepass_phase.render(&mut render_pass, world, view_entity)
+                {
+                    error!("Error encountered while rendering the opaque prepass phase {err:?}");
+                }
             }
 
             // Alpha masked draws
             if !alpha_mask_prepass_phase.is_empty() {
                 #[cfg(feature = "trace")]
                 let _alpha_mask_prepass_span = info_span!("alpha_mask_prepass").entered();
-                alpha_mask_prepass_phase.render(&mut render_pass, world, view_entity);
+                if let Err(err) =
+                    alpha_mask_prepass_phase.render(&mut render_pass, world, view_entity)
+                {
+                    error!(
+                        "Error encountered while rendering the alpha mask prepass phase {err:?}"
+                    );
+                }
             }
 
             // Skybox draw using a fullscreen triangle

diff --git a/crates/bevy_gizmos/src/lib.rs b/crates/bevy_gizmos/src/lib.rs
@@ -540,7 +540,7 @@ impl<const I: usize, P: PhaseItem> RenderCommand<P> for SetLineGizmoBindGroup<I>
         pass: &mut TrackedRenderPass<'w>,
     ) -> RenderCommandResult {
         let Some(uniform_index) = uniform_index else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
         pass.set_bind_group(
             I,
@@ -566,10 +566,10 @@ impl<P: PhaseItem> RenderCommand<P> for DrawLineGizmo {
         pass: &mut TrackedRenderPass<'w>,
     ) -> RenderCommandResult {
         let Some(handle) = handle else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
         let Some(line_gizmo) = line_gizmos.into_inner().get(handle) else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
 
         if line_gizmo.vertex_count < 2 {
@@ -612,10 +612,10 @@ impl<P: PhaseItem> RenderCommand<P> for DrawLineJointGizmo {
         pass: &mut TrackedRenderPass<'w>,
     ) -> RenderCommandResult {
         let Some(handle) = handle else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
         let Some(line_gizmo) = line_gizmos.into_inner().get(handle) else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
 
         if line_gizmo.vertex_count <= 2 || !line_gizmo.strip {

diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs
@@ -456,10 +456,10 @@ impl<P: PhaseItem, M: Material, const I: usize> RenderCommand<P> for SetMaterial
         let material_instances = material_instances.into_inner();
 
         let Some(material_asset_id) = material_instances.get(&item.entity()) else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
         let Some(material) = materials.get(*material_asset_id) else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
         pass.set_bind_group(I, &material.bind_group, &[]);
         RenderCommandResult::Success

diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs
@@ -1445,7 +1445,11 @@ impl Node for ShadowPassNode {
                     let pass_span =
                         diagnostics.pass_span(&mut render_pass, view_light.pass_name.clone());
 
-                    shadow_phase.render(&mut render_pass, world, view_light_entity);
+                    if let Err(err) =
+                        shadow_phase.render(&mut render_pass, world, view_light_entity)
+                    {
+                        error!("Error encountered while rendering the shadow phase {err:?}");
+                    }
 
                     pass_span.end(&mut render_pass);
                     drop(render_pass);

diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs
@@ -2261,12 +2261,11 @@ impl<P: PhaseItem, const I: usize> RenderCommand<P> for SetMeshBindGroup<I> {
             is_morphed,
             has_motion_vector_prepass,
         ) else {
-            error!(
+            return RenderCommandResult::Failure(
                 "The MeshBindGroups resource wasn't set in the render phase. \
                 It should be set by the prepare_mesh_bind_group system.\n\
-                This is a bevy bug! Please open an issue."
+                This is a bevy bug! Please open an issue.",
             );
-            return RenderCommandResult::Failure;
         };
 
         let mut dynamic_offsets: [u32; 3] = Default::default();
@@ -2349,7 +2348,7 @@ impl<P: PhaseItem> RenderCommand<P> for DrawMesh {
             if !has_preprocess_bind_group
                 || !preprocess_pipelines.pipelines_are_loaded(&pipeline_cache)
             {
-                return RenderCommandResult::Failure;
+                return RenderCommandResult::Skip;
             }
         }
 
@@ -2359,13 +2358,13 @@ impl<P: PhaseItem> RenderCommand<P> for DrawMesh {
         let mesh_allocator = mesh_allocator.into_inner();
 
         let Some(mesh_asset_id) = mesh_instances.mesh_asset_id(item.entity()) else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
         let Some(gpu_mesh) = meshes.get(mesh_asset_id) else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
         let Some(vertex_buffer_slice) = mesh_allocator.mesh_vertex_slice(&mesh_asset_id) else {
-            return RenderCommandResult::Failure;
+            return RenderCommandResult::Skip;
         };
 
         // Calculate the indirect offset, and look up the buffer.
@@ -2374,7 +2373,7 @@ impl<P: PhaseItem> RenderCommand<P> for DrawMesh {
             Some(index) => match indirect_parameters_buffer.buffer() {
                 None => {
                     warn!("Not rendering mesh because indirect parameters buffer wasn't present");
-                    return RenderCommandResult::Failure;
+                    return RenderCommandResult::Skip;
                 }
                 Some(buffer) => Some((
                     index as u64 * mem::size_of::<IndirectParameters>() as u64,
@@ -2395,7 +2394,7 @@ impl<P: PhaseItem> RenderCommand<P> for DrawMesh {
             } => {
                 let Some(index_buffer_slice) = mesh_allocator.mesh_index_slice(&mesh_asset_id)
                 else {
-                    return RenderCommandResult::Failure;
+                    return RenderCommandResult::Skip;
                 };
 
                 pass.set_index_buffer(index_buffer_slice.buffer.slice(..), 0, *index_format);

diff --git a/crates/bevy_render/src/render_graph/node.rs b/crates/bevy_render/src/render_graph/node.rs
@@ -3,6 +3,7 @@ use crate::{
         Edge, InputSlotError, OutputSlotError, RenderGraphContext, RenderGraphError,
         RunSubGraphError, SlotInfo, SlotInfos,
     },
+    render_phase::DrawError,
     renderer::RenderContext,
 };
 pub use bevy_ecs::label::DynEq;
@@ -97,6 +98,8 @@ pub enum NodeRunError {
     OutputSlotError(#[from] OutputSlotError),
     #[error("encountered an error when running a sub-graph")]
     RunSubGraphError(#[from] RunSubGraphError),
+    #[error("encountered an error when executing draw command")]
+    DrawError(#[from] DrawError),
 }
 
 /// A collection of input and output [`Edges`](Edge) for a [`Node`].

diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs
@@ -2,7 +2,7 @@ use crate::render_phase::{PhaseItem, TrackedRenderPass};
 use bevy_app::{App, SubApp};
 use bevy_ecs::{
     entity::Entity,
-    query::{QueryState, ROQueryItem, ReadOnlyQueryData},
+    query::{QueryEntityError, QueryState, ROQueryItem, ReadOnlyQueryData},
     system::{ReadOnlySystemParam, Resource, SystemParam, SystemParamItem, SystemState},
     world::World,
 };
@@ -13,6 +13,7 @@ use std::{
     hash::Hash,
     sync::{PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard},
 };
+use thiserror::Error;
 
 /// A draw function used to draw [`PhaseItem`]s.
 ///
@@ -34,7 +35,17 @@ pub trait Draw<P: PhaseItem>: Send + Sync + 'static {
         pass: &mut TrackedRenderPass<'w>,
         view: Entity,
         item: &P,
-    );
+    ) -> Result<(), DrawError>;
+}
+
+#[derive(Error, Debug, PartialEq, Eq)]
+pub enum DrawError {
+    #[error("Failed to execute render command {0:?}")]
+    RenderCommandFailure(&'static str),
+    #[error("Failed to get execute view query")]
+    InvalidViewQuery,
+    #[error("View entity not found")]
+    ViewEntityNotFound,
 }
 
 // TODO: make this generic?
@@ -212,7 +223,8 @@ pub trait RenderCommand<P: PhaseItem> {
 #[derive(Debug)]
 pub enum RenderCommandResult {
     Success,
-    Failure,
+    Skip,
+    Failure(&'static str),
 }
 
 macro_rules! render_command_tuple_impl {
@@ -232,14 +244,22 @@ macro_rules! render_command_tuple_impl {
             ) -> RenderCommandResult {
                 match maybe_entities {
                     None => {
-                        $(if let RenderCommandResult::Failure = $name::render(_item, $view, None, $name, _pass) {
-                            return RenderCommandResult::Failure;
-                        })*
+                        $(
+                            match $name::render(_item, $view, None, $name, _pass) {
+                                RenderCommandResult::Skip => return RenderCommandResult::Skip,
+                                RenderCommandResult::Failure(reason) => return RenderCommandResult::Failure(reason),
+                                _ => {},
+                            }
+                        )*
                     }
                     Some(($($entity,)*)) => {
-                        $(if let RenderCommandResult::Failure = $name::render(_item, $view, Some($entity), $name, _pass) {
-                            return RenderCommandResult::Failure;
-                        })*
+                        $(
+                            match $name::render(_item, $view, Some($entity), $name, _pass) {
+                                RenderCommandResult::Skip => return RenderCommandResult::Skip,
+                                RenderCommandResult::Failure(reason) => return RenderCommandResult::Failure(reason),
+                                _ => {},
+                            }
+                        )*
                     }
                 }
                 RenderCommandResult::Success
@@ -290,12 +310,23 @@ where
         pass: &mut TrackedRenderPass<'w>,
         view: Entity,
         item: &P,
-    ) {
+    ) -> Result<(), DrawError> {
         let param = self.state.get_manual(world);
-        let view = self.view.get_manual(world, view).unwrap();
+        let view = match self.view.get_manual(world, view) {
+            Ok(view) => view,
+            Err(err) => match err {
+                QueryEntityError::NoSuchEntity(_) => return Err(DrawError::ViewEntityNotFound),
+                QueryEntityError::QueryDoesNotMatch(_) | QueryEntityError::AliasedMutability(_) => {
+                    return Err(DrawError::InvalidViewQuery)
+                }
+            },
+        };
+
         let entity = self.entity.get_manual(world, item.entity()).ok();
-        // TODO: handle/log `RenderCommand` failure
-        C::render(item, view, entity, param, pass);
+        match C::render(item, view, entity, param, pass) {
+            RenderCommandResult::Success | RenderCommandResult::Skip => Ok(()),
+            RenderCommandResult::Failure(reason) => Err(DrawError::RenderCommandFailure(reason)),
+        }
     }
 }