From 78b904d9b93e8aae4209c50efb26ad6226efb8b1 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 20 Apr 2023 18:13:28 +0200 Subject: [PATCH 1/7] 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 --- crates/re_renderer/examples/2d.rs | 23 ++++--- crates/re_renderer/examples/depth_cloud.rs | 50 ++++++++------- crates/re_renderer/src/importer/gltf.rs | 12 +++- .../re_renderer/src/resource_managers/mod.rs | 4 +- .../src/resource_managers/texture_manager.rs | 41 ++++++++----- crates/re_viewer/src/gpu_bridge/mod.rs | 15 ++--- .../re_viewer/src/gpu_bridge/tensor_to_gpu.rs | 18 ++++-- .../view_spatial/scene/scene_part/images.rs | 61 ++++++++++--------- .../src/ui/view_tensor/tensor_slice_to_gpu.rs | 12 ++-- crates/re_viewer/src/ui/view_tensor/ui.rs | 3 +- 10 files changed, 138 insertions(+), 101 deletions(-) diff --git a/crates/re_renderer/examples/2d.rs b/crates/re_renderer/examples/2d.rs index 32940991c1f2..eb17d6955cfd 100644 --- 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 index d1bf120fa325..dddc40d1ef71 100644 --- 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 index 9d628931c6f3..77509f318e81 100644 --- 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 index 7e1eff185a79..0d5c10efb534 100644 --- a/crates/re_renderer/src/resource_managers/mod.rs +++ b/crates/re_renderer/src/resource_managers/mod.rs @@ -10,7 +10,9 @@ 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, 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 index 04999dc7ffe8..8bba0bdf7494 100644 --- a/crates/re_renderer/src/resource_managers/texture_manager.rs +++ b/crates/re_renderer/src/resource_managers/texture_manager.rs @@ -95,6 +95,15 @@ impl<'a> Texture2DCreationDesc<'a> { } } +#[derive(thiserror::Error, Debug)] +pub enum TextureManager2DError { + #[error("Texture with debug label {0:?} has zero width or height!")] + ZeroSize(DebugLabel), + + #[error(transparent)] + DataCreationError(#[from] DataCreationError), +} + /// Texture manager for 2D textures. /// /// The scope is intentionally limited to particular kinds of textures that currently @@ -135,7 +144,7 @@ impl TextureManager2D { ) -> Self { crate::profile_function!(); - let white_texture_unorm = Self::create_and_upload_texture( + let white_texture_unorm = Self::create_and_upload_texture::<()>( &device, &queue, texture_pool, @@ -146,7 +155,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 +186,7 @@ impl TextureManager2D { &mut self, texture_pool: &mut GpuTexturePool, creation_desc: &Texture2DCreationDesc<'_>, - ) -> GpuTexture2D { + ) -> Result> { // 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,14 +213,8 @@ 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> { + self.get_or_create_with(key, texture_pool, || -> Result<_, ()> { Ok(texture_desc) }) } /// Creates a new 2D texture resource and schedules data upload to the GPU if a texture @@ -220,7 +224,7 @@ impl TextureManager2D { key: u64, texture_pool: &mut GpuTexturePool, try_create_texture_desc: impl FnOnce() -> Result, Err>, - ) -> Result { + ) -> Result> { let texture_handle = match self.texture_cache.entry(key) { std::collections::hash_map::Entry::Occupied(texture_handle) => { texture_handle.get().clone() // already inserted @@ -234,7 +238,7 @@ impl TextureManager2D { &self.queue, texture_pool, &tex_creation_desc, - )) + )?) .clone() } }; @@ -273,13 +277,18 @@ impl TextureManager2D { &self.zeroed_texture_uint.0 } - fn create_and_upload_texture( + fn create_and_upload_texture( device: &wgpu::Device, queue: &wgpu::Queue, texture_pool: &mut GpuTexturePool, creation_desc: &Texture2DCreationDesc<'_>, - ) -> GpuTexture2D { + ) -> Result> { crate::profile_function!(); + + if creation_desc.width == 0 || creation_desc.height == 0 { + return Err(TextureManager2DError::ZeroSize(creation_desc.label.clone())); + } + let size = wgpu::Extent3d { width: creation_desc.width, height: creation_desc.height, @@ -329,7 +338,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 index 176f28149dee..2f850be064a6 100644 --- a/crates/re_viewer/src/gpu_bridge/mod.rs +++ b/crates/re_viewer/src/gpu_bridge/mod.rs @@ -9,7 +9,7 @@ use egui::mutex::Mutex; use re_renderer::{ renderer::{ColormappedTexture, RectangleOptions}, - resource_managers::{GpuTexture2D, Texture2DCreationDesc}, + resource_managers::{GpuTexture2D, Texture2DCreationDesc, TextureManager2DError}, RenderContext, ViewBuilder, }; @@ -56,7 +56,7 @@ pub fn try_get_or_create_texture<'a, Err>( render_ctx: &mut RenderContext, texture_key: u64, try_create_texture_desc: impl FnOnce() -> Result, Err>, -) -> Result { +) -> Result> { render_ctx.texture_manager_2d.get_or_create_with( texture_key, &mut render_ctx.gpu_resources.textures, @@ -68,17 +68,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 = render_ctx.texture_manager_2d.get_or_create_with( +) -> Result> { + 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 {}, - } + ) } /// Render a `re_render` view using the given clip rectangle. diff --git a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs index adcd47dd0922..6a255a9f50b8 100644 --- a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs +++ b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs @@ -3,7 +3,7 @@ use std::borrow::Cow; use bytemuck::{allocation::pod_collect_to_vec, cast_slice, Pod}; -use egui::util::hash; +use egui::{util::hash, NumExt}; use wgpu::TextureFormat; use re_log_types::component_types::{DecodedTensor, Tensor, TensorData}; @@ -94,7 +94,8 @@ fn color_tensor_to_gpu( width, height, }) - })?; + }) + .map_err(|e| anyhow::anyhow!("Failed to create texture for color tensor: {e:?}"))?; let texture_format = texture_handle.format(); @@ -157,8 +158,10 @@ fn class_id_tensor_to_gpu( // We pack the colormap into a 2D texture so we don't go over the max texture size. // We only support u8 and u16 class ids, so 256^2 is the biggest texture we need. + // Note that if max is 0, we still need to generate a row to accomodate the 0 class id + // (also zero sized textures are not allowed) let colormap_width = 256; - let colormap_height = (max as usize + colormap_width - 1) / colormap_width; + let colormap_height = (max.at_least(1.0) as usize + colormap_width - 1) / colormap_width; let colormap_texture_handle = get_or_create_texture(render_ctx, hash(annotations.row_id), || { @@ -179,11 +182,13 @@ fn class_id_tensor_to_gpu( width: colormap_width as u32, height: colormap_height as u32, } - }); + }) + .map_err(|e| anyhow::anyhow!("Failed to create class_id_colormap: {e:?}"))?; let main_texture_handle = try_get_or_create_texture(render_ctx, hash(tensor.id()), || { general_texture_creation_desc_from_tensor(debug_name, tensor) - })?; + }) + .map_err(|e| anyhow::anyhow!("Failed to create texture for class id tensor: {e:?}"))?; Ok(ColormappedTexture { texture: main_texture_handle, @@ -212,7 +217,8 @@ fn depth_tensor_to_gpu( let texture = try_get_or_create_texture(render_ctx, hash(tensor.id()), || { general_texture_creation_desc_from_tensor(debug_name, tensor) - })?; + }) + .map_err(|e| anyhow::anyhow!("Failed to create depth tensor texture: {e:?}"))?; Ok(ColormappedTexture { texture, diff --git a/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs b/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs index 078574468943..e3c572bb9a41 100644 --- a/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs +++ b/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs @@ -290,35 +290,38 @@ impl ImagesPart { // So to not couple this, we use a different key here let texture_key = egui::util::hash((tensor.id(), "depth_cloud")); let mut data_f32 = Vec::new(); - ctx.render_ctx.texture_manager_2d.get_or_create_with( - texture_key, - &mut ctx.render_ctx.gpu_resources.textures, - || { - // TODO(andreas/cmc): Ideally we'd upload the u16 data as-is. - // However, R16Unorm is behind a feature flag and Depth16Unorm doesn't work on WebGL (and is awkward as this is a depth buffer format!). - let data = match &tensor.data { - TensorData::U16(data) => { - data_f32.extend(data.as_slice().iter().map(|d| *d as f32)); - bytemuck::cast_slice(&data_f32).into() - } - TensorData::F32(data) => bytemuck::cast_slice(data).into(), - _ => { - return Err(format!( - "Tensor datatype {} is not supported for back-projection", - tensor.dtype() - )); - } - }; - - Ok(Texture2DCreationDesc { - label: format!("Depth cloud for {ent_path:?}").into(), - data, - format: wgpu::TextureFormat::R32Float, - width: width as _, - height: height as _, - }) - }, - )? + ctx.render_ctx + .texture_manager_2d + .get_or_create_with( + texture_key, + &mut ctx.render_ctx.gpu_resources.textures, + || { + // TODO(andreas/cmc): Ideally we'd upload the u16 data as-is. + // However, R16Unorm is behind a feature flag and Depth16Unorm doesn't work on WebGL (and is awkward as this is a depth buffer format!). + let data = match &tensor.data { + TensorData::U16(data) => { + data_f32.extend(data.as_slice().iter().map(|d| *d as f32)); + bytemuck::cast_slice(&data_f32).into() + } + TensorData::F32(data) => bytemuck::cast_slice(data).into(), + _ => { + return Err(format!( + "Tensor datatype {} is not supported for back-projection", + tensor.dtype() + )); + } + }; + + Ok(Texture2DCreationDesc { + label: format!("Depth cloud for {ent_path:?}").into(), + data, + format: wgpu::TextureFormat::R32Float, + width: width as _, + height: height as _, + }) + }, + ) + .map_err(|e| format!("Failed to create depth cloud texture: {e:?}"))? }; let depth_from_world_scale = *properties.depth_from_world_scale.get(); diff --git a/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs b/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs index 761c46d5da77..57c530ed9b76 100644 --- a/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs +++ b/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs @@ -1,5 +1,8 @@ use re_log_types::{component_types::TensorCastError, DecodedTensor, TensorDataType}; -use re_renderer::{renderer::ColormappedTexture, resource_managers::Texture2DCreationDesc}; +use re_renderer::{ + renderer::ColormappedTexture, + resource_managers::{GpuTexture2D, Texture2DCreationDesc, TextureManager2DError}, +}; use crate::{ gpu_bridge::{range, RangeError}, @@ -28,10 +31,11 @@ pub fn colormapped_texture( tensor: &DecodedTensor, tensor_stats: &TensorStats, state: &ViewTensorState, -) -> Result { +) -> Result> { crate::profile_function!(); - let range = range(tensor_stats)?; + let range = + range(tensor_stats).map_err(|e| TextureManager2DError::DataCreationError(e.into()))?; let texture = upload_texture_slice_to_gpu(render_ctx, tensor, state.slice())?; let color_mapping = state.color_mapping(); @@ -50,7 +54,7 @@ fn upload_texture_slice_to_gpu( render_ctx: &mut re_renderer::RenderContext, tensor: &DecodedTensor, slice_selection: &SliceSelection, -) -> Result { +) -> Result> { let id = egui::util::hash((tensor.id(), slice_selection)); crate::gpu_bridge::try_get_or_create_texture(render_ctx, id, || { diff --git a/crates/re_viewer/src/ui/view_tensor/ui.rs b/crates/re_viewer/src/ui/view_tensor/ui.rs index b26e2ba2f03b..72678f38bac5 100644 --- a/crates/re_viewer/src/ui/view_tensor/ui.rs +++ b/crates/re_viewer/src/ui/view_tensor/ui.rs @@ -321,7 +321,8 @@ fn paint_colormap_gradient( width, height, } - }); + }) + .map_err(|e| anyhow::anyhow!("Failed to create horizontal gradient texture: {e:?}"))?; let colormapped_texture = re_renderer::renderer::ColormappedTexture { texture: horizontal_gradient, From a01584f720406aebebcfde95e864185685c26f70 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 20 Apr 2023 20:01:56 +0200 Subject: [PATCH 2/7] Use Display for all errors --- Cargo.lock | 7 +++ crates/re_renderer/Cargo.toml | 1 + crates/re_renderer/src/importer/gltf.rs | 2 +- .../re_renderer/src/resource_managers/mod.rs | 1 + .../src/resource_managers/texture_manager.rs | 63 +++++++++++++------ crates/re_viewer/src/gpu_bridge/mod.rs | 12 ++-- .../re_viewer/src/gpu_bridge/tensor_to_gpu.rs | 8 +-- .../view_spatial/scene/scene_part/images.rs | 4 +- .../src/ui/view_tensor/tensor_slice_to_gpu.rs | 2 +- crates/re_viewer/src/ui/view_tensor/ui.rs | 2 +- 10 files changed, 70 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d7917e62575f..a29d6bd5fb5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2855,6 +2855,12 @@ dependencies = [ "jni-sys", ] +[[package]] +name = "never" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c96aba5aa877601bb3f6dd6a63a969e1f82e60646e81e71b14496995e9853c91" + [[package]] name = "nix" version = "0.23.2" @@ -4026,6 +4032,7 @@ dependencies = [ "itertools", "log", "macaw", + "never", "notify", "ordered-float", "parking_lot 0.12.1", diff --git a/crates/re_renderer/Cargo.toml b/crates/re_renderer/Cargo.toml index 03bc85390551..db118315830c 100644 --- 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/src/importer/gltf.rs b/crates/re_renderer/src/importer/gltf.rs index 77509f318e81..ba259c9f30bb 100644 --- a/crates/re_renderer/src/importer/gltf.rs +++ b/crates/re_renderer/src/importer/gltf.rs @@ -74,7 +74,7 @@ pub fn load_gltf_from_buffer( { Ok(texture) => texture, Err(err) => { - re_log::error!("Failed to create texture: {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 index 0d5c10efb534..26d3a949d82c 100644 --- a/crates/re_renderer/src/resource_managers/mod.rs +++ b/crates/re_renderer/src/resource_managers/mod.rs @@ -12,6 +12,7 @@ pub use mesh_manager::{GpuMeshHandle, MeshManager}; mod texture_manager; pub use texture_manager::{ GpuTexture2D, Texture2DCreationDesc, TextureManager2D, TextureManager2DError, + ZeroSizeTextureError, }; mod resource_manager; diff --git a/crates/re_renderer/src/resource_managers/texture_manager.rs b/crates/re_renderer/src/resource_managers/texture_manager.rs index 8bba0bdf7494..a027d20e0200 100644 --- a/crates/re_renderer/src/resource_managers/texture_manager.rs +++ b/crates/re_renderer/src/resource_managers/texture_manager.rs @@ -95,15 +95,28 @@ impl<'a> Texture2DCreationDesc<'a> { } } +#[derive(thiserror::Error, Debug)] +#[error("Texture with debug label {0:?} has zero width or height!")] +pub struct ZeroSizeTextureError(DebugLabel); + #[derive(thiserror::Error, Debug)] pub enum TextureManager2DError { - #[error("Texture with debug label {0:?} has zero width or height!")] - ZeroSize(DebugLabel), + #[error(transparent)] + ZeroSize(ZeroSizeTextureError), #[error(transparent)] DataCreationError(#[from] DataCreationError), } +impl From> for ZeroSizeTextureError { + fn from(err: TextureManager2DError) -> Self { + match err { + TextureManager2DError::ZeroSize(zero_size) => zero_size, + TextureManager2DError::DataCreationError(never) => match never {}, + } + } +} + /// Texture manager for 2D textures. /// /// The scope is intentionally limited to particular kinds of textures that currently @@ -144,7 +157,7 @@ impl TextureManager2D { ) -> Self { crate::profile_function!(); - let white_texture_unorm = Self::create_and_upload_texture::<()>( + let white_texture_unorm = Self::create_and_upload_texture( &device, &queue, texture_pool, @@ -186,7 +199,7 @@ impl TextureManager2D { &mut self, texture_pool: &mut GpuTexturePool, creation_desc: &Texture2DCreationDesc<'_>, - ) -> Result> { + ) -> Result { // 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. // @@ -213,13 +226,27 @@ impl TextureManager2D { key: u64, texture_pool: &mut GpuTexturePool, texture_desc: Texture2DCreationDesc<'_>, - ) -> Result> { - self.get_or_create_with(key, texture_pool, || -> Result<_, ()> { Ok(texture_desc) }) + ) -> Result { + 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 { + 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, @@ -232,14 +259,14 @@ impl TextureManager2D { 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 texture = Self::create_and_upload_texture( + &self.device, + &self.queue, + texture_pool, + &tex_creation_desc, + ) + .map_err(TextureManager2DError::ZeroSize)?; + entry.insert(texture).clone() } }; @@ -277,16 +304,16 @@ impl TextureManager2D { &self.zeroed_texture_uint.0 } - fn create_and_upload_texture( + fn create_and_upload_texture( device: &wgpu::Device, queue: &wgpu::Queue, texture_pool: &mut GpuTexturePool, creation_desc: &Texture2DCreationDesc<'_>, - ) -> Result> { + ) -> Result { crate::profile_function!(); if creation_desc.width == 0 || creation_desc.height == 0 { - return Err(TextureManager2DError::ZeroSize(creation_desc.label.clone())); + return Err(ZeroSizeTextureError(creation_desc.label.clone())); } let size = wgpu::Extent3d { diff --git a/crates/re_viewer/src/gpu_bridge/mod.rs b/crates/re_viewer/src/gpu_bridge/mod.rs index 2f850be064a6..6e0d70ba43fe 100644 --- 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, TextureManager2DError}, + resource_managers::{ + GpuTexture2D, Texture2DCreationDesc, TextureManager2DError, ZeroSizeTextureError, + }, 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, Err>, ) -> Result> { - render_ctx.texture_manager_2d.get_or_create_with( + render_ctx.texture_manager_2d.get_or_try_create_with( texture_key, &mut render_ctx.gpu_resources.textures, try_create_texture_desc, @@ -68,11 +70,11 @@ pub fn get_or_create_texture<'a>( render_ctx: &mut RenderContext, texture_key: u64, create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>, -) -> Result> { +) -> Result { render_ctx.texture_manager_2d.get_or_create_with( texture_key, &mut render_ctx.gpu_resources.textures, - || Ok(create_texture_desc()), + create_texture_desc, ) } diff --git a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs index 6a255a9f50b8..6514169295fd 100644 --- a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs +++ b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs @@ -95,7 +95,7 @@ fn color_tensor_to_gpu( height, }) }) - .map_err(|e| anyhow::anyhow!("Failed to create texture for color tensor: {e:?}"))?; + .map_err(|err| anyhow::anyhow!("Failed to create texture for color tensor: {err}"))?; let texture_format = texture_handle.format(); @@ -183,12 +183,12 @@ fn class_id_tensor_to_gpu( height: colormap_height as u32, } }) - .map_err(|e| anyhow::anyhow!("Failed to create class_id_colormap: {e:?}"))?; + .map_err(|err| anyhow::anyhow!("Failed to create class_id_colormap: {err}"))?; let main_texture_handle = try_get_or_create_texture(render_ctx, hash(tensor.id()), || { general_texture_creation_desc_from_tensor(debug_name, tensor) }) - .map_err(|e| anyhow::anyhow!("Failed to create texture for class id tensor: {e:?}"))?; + .map_err(|err| anyhow::anyhow!("Failed to create texture for class id tensor: {err}"))?; Ok(ColormappedTexture { texture: main_texture_handle, @@ -218,7 +218,7 @@ fn depth_tensor_to_gpu( let texture = try_get_or_create_texture(render_ctx, hash(tensor.id()), || { general_texture_creation_desc_from_tensor(debug_name, tensor) }) - .map_err(|e| anyhow::anyhow!("Failed to create depth tensor texture: {e:?}"))?; + .map_err(|err| anyhow::anyhow!("Failed to create depth tensor texture: {err}"))?; Ok(ColormappedTexture { texture, diff --git a/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs b/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs index e3c572bb9a41..8a40a7957168 100644 --- a/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs +++ b/crates/re_viewer/src/ui/view_spatial/scene/scene_part/images.rs @@ -292,7 +292,7 @@ impl ImagesPart { let mut data_f32 = Vec::new(); ctx.render_ctx .texture_manager_2d - .get_or_create_with( + .get_or_try_create_with( texture_key, &mut ctx.render_ctx.gpu_resources.textures, || { @@ -321,7 +321,7 @@ impl ImagesPart { }) }, ) - .map_err(|e| format!("Failed to create depth cloud texture: {e:?}"))? + .map_err(|err| format!("Failed to create depth cloud texture: {err}"))? }; let depth_from_world_scale = *properties.depth_from_world_scale.get(); diff --git a/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs b/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs index 57c530ed9b76..ecb8793fc179 100644 --- a/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs +++ b/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs @@ -35,7 +35,7 @@ pub fn colormapped_texture( crate::profile_function!(); let range = - range(tensor_stats).map_err(|e| TextureManager2DError::DataCreationError(e.into()))?; + range(tensor_stats).map_err(|err| TextureManager2DError::DataCreationError(err.into()))?; let texture = upload_texture_slice_to_gpu(render_ctx, tensor, state.slice())?; let color_mapping = state.color_mapping(); diff --git a/crates/re_viewer/src/ui/view_tensor/ui.rs b/crates/re_viewer/src/ui/view_tensor/ui.rs index 72678f38bac5..374f2aee6836 100644 --- a/crates/re_viewer/src/ui/view_tensor/ui.rs +++ b/crates/re_viewer/src/ui/view_tensor/ui.rs @@ -322,7 +322,7 @@ fn paint_colormap_gradient( height, } }) - .map_err(|e| anyhow::anyhow!("Failed to create horizontal gradient texture: {e:?}"))?; + .map_err(|err| anyhow::anyhow!("Failed to create horizontal gradient texture: {err}"))?; let colormapped_texture = re_renderer::renderer::ColormappedTexture { texture: horizontal_gradient, From 449735e9355da0360643a0953c93d4ad72260ac1 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 20 Apr 2023 20:02:51 +0200 Subject: [PATCH 3/7] typo --- crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs index 6514169295fd..8258bc04c657 100644 --- a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs +++ b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs @@ -158,7 +158,7 @@ fn class_id_tensor_to_gpu( // We pack the colormap into a 2D texture so we don't go over the max texture size. // We only support u8 and u16 class ids, so 256^2 is the biggest texture we need. - // Note that if max is 0, we still need to generate a row to accomodate the 0 class id + // Note that if max is 0, we still need to generate a row to accommodate the 0 class id // (also zero sized textures are not allowed) let colormap_width = 256; let colormap_height = (max.at_least(1.0) as usize + colormap_width - 1) / colormap_width; From efb36ec30969e7ba05ce4eb488a015124e0e0e9b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 20 Apr 2023 20:04:01 +0200 Subject: [PATCH 4/7] Better naming of error --- .../re_renderer/src/resource_managers/mod.rs | 4 ++-- .../src/resource_managers/texture_manager.rs | 20 ++++++++++--------- crates/re_viewer/src/gpu_bridge/mod.rs | 4 ++-- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/crates/re_renderer/src/resource_managers/mod.rs b/crates/re_renderer/src/resource_managers/mod.rs index 26d3a949d82c..d9455d266071 100644 --- a/crates/re_renderer/src/resource_managers/mod.rs +++ b/crates/re_renderer/src/resource_managers/mod.rs @@ -11,8 +11,8 @@ pub use mesh_manager::{GpuMeshHandle, MeshManager}; mod texture_manager; pub use texture_manager::{ - GpuTexture2D, Texture2DCreationDesc, TextureManager2D, TextureManager2DError, - ZeroSizeTextureError, + GpuTexture2D, Texture2DCreationDesc, TextureCreationError, TextureManager2D, + TextureManager2DError, }; mod resource_manager; diff --git a/crates/re_renderer/src/resource_managers/texture_manager.rs b/crates/re_renderer/src/resource_managers/texture_manager.rs index a027d20e0200..63cb99af803f 100644 --- a/crates/re_renderer/src/resource_managers/texture_manager.rs +++ b/crates/re_renderer/src/resource_managers/texture_manager.rs @@ -96,19 +96,21 @@ impl<'a> Texture2DCreationDesc<'a> { } #[derive(thiserror::Error, Debug)] -#[error("Texture with debug label {0:?} has zero width or height!")] -pub struct ZeroSizeTextureError(DebugLabel); +pub enum TextureCreationError { + #[error("Texture with debug label {0:?} has zero width or height!")] + ZeroSize(DebugLabel), +} #[derive(thiserror::Error, Debug)] pub enum TextureManager2DError { #[error(transparent)] - ZeroSize(ZeroSizeTextureError), + ZeroSize(TextureCreationError), #[error(transparent)] DataCreationError(#[from] DataCreationError), } -impl From> for ZeroSizeTextureError { +impl From> for TextureCreationError { fn from(err: TextureManager2DError) -> Self { match err { TextureManager2DError::ZeroSize(zero_size) => zero_size, @@ -199,7 +201,7 @@ impl TextureManager2D { &mut self, texture_pool: &mut GpuTexturePool, creation_desc: &Texture2DCreationDesc<'_>, - ) -> Result { + ) -> Result { // 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. // @@ -226,7 +228,7 @@ impl TextureManager2D { key: u64, texture_pool: &mut GpuTexturePool, texture_desc: Texture2DCreationDesc<'_>, - ) -> Result { + ) -> Result { self.get_or_create_with(key, texture_pool, || texture_desc) } @@ -237,7 +239,7 @@ impl TextureManager2D { key: u64, texture_pool: &mut GpuTexturePool, create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>, - ) -> Result { + ) -> Result { self.get_or_try_create_with(key, texture_pool, || -> Result<_, never::Never> { Ok(create_texture_desc()) }) @@ -309,11 +311,11 @@ impl TextureManager2D { queue: &wgpu::Queue, texture_pool: &mut GpuTexturePool, creation_desc: &Texture2DCreationDesc<'_>, - ) -> Result { + ) -> Result { crate::profile_function!(); if creation_desc.width == 0 || creation_desc.height == 0 { - return Err(ZeroSizeTextureError(creation_desc.label.clone())); + return Err(TextureCreationError::ZeroSize(creation_desc.label.clone())); } let size = wgpu::Extent3d { diff --git a/crates/re_viewer/src/gpu_bridge/mod.rs b/crates/re_viewer/src/gpu_bridge/mod.rs index 6e0d70ba43fe..9cf35e1d4c01 100644 --- a/crates/re_viewer/src/gpu_bridge/mod.rs +++ b/crates/re_viewer/src/gpu_bridge/mod.rs @@ -10,7 +10,7 @@ use egui::mutex::Mutex; use re_renderer::{ renderer::{ColormappedTexture, RectangleOptions}, resource_managers::{ - GpuTexture2D, Texture2DCreationDesc, TextureManager2DError, ZeroSizeTextureError, + GpuTexture2D, Texture2DCreationDesc, TextureCreationError, TextureManager2DError, }, RenderContext, ViewBuilder, }; @@ -70,7 +70,7 @@ pub fn get_or_create_texture<'a>( render_ctx: &mut RenderContext, texture_key: u64, create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>, -) -> Result { +) -> Result { render_ctx.texture_manager_2d.get_or_create_with( texture_key, &mut render_ctx.gpu_resources.textures, From f06abad8957dba6f4157d132c0a6609d75811fa1 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 20 Apr 2023 20:07:28 +0200 Subject: [PATCH 5/7] Better docs and names --- .../src/resource_managers/texture_manager.rs | 12 +++++++----- .../src/ui/view_tensor/tensor_slice_to_gpu.rs | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/re_renderer/src/resource_managers/texture_manager.rs b/crates/re_renderer/src/resource_managers/texture_manager.rs index 63cb99af803f..326d370a3d5c 100644 --- a/crates/re_renderer/src/resource_managers/texture_manager.rs +++ b/crates/re_renderer/src/resource_managers/texture_manager.rs @@ -103,18 +103,20 @@ pub enum TextureCreationError { #[derive(thiserror::Error, Debug)] pub enum TextureManager2DError { + /// Something went wrong when creating the GPU texture. #[error(transparent)] - ZeroSize(TextureCreationError), + TextureCreation(TextureCreationError), + /// Something went wrong in a user-callback. #[error(transparent)] - DataCreationError(#[from] DataCreationError), + DataCreation(#[from] DataCreationError), } impl From> for TextureCreationError { fn from(err: TextureManager2DError) -> Self { match err { - TextureManager2DError::ZeroSize(zero_size) => zero_size, - TextureManager2DError::DataCreationError(never) => match never {}, + TextureManager2DError::TextureCreation(texture_creation) => texture_creation, + TextureManager2DError::DataCreation(never) => match never {}, } } } @@ -267,7 +269,7 @@ impl TextureManager2D { texture_pool, &tex_creation_desc, ) - .map_err(TextureManager2DError::ZeroSize)?; + .map_err(TextureManager2DError::TextureCreation)?; entry.insert(texture).clone() } }; diff --git a/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs b/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs index ecb8793fc179..887471a9e519 100644 --- a/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs +++ b/crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs @@ -35,7 +35,7 @@ pub fn colormapped_texture( crate::profile_function!(); let range = - range(tensor_stats).map_err(|err| TextureManager2DError::DataCreationError(err.into()))?; + range(tensor_stats).map_err(|err| TextureManager2DError::DataCreation(err.into()))?; let texture = upload_texture_slice_to_gpu(render_ctx, tensor, state.slice())?; let color_mapping = state.color_mapping(); From 75297a1b39d41359df32ec3349a15f1de1c325db Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 20 Apr 2023 20:17:24 +0200 Subject: [PATCH 6/7] Fix off-by-one error --- crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs index 8258bc04c657..26bb5dc0d4cd 100644 --- a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs +++ b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs @@ -3,7 +3,7 @@ use std::borrow::Cow; use bytemuck::{allocation::pod_collect_to_vec, cast_slice, Pod}; -use egui::{util::hash, NumExt}; +use egui::util::hash; use wgpu::TextureFormat; use re_log_types::component_types::{DecodedTensor, Tensor, TensorData}; @@ -152,16 +152,13 @@ fn class_id_tensor_to_gpu( .ok_or_else(|| anyhow::anyhow!("compressed_tensor!?"))?; anyhow::ensure!(0.0 <= min, "Negative class id"); - // create a lookup texture for the colors that's 256 wide, - // and as many rows as needed to fit all the classes. - anyhow::ensure!(max <= 65535.0, "Too many class ids"); + anyhow::ensure!(max <= 65535.0, "Too many class ids"); // we only support u8 and u16 tensors // We pack the colormap into a 2D texture so we don't go over the max texture size. // We only support u8 and u16 class ids, so 256^2 is the biggest texture we need. - // Note that if max is 0, we still need to generate a row to accommodate the 0 class id - // (also zero sized textures are not allowed) + let num_colors = (max + 1.0) as usize; let colormap_width = 256; - let colormap_height = (max.at_least(1.0) as usize + colormap_width - 1) / colormap_width; + let colormap_height = (num_colors + colormap_width - 1) / colormap_width; let colormap_texture_handle = get_or_create_texture(render_ctx, hash(annotations.row_id), || { From dc331b10eab27796dea2496fa08ee8c17febcff7 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 21 Apr 2023 11:28:55 +0200 Subject: [PATCH 7/7] some use of `context`, change which error is implicitly converted on texture manager2d --- .../src/resource_managers/texture_manager.rs | 11 ++++++----- crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs | 3 ++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/re_renderer/src/resource_managers/texture_manager.rs b/crates/re_renderer/src/resource_managers/texture_manager.rs index 326d370a3d5c..d81796e9dffe 100644 --- a/crates/re_renderer/src/resource_managers/texture_manager.rs +++ b/crates/re_renderer/src/resource_managers/texture_manager.rs @@ -95,6 +95,7 @@ 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!")] @@ -105,11 +106,11 @@ pub enum TextureCreationError { pub enum TextureManager2DError { /// Something went wrong when creating the GPU texture. #[error(transparent)] - TextureCreation(TextureCreationError), + TextureCreation(#[from] TextureCreationError), /// Something went wrong in a user-callback. #[error(transparent)] - DataCreation(#[from] DataCreationError), + DataCreation(DataCreationError), } impl From> for TextureCreationError { @@ -262,14 +263,14 @@ impl TextureManager2D { } std::collections::hash_map::Entry::Vacant(entry) => { // Run potentially expensive texture creation code: - let tex_creation_desc = try_create_texture_desc()?; + 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, - ) - .map_err(TextureManager2DError::TextureCreation)?; + )?; entry.insert(texture).clone() } }; diff --git a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs index 26bb5dc0d4cd..109a202d9a9a 100644 --- a/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs +++ b/crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs @@ -1,5 +1,6 @@ //! Upload [`Tensor`] to [`re_renderer`]. +use anyhow::Context; use std::borrow::Cow; use bytemuck::{allocation::pod_collect_to_vec, cast_slice, Pod}; @@ -180,7 +181,7 @@ fn class_id_tensor_to_gpu( height: colormap_height as u32, } }) - .map_err(|err| anyhow::anyhow!("Failed to create class_id_colormap: {err}"))?; + .context("Failed to create class_id_colormap.")?; let main_texture_handle = try_get_or_create_texture(render_ctx, hash(tensor.id()), || { general_texture_creation_desc_from_tensor(debug_name, tensor)