From d58edc41272c73617899e20f8c604c21e09e9a39 Mon Sep 17 00:00:00 2001 From: Schmarni Date: Wed, 29 Jan 2025 21:46:56 +0100 Subject: [PATCH 1/2] fix: incorrect material batching Signed-off-by: Schmarni --- src/nodes/drawable/model.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/nodes/drawable/model.rs b/src/nodes/drawable/model.rs index 3e98179..02a7cc4 100644 --- a/src/nodes/drawable/model.rs +++ b/src/nodes/drawable/model.rs @@ -26,7 +26,8 @@ impl Hash for MaterialWrapper { fn hash(&self, state: &mut H) { self.0.get_shader().0.as_ptr().hash(state); for param in self.0.get_all_param_info() { - param.to_string().hash(state) + param.name.hash(state); + param.to_string().hash(state); } self.0.get_chain().map(MaterialWrapper).hash(state) } @@ -59,7 +60,7 @@ unsafe impl Send for MaterialWrapper {} unsafe impl Sync for MaterialWrapper {} #[derive(Default)] -struct MaterialRegistry(Mutex>); +struct MaterialRegistry(Mutex>>); impl MaterialRegistry { fn add_or_get(&self, material: Arc) -> Arc { let mut lock = self.0.lock(); @@ -70,13 +71,11 @@ impl MaterialRegistry { hasher.finish() }; - if let Some(id) = lock.get(&hash) { - if let Ok(existing) = Material::find(id) { - return Arc::new(MaterialWrapper(existing)); - } + if let Some(mat) = lock.get(&hash) { + return mat.clone(); } - lock.insert(hash, material.0.get_id().to_string()); + lock.insert(hash, material.clone()); material } } -- 2.49.1 From 8a0563b9de26d883786eb1fc07d385eb8c54c213 Mon Sep 17 00:00:00 2001 From: Schmarni Date: Wed, 29 Jan 2025 22:15:20 +0100 Subject: [PATCH 2/2] fix: prevent memory leak with batched materials Signed-off-by: Schmarni --- src/nodes/drawable/model.rs | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/nodes/drawable/model.rs b/src/nodes/drawable/model.rs index 02a7cc4..0c7469d 100644 --- a/src/nodes/drawable/model.rs +++ b/src/nodes/drawable/model.rs @@ -22,6 +22,12 @@ use stereokit_rust::sk::MainThreadToken; use stereokit_rust::{material::Material, model::Model as SKModel, tex::Tex, util::Color128}; pub struct MaterialWrapper(pub Material); +impl Drop for MaterialWrapper { + fn drop(&mut self) { + MATERIAL_REGISTRY.remove(self); + } +} + impl Hash for MaterialWrapper { fn hash(&self, state: &mut H) { self.0.get_shader().0.as_ptr().hash(state); @@ -60,10 +66,9 @@ unsafe impl Send for MaterialWrapper {} unsafe impl Sync for MaterialWrapper {} #[derive(Default)] -struct MaterialRegistry(Mutex>>); +struct MaterialRegistry(Mutex>>); impl MaterialRegistry { fn add_or_get(&self, material: Arc) -> Arc { - let mut lock = self.0.lock(); let hash = { use std::hash::{Hash, Hasher}; let mut hasher = std::collections::hash_map::DefaultHasher::new(); @@ -71,13 +76,26 @@ impl MaterialRegistry { hasher.finish() }; + let mut lock = self.0.lock(); if let Some(mat) = lock.get(&hash) { - return mat.clone(); + if let Some(mat) = mat.upgrade() { + return mat; + } } - lock.insert(hash, material.clone()); + lock.insert(hash, Arc::downgrade(&material)); material } + fn remove(&self, material: &MaterialWrapper) { + let hash = { + use std::hash::{Hash, Hasher}; + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + material.hash(&mut hasher); + hasher.finish() + }; + let mut lock = self.0.lock(); + lock.remove(&hash); + } } static MATERIAL_REGISTRY: Lazy = Lazy::new(MaterialRegistry::default); @@ -131,6 +149,7 @@ pub struct ModelPart { path: String, space: Arc, model: Weak, + material: Mutex>>, pending_material_parameters: Mutex>, pending_material_replacement: Mutex>>, aliases: AliasList, @@ -204,6 +223,7 @@ impl ModelPart { pending_material_parameters: Mutex::new(FxHashMap::default()), pending_material_replacement: Mutex::new(None), aliases: AliasList::default(), + material: Mutex::new(part.get_material().map(MaterialWrapper).map(Arc::new)), }); node.add_aspect_raw(model_part.clone()); parts.push(model_part.clone()); @@ -230,7 +250,10 @@ impl ModelPart { }; let shared_material = MATERIAL_REGISTRY.add_or_get(Arc::new(MaterialWrapper(replacement.copy()))); + + let mut lock = self.material.lock(); part.material(&shared_material.0); + lock.replace(shared_material); } fn update(&self) { @@ -257,7 +280,9 @@ impl ModelPart { }; if let Some(material_replacement) = self.pending_material_replacement.lock().take() { + let mut lock = self.material.lock(); part.material(&material_replacement.0); + lock.replace(material_replacement); } 'mat_params: { @@ -273,7 +298,9 @@ impl ModelPart { let shared_material = MATERIAL_REGISTRY.add_or_get(Arc::new(MaterialWrapper(new_material))); + let mut lock = self.material.lock(); part.material(&shared_material.0); + lock.replace(shared_material); } } } -- 2.49.1