Fix crash for missing class ids causing zero sized texture (#1947)

* Fix crash for missing class ids causing zero sized texture Two things fixed actually: * texture manager now checks for zero sized texture, this ripples out in a lot more error handling * class id texture texture handles not having any classes gracefully * Use Display for all errors * typo * Better naming of error * Better docs and names * Fix off-by-one error * some use of `context`, change which error is implicitly converted on texture manager2d --------- Co-authored-by: Emil Ernerfeldt <[email protected]>
rerun-io · Apr 21, 2023 · 593a34b · 593a34b
1 parent a3178e6
commit 593a34b
Show file tree

Hide file tree

Showing 12 changed files with 191 additions and 113 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/re_renderer/Cargo.toml b/crates/re_renderer/Cargo.toml
@@ -55,6 +55,7 @@ glam = { workspace = true, features = ["bytemuck"] }
 half = { workspace = true, features = ["bytemuck"] }
 itertools = { workspace = true }
 macaw.workspace = true
+never = '0.1'
 ordered-float = "3.2"
 parking_lot.workspace = true
 slotmap = "1.0.6"

diff --git a/crates/re_renderer/examples/2d.rs b/crates/re_renderer/examples/2d.rs
@@ -36,16 +36,19 @@ impl framework::Example for Render2D {
             );
         }
 
-        let rerun_logo_texture = re_ctx.texture_manager_2d.create(
-            &mut re_ctx.gpu_resources.textures,
-            &Texture2DCreationDesc {
-                label: "rerun logo".into(),
-                data: image_data.into(),
-                format: wgpu::TextureFormat::Rgba8UnormSrgb,
-                width: rerun_logo.width(),
-                height: rerun_logo.height(),
-            },
-        );
+        let rerun_logo_texture = re_ctx
+            .texture_manager_2d
+            .create(
+                &mut re_ctx.gpu_resources.textures,
+                &Texture2DCreationDesc {
+                    label: "rerun logo".into(),
+                    data: image_data.into(),
+                    format: wgpu::TextureFormat::Rgba8UnormSrgb,
+                    width: rerun_logo.width(),
+                    height: rerun_logo.height(),
+                },
+            )
+            .expect("Failed to create texture for rerun logo");
         Render2D {
             rerun_logo_texture,
 

diff --git a/crates/re_renderer/examples/depth_cloud.rs b/crates/re_renderer/examples/depth_cloud.rs
@@ -408,17 +408,20 @@ impl DepthTexture {
         });
 
         let label = format!("depth texture spiral {dimensions}");
-        let texture = re_ctx.texture_manager_2d.get_or_create(
-            hash(&label),
-            &mut re_ctx.gpu_resources.textures,
-            Texture2DCreationDesc {
-                label: label.into(),
-                data: bytemuck::cast_slice(&data).into(),
-                format: wgpu::TextureFormat::R32Float,
-                width: dimensions.x,
-                height: dimensions.y,
-            },
-        );
+        let texture = re_ctx
+            .texture_manager_2d
+            .get_or_create(
+                hash(&label),
+                &mut re_ctx.gpu_resources.textures,
+                Texture2DCreationDesc {
+                    label: label.into(),
+                    data: bytemuck::cast_slice(&data).into(),
+                    format: wgpu::TextureFormat::R32Float,
+                    width: dimensions.x,
+                    height: dimensions.y,
+                },
+            )
+            .expect("Failed to create depth texture.");
 
         Self {
             dimensions,
@@ -448,17 +451,20 @@ impl AlbedoTexture {
         });
 
         let label = format!("albedo texture spiral {dimensions}");
-        let texture = re_ctx.texture_manager_2d.get_or_create(
-            hash(&label),
-            &mut re_ctx.gpu_resources.textures,
-            Texture2DCreationDesc {
-                label: label.into(),
-                data: bytemuck::cast_slice(&rgba8).into(),
-                format: wgpu::TextureFormat::Rgba8UnormSrgb,
-                width: dimensions.x,
-                height: dimensions.y,
-            },
-        );
+        let texture = re_ctx
+            .texture_manager_2d
+            .get_or_create(
+                hash(&label),
+                &mut re_ctx.gpu_resources.textures,
+                Texture2DCreationDesc {
+                    label: label.into(),
+                    data: bytemuck::cast_slice(&rgba8).into(),
+                    format: wgpu::TextureFormat::Rgba8UnormSrgb,
+                    width: dimensions.x,
+                    height: dimensions.y,
+                },
+            )
+            .expect("Failed to create albedo texture.");
 
         Self {
             dimensions,

diff --git a/crates/re_renderer/src/importer/gltf.rs b/crates/re_renderer/src/importer/gltf.rs
@@ -68,8 +68,16 @@ pub fn load_gltf_from_buffer(
         };
 
         images_as_textures.push(
-            ctx.texture_manager_2d
-                .create(&mut ctx.gpu_resources.textures, &texture),
+            match ctx
+                .texture_manager_2d
+                .create(&mut ctx.gpu_resources.textures, &texture)
+            {
+                Ok(texture) => texture,
+                Err(err) => {
+                    re_log::error!("Failed to create texture: {err}");
+                    ctx.texture_manager_2d.white_texture_unorm_handle().clone()
+                }
+            },
         );
     }
 

