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

Use less mut when using RenderContext #2312

Merged
merged 4 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion crates/re_renderer/examples/2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl framework::Example for Render2D {
let rerun_logo_texture = re_ctx
.texture_manager_2d
.create(
&mut re_ctx.gpu_resources.textures,
&re_ctx.gpu_resources.textures,
&Texture2DCreationDesc {
label: "rerun logo".into(),
data: image_data.into(),
Expand Down
4 changes: 2 additions & 2 deletions crates/re_renderer/examples/depth_cloud.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ impl DepthTexture {
.texture_manager_2d
.get_or_create(
hash(&label),
&mut re_ctx.gpu_resources.textures,
&re_ctx.gpu_resources.textures,
Texture2DCreationDesc {
label: label.into(),
data: bytemuck::cast_slice(&data).into(),
Expand Down Expand Up @@ -458,7 +458,7 @@ impl AlbedoTexture {
.texture_manager_2d
.get_or_create(
hash(&label),
&mut re_ctx.gpu_resources.textures,
&re_ctx.gpu_resources.textures,
Texture2DCreationDesc {
label: label.into(),
data: bytemuck::cast_slice(&rgba8).into(),
Expand Down
2 changes: 1 addition & 1 deletion crates/re_renderer/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ impl RenderContext {
&mut resolver,
)));
let texture_manager_2d =
TextureManager2D::new(device.clone(), queue.clone(), &mut gpu_resources.textures);
TextureManager2D::new(device.clone(), queue.clone(), &gpu_resources.textures);

let active_frame = ActiveFrameContext {
before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&device)),
Expand Down
15 changes: 5 additions & 10 deletions crates/re_renderer/src/importer/gltf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub fn load_gltf_from_buffer(
mesh_name: &str,
buffer: &[u8],
lifetime: ResourceLifeTime,
ctx: &mut RenderContext,
ctx: &RenderContext,
) -> anyhow::Result<Vec<MeshInstance>> {
re_tracing::profile_function!();

Expand Down Expand Up @@ -77,7 +77,7 @@ pub fn load_gltf_from_buffer(
images_as_textures.push(
match ctx
.texture_manager_2d
.create(&mut ctx.gpu_resources.textures, &texture)
.create(&ctx.gpu_resources.textures, &texture)
{
Ok(texture) => texture,
Err(err) => {
Expand All @@ -92,13 +92,8 @@ pub fn load_gltf_from_buffer(
for ref mesh in doc.meshes() {
re_tracing::profile_scope!("mesh");

let re_mesh = import_mesh(
mesh,
&buffers,
&images_as_textures,
&mut ctx.texture_manager_2d,
)
.with_context(|| format!("mesh {} (name {:?})", mesh.index(), mesh.name()))?;
let re_mesh = import_mesh(mesh, &buffers, &images_as_textures, &ctx.texture_manager_2d)
.with_context(|| format!("mesh {} (name {:?})", mesh.index(), mesh.name()))?;
meshes.insert(
mesh.index(),
(
Expand Down Expand Up @@ -143,7 +138,7 @@ fn import_mesh(
mesh: &gltf::Mesh<'_>,
buffers: &[gltf::buffer::Data],
gpu_image_handles: &[GpuTexture2D],
texture_manager: &mut TextureManager2D, //imported_materials: HashMap<usize, Material>,
texture_manager: &TextureManager2D, //imported_materials: HashMap<usize, Material>,
) -> anyhow::Result<Mesh> {
re_tracing::profile_function!();

Expand Down
55 changes: 35 additions & 20 deletions crates/re_renderer/src/resource_managers/texture_manager.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::sync::Arc;

use ahash::{HashMap, HashSet};
use parking_lot::Mutex;

use crate::{
wgpu_resources::{GpuTexture, GpuTexturePool, TextureDesc},
Expand Down Expand Up @@ -128,6 +129,8 @@ impl From<TextureManager2DError<never::Never>> for TextureCreationError {
/// More complex textures types are typically handled within renderer which utilize the texture pool directly.
/// This manager in contrast, deals with user provided texture data!
/// We might revisit this later and make this texture manager more general purpose.
///
/// Has intertior mutability.
pub struct TextureManager2D {
// Long lived/short lived doesn't make sense for textures since we don't yet know a way to
// optimize for short lived textures as we do with buffer data.
Expand All @@ -142,6 +145,12 @@ pub struct TextureManager2D {
device: Arc<wgpu::Device>,
queue: Arc<wgpu::Queue>,

/// The mutable part of the manager.
inner: Mutex<Inner>,
}

#[derive(Default)]
struct Inner {
// Cache the texture using a user-provided u64 id. This is expected
// to be derived from the `TensorId` which isn't available here for
// dependency reasons.
Expand All @@ -153,11 +162,20 @@ pub struct TextureManager2D {
accessed_textures: HashSet<u64>,
}

impl Inner {
fn begin_frame(&mut self, _frame_index: u64) {
// Drop any textures that weren't accessed in the last frame
self.texture_cache
.retain(|k, _| self.accessed_textures.contains(k));
self.accessed_textures.clear();
}
}

impl TextureManager2D {
pub(crate) fn new(
device: Arc<wgpu::Device>,
queue: Arc<wgpu::Queue>,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
) -> Self {
re_tracing::profile_function!();

Expand Down Expand Up @@ -192,16 +210,15 @@ impl TextureManager2D {
zeroed_texture_uint,
device,
queue,
texture_cache: Default::default(),
accessed_textures: Default::default(),
inner: Default::default(),
}
}

/// Creates a new 2D texture resource and schedules data upload to the GPU.
/// TODO(jleibs): All usages of this should be be replaced with `get_or_create`, which is strictly preferable
pub fn create(
&mut self,
texture_pool: &mut GpuTexturePool,
&self,
texture_pool: &GpuTexturePool,
creation_desc: &Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureCreationError> {
// TODO(andreas): Disabled the warning as we're moving towards using this texture manager for user-logged images.
Expand All @@ -226,9 +243,9 @@ impl TextureManager2D {
/// 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(
&mut self,
&self,
key: u64,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
texture_desc: Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureCreationError> {
self.get_or_create_with(key, texture_pool, || texture_desc)
Expand All @@ -237,9 +254,9 @@ impl TextureManager2D {
/// 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>(
&mut self,
&self,
key: u64,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>,
) -> Result<GpuTexture2D, TextureCreationError> {
self.get_or_try_create_with(key, texture_pool, || -> Result<_, never::Never> {
Expand All @@ -251,12 +268,13 @@ impl TextureManager2D {
/// 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,
&self,
key: u64,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
try_create_texture_desc: impl FnOnce() -> Result<Texture2DCreationDesc<'a>, Err>,
) -> Result<GpuTexture2D, TextureManager2DError<Err>> {
let texture_handle = match self.texture_cache.entry(key) {
let mut inner = self.inner.lock();
Copy link
Member

Choose a reason for hiding this comment

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

This could be improved to be a read/write lock where we take the write lock only if a new texture actually needs to be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

if it needs to be added or inserted in accessed_textures… it gets a bit hairy, so I opted to keep it simple instead

let texture_handle = match inner.texture_cache.entry(key) {
std::collections::hash_map::Entry::Occupied(texture_handle) => {
texture_handle.get().clone() // already inserted
}
Expand All @@ -274,7 +292,7 @@ impl TextureManager2D {
}
};

self.accessed_textures.insert(key);
inner.accessed_textures.insert(key);
Ok(texture_handle)
}

Expand Down Expand Up @@ -311,7 +329,7 @@ impl TextureManager2D {
fn create_and_upload_texture(
device: &wgpu::Device,
queue: &wgpu::Queue,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
creation_desc: &Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureCreationError> {
re_tracing::profile_function!();
Expand Down Expand Up @@ -376,16 +394,13 @@ impl TextureManager2D {
Ok(GpuTexture2D(texture))
}

pub(crate) fn begin_frame(&mut self, _frame_index: u64) {
// Drop any textures that weren't accessed in the last frame
self.texture_cache
.retain(|k, _| self.accessed_textures.contains(k));
self.accessed_textures.clear();
pub(crate) fn begin_frame(&self, _frame_index: u64) {
self.inner.lock().begin_frame(_frame_index);
}
}

fn create_zero_texture(
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
device: &Arc<wgpu::Device>,
format: wgpu::TextureFormat,
) -> GpuTexture2D {
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view_spatial/src/mesh_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ impl MeshCache {
&mut self,
name: &str,
mesh: &Mesh3D,
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> Option<Arc<LoadedMesh>> {
re_tracing::profile_function!();

Expand Down
12 changes: 4 additions & 8 deletions crates/re_space_view_spatial/src/mesh_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ pub struct LoadedMesh {
}

impl LoadedMesh {
pub fn load(
name: String,
mesh: &Mesh3D,
render_ctx: &mut RenderContext,
) -> anyhow::Result<Self> {
pub fn load(name: String, mesh: &Mesh3D, render_ctx: &RenderContext) -> anyhow::Result<Self> {
// TODO(emilk): load CpuMesh in background thread.
match mesh {
// Mesh from some file format. File passed in bytes.
Expand All @@ -32,7 +28,7 @@ impl LoadedMesh {
name: String,
format: MeshFormat,
bytes: &[u8],
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> anyhow::Result<Self> {
re_tracing::profile_function!();

Expand Down Expand Up @@ -60,7 +56,7 @@ impl LoadedMesh {
fn load_encoded_mesh(
name: String,
encoded_mesh: &EncodedMesh3D,
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> anyhow::Result<Self> {
re_tracing::profile_function!();
let EncodedMesh3D {
Expand Down Expand Up @@ -88,7 +84,7 @@ impl LoadedMesh {
fn load_raw_mesh(
name: String,
raw_mesh: &RawMesh3D,
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> anyhow::Result<Self> {
re_tracing::profile_function!();

Expand Down
54 changes: 25 additions & 29 deletions crates/re_space_view_spatial/src/scene/parts/images.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,35 +323,31 @@ impl ImagesPart {
let mut data_f32 = Vec::new();
ctx.render_ctx
.texture_manager_2d
.get_or_try_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 _,
})
},
)
.get_or_try_create_with(texture_key, &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(|err| format!("Failed to create depth cloud texture: {err}"))?
};

Expand Down
8 changes: 4 additions & 4 deletions crates/re_viewer_context/src/gpu_bridge/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,25 +57,25 @@ pub fn viewport_resolution_in_pixels(clip_rect: egui::Rect, pixels_from_point: f
}

pub fn try_get_or_create_texture<'a, Err: std::fmt::Display>(
render_ctx: &mut RenderContext,
render_ctx: &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_try_create_with(
texture_key,
&mut render_ctx.gpu_resources.textures,
&render_ctx.gpu_resources.textures,
try_create_texture_desc,
)
}

pub fn get_or_create_texture<'a>(
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
texture_key: u64,
create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>,
) -> Result<GpuTexture2D, TextureCreationError> {
render_ctx.texture_manager_2d.get_or_create_with(
texture_key,
&mut render_ctx.gpu_resources.textures,
&render_ctx.gpu_resources.textures,
create_texture_desc,
)
}
Expand Down
Loading