Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/re_renderer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ glam = { workspace = true, features = ["bytemuck"] }
half = { workspace = true, features = ["bytemuck"] }
itertools = { workspace = true }
macaw.workspace = true
never = '0.1'
Wumpf marked this conversation as resolved.
Show resolved Hide resolved
ordered-float = "3.2"
parking_lot.workspace = true
slotmap = "1.0.6"
Expand Down
2 changes: 1 addition & 1 deletion crates/re_renderer/src/importer/gltf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
},
Expand Down
3 changes: 2 additions & 1 deletion crates/re_renderer/src/resource_managers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ pub use mesh_manager::{GpuMeshHandle, MeshManager};

mod texture_manager;
pub use texture_manager::{
GpuTexture2D, Texture2DCreationDesc, TextureManager2D, TextureManager2DError,
GpuTexture2D, Texture2DCreationDesc, TextureCreationError, TextureManager2D,
TextureManager2DError,
};

mod resource_manager;
Expand Down
67 changes: 49 additions & 18 deletions crates/re_renderer/src/resource_managers/texture_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,29 @@ impl<'a> Texture2DCreationDesc<'a> {
}

#[derive(thiserror::Error, Debug)]
pub enum TextureManager2DError<DataCreationError> {
pub enum TextureCreationError {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea to split it out. we should consider moving this down to the texture pool later :)

#[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)]
DataCreationError(#[from] DataCreationError),
TextureCreation(TextureCreationError),

/// Something went wrong in a user-callback.
#[error(transparent)]
DataCreation(#[from] 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.
Expand Down Expand Up @@ -144,7 +161,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,
Expand Down Expand Up @@ -186,7 +203,7 @@ impl TextureManager2D {
&mut self,
texture_pool: &mut GpuTexturePool,
creation_desc: &Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureManager2DError<()>> {
) -> 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.
//
Expand All @@ -213,13 +230,27 @@ impl TextureManager2D {
key: u64,
texture_pool: &mut GpuTexturePool,
texture_desc: Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureManager2DError<()>> {
self.get_or_create_with(key, texture_pool, || -> Result<_, ()> { Ok(texture_desc) })
) -> 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,
Expand All @@ -232,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()?;
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::TextureCreation)?;
entry.insert(texture).clone()
}
};

Expand Down Expand Up @@ -277,16 +308,16 @@ impl TextureManager2D {
&self.zeroed_texture_uint.0
}

fn create_and_upload_texture<Err>(
fn create_and_upload_texture(
device: &wgpu::Device,
queue: &wgpu::Queue,
texture_pool: &mut GpuTexturePool,
creation_desc: &Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureManager2DError<Err>> {
) -> Result<GpuTexture2D, TextureCreationError> {
crate::profile_function!();

if creation_desc.width == 0 || creation_desc.height == 0 {
return Err(TextureManager2DError::ZeroSize(creation_desc.label.clone()));
return Err(TextureCreationError::ZeroSize(creation_desc.label.clone()));
}

let size = wgpu::Extent3d {
Expand Down
12 changes: 7 additions & 5 deletions crates/re_viewer/src/gpu_bridge/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ use egui::mutex::Mutex;

use re_renderer::{
renderer::{ColormappedTexture, RectangleOptions},
resource_managers::{GpuTexture2D, Texture2DCreationDesc, TextureManager2DError},
resource_managers::{
GpuTexture2D, Texture2DCreationDesc, TextureCreationError, TextureManager2DError,
},
RenderContext, ViewBuilder,
};

Expand Down Expand Up @@ -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, TextureManager2DError<Err>> {
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,
Expand All @@ -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<GpuTexture2D, TextureManager2DError<()>> {
) -> Result<GpuTexture2D, TextureCreationError> {
render_ctx.texture_manager_2d.get_or_create_with(
texture_key,
&mut render_ctx.gpu_resources.textures,
|| Ok(create_texture_desc()),
create_texture_desc,
)
}

Expand Down
19 changes: 8 additions & 11 deletions crates/re_viewer/src/gpu_bridge/tensor_to_gpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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}"))?;
Copy link
Member

@emilk emilk Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be using .context() instead, here and elsewhere


let texture_format = texture_handle.format();

Expand Down Expand Up @@ -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 accomodate 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), || {
Expand All @@ -183,12 +180,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,
Expand Down Expand Up @@ -218,7 +215,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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
|| {
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion crates/re_viewer/src/ui/view_tensor/tensor_slice_to_gpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::DataCreation(err.into()))?;
let texture = upload_texture_slice_to_gpu(render_ctx, tensor, state.slice())?;

let color_mapping = state.color_mapping();
Expand Down
2 changes: 1 addition & 1 deletion crates/re_viewer/src/ui/view_tensor/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down