diff --git a/crates/re_renderer/src/resource_managers/mod.rs b/crates/re_renderer/src/resource_managers/mod.rs
@@ -10,7 +10,10 @@ mod mesh_manager;
 pub use mesh_manager::{GpuMeshHandle, MeshManager};
 
 mod texture_manager;
-pub use texture_manager::{GpuTexture2D, Texture2DCreationDesc, TextureManager2D};
+pub use texture_manager::{
+    GpuTexture2D, Texture2DCreationDesc, TextureCreationError, TextureManager2D,
+    TextureManager2DError,
+};
 
 mod resource_manager;
 pub use resource_manager::{ResourceHandle, ResourceLifeTime, ResourceManagerError};
diff --git a/crates/re_renderer/src/resource_managers/texture_manager.rs b/crates/re_renderer/src/resource_managers/texture_manager.rs
@@ -95,6 +95,33 @@ impl<'a> Texture2DCreationDesc<'a> {
     }
 }
 
+// TODO(andreas): Move this to texture pool.
+#[derive(thiserror::Error, Debug)]
+pub enum TextureCreationError {
+    #[error("Texture with debug label {0:?} has zero width or height!")]
+    ZeroSize(DebugLabel),
+}
+
+#[derive(thiserror::Error, Debug)]
+pub enum TextureManager2DError<DataCreationError> {
+    /// Something went wrong when creating the GPU texture.
+    #[error(transparent)]
+    TextureCreation(#[from] TextureCreationError),
+
+    /// Something went wrong in a user-callback.
+    #[error(transparent)]
+    DataCreation(DataCreationError),
+}
+
+impl From<TextureManager2DError<never::Never>> for TextureCreationError {
+    fn from(err: TextureManager2DError<never::Never>) -> Self {
+        match err {
+            TextureManager2DError::TextureCreation(texture_creation) => texture_creation,
+            TextureManager2DError::DataCreation(never) => match never {},
+        }
+    }
+}
+
 /// Texture manager for 2D textures.
 ///
 /// The scope is intentionally limited to particular kinds of textures that currently
@@ -146,7 +173,8 @@ impl TextureManager2D {
                 width: 1,
                 height: 1,
             },
-        );
+        )
+        .expect("Failed to create white pixel texture!");
 
         let zeroed_texture_float =
             create_zero_texture(texture_pool, &device, wgpu::TextureFormat::Rgba8Unorm);
@@ -176,7 +204,7 @@ impl TextureManager2D {
         &mut self,
         texture_pool: &mut GpuTexturePool,
         creation_desc: &Texture2DCreationDesc<'_>,
-    ) -> GpuTexture2D {
+    ) -> Result<GpuTexture2D, TextureCreationError> {
         // TODO(andreas): Disabled the warning as we're moving towards using this texture manager for user-logged images.
         // However, it's still very much a concern especially once we add mipmapping. Something we need to keep in mind.
         //
@@ -203,39 +231,47 @@ impl TextureManager2D {
         key: u64,
         texture_pool: &mut GpuTexturePool,
         texture_desc: Texture2DCreationDesc<'_>,
-    ) -> GpuTexture2D {
-        enum Never {}
-        match self.get_or_create_with(key, texture_pool, || -> Result<_, Never> {
-            Ok(texture_desc)
-        }) {
-            Ok(tex_handle) => tex_handle,
-            Err(never) => match never {},
-        }
+    ) -> Result<GpuTexture2D, TextureCreationError> {
+        self.get_or_create_with(key, texture_pool, || texture_desc)
     }
 
     /// Creates a new 2D texture resource and schedules data upload to the GPU if a texture
     /// wasn't already created using the same key.
-    pub fn get_or_create_with<'a, Err>(
+    pub fn get_or_create_with<'a>(
+        &mut self,
+        key: u64,
+        texture_pool: &mut GpuTexturePool,
+        create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>,
+    ) -> Result<GpuTexture2D, TextureCreationError> {
+        self.get_or_try_create_with(key, texture_pool, || -> Result<_, never::Never> {
+            Ok(create_texture_desc())
+        })
+        .map_err(|err| err.into())
+    }
+
+    /// Creates a new 2D texture resource and schedules data upload to the GPU if a texture
+    /// wasn't already created using the same key.
+    pub fn get_or_try_create_with<'a, Err: std::fmt::Display>(
         &mut self,
         key: u64,
         texture_pool: &mut GpuTexturePool,
         try_create_texture_desc: impl FnOnce() -> Result<Texture2DCreationDesc<'a>, Err>,
-    ) -> Result<GpuTexture2D, Err> {
+    ) -> Result<GpuTexture2D, TextureManager2DError<Err>> {
         let texture_handle = match self.texture_cache.entry(key) {
             std::collections::hash_map::Entry::Occupied(texture_handle) => {
                 texture_handle.get().clone() // already inserted
             }
             std::collections::hash_map::Entry::Vacant(entry) => {
                 // Run potentially expensive texture creation code:
-                let tex_creation_desc = try_create_texture_desc()?;
-                entry
-                    .insert(Self::create_and_upload_texture(
-                        &self.device,
-                        &self.queue,
-                        texture_pool,
-                        &tex_creation_desc,
-                    ))
-                    .clone()
+                let tex_creation_desc = try_create_texture_desc()
+                    .map_err(|err| TextureManager2DError::DataCreation(err))?;
+                let texture = Self::create_and_upload_texture(
+                    &self.device,
+                    &self.queue,
+                    texture_pool,
+                    &tex_creation_desc,
+                )?;
+                entry.insert(texture).clone()
             }
         };
 
@@ -278,8 +314,13 @@ impl TextureManager2D {
         queue: &wgpu::Queue,
         texture_pool: &mut GpuTexturePool,
         creation_desc: &Texture2DCreationDesc<'_>,
-    ) -> GpuTexture2D {
+    ) -> Result<GpuTexture2D, TextureCreationError> {
         crate::profile_function!();
+
+        if creation_desc.width == 0 || creation_desc.height == 0 {
+            return Err(TextureCreationError::ZeroSize(creation_desc.label.clone()));
+        }
+
         let size = wgpu::Extent3d {
             width: creation_desc.width,
             height: creation_desc.height,
@@ -329,7 +370,7 @@ impl TextureManager2D {
 
         // TODO(andreas): mipmap generation
 
-        GpuTexture2D(texture)
+        Ok(GpuTexture2D(texture))
     }
 
     pub(crate) fn begin_frame(&mut self, _frame_index: u64) {

diff --git a/crates/re_viewer/src/gpu_bridge/mod.rs b/crates/re_viewer/src/gpu_bridge/mod.rs
@@ -9,7 +9,9 @@ use egui::mutex::Mutex;
 
 use re_renderer::{
     renderer::{ColormappedTexture, RectangleOptions},
-    resource_managers::{GpuTexture2D, Texture2DCreationDesc},
+    resource_managers::{
+        GpuTexture2D, Texture2DCreationDesc, TextureCreationError, TextureManager2DError,
+    },
     RenderContext, ViewBuilder,
 };
 
@@ -52,12 +54,12 @@ pub fn viewport_resolution_in_pixels(clip_rect: egui::Rect, pixels_from_point: f
     [resolution.x as u32, resolution.y as u32]
 }
 
-pub fn try_get_or_create_texture<'a, Err>(
+pub fn try_get_or_create_texture<'a, Err: std::fmt::Display>(
     render_ctx: &mut RenderContext,
     texture_key: u64,
     try_create_texture_desc: impl FnOnce() -> Result<Texture2DCreationDesc<'a>, Err>,
-) -> Result<GpuTexture2D, Err> {
-    render_ctx.texture_manager_2d.get_or_create_with(
+) -> Result<GpuTexture2D, TextureManager2DError<Err>> {
+    render_ctx.texture_manager_2d.get_or_try_create_with(
         texture_key,
         &mut render_ctx.gpu_resources.textures,
         try_create_texture_desc,
@@ -68,17 +70,12 @@ pub fn get_or_create_texture<'a>(
     render_ctx: &mut RenderContext,
     texture_key: u64,
     create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>,
-) -> GpuTexture2D {
-    enum Never {}
-    let result: Result<GpuTexture2D, Never> = render_ctx.texture_manager_2d.get_or_create_with(
+) -> Result<GpuTexture2D, TextureCreationError> {
+    render_ctx.texture_manager_2d.get_or_create_with(
         texture_key,
         &mut render_ctx.gpu_resources.textures,
-        || Ok(create_texture_desc()),
-    );
-    match result {
-        Ok(handle) => handle,
-        Err(never) => match never {},
-    }
+        create_texture_desc,
+    )
 }
 
 /// Render a `re_render` view using the given clip rectangle.