From a40de58c7c3ef3cf568476dea5b9e4549a04506a Mon Sep 17 00:00:00 2001 From: mTvare Date: Wed, 9 Jul 2025 12:15:41 +0530 Subject: [PATCH] Fix brush bounding boxes by making BrushCacheImpl's hash not shared between different instances (#2845) * fix: add cache to each layer * fix: warning * fix: tests * Clean up code --------- Co-authored-by: Keavon Chambers --- .../node_graph/document_node_definitions.rs | 2 +- .../messages/tool/tool_messages/brush_tool.rs | 6 +- node-graph/gbrush/src/brush.rs | 2 +- node-graph/gbrush/src/brush_cache.rs | 84 +++++++++---------- 4 files changed, 45 insertions(+), 49 deletions(-) diff --git a/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs b/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs index 594f8125..3c851020 100644 --- a/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs +++ b/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs @@ -946,7 +946,7 @@ fn static_nodes() -> Vec { inputs: vec![ NodeInput::value(TaggedValue::RasterData(RasterDataTable::default()), true), NodeInput::value(TaggedValue::BrushStrokes(Vec::new()), false), - NodeInput::value(TaggedValue::BrushCache(BrushCache::new_proto()), false), + NodeInput::value(TaggedValue::BrushCache(BrushCache::default()), false), ], ..Default::default() }, diff --git a/editor/src/messages/tool/tool_messages/brush_tool.rs b/editor/src/messages/tool/tool_messages/brush_tool.rs index 9bba3711..a625bdb9 100644 --- a/editor/src/messages/tool/tool_messages/brush_tool.rs +++ b/editor/src/messages/tool/tool_messages/brush_tool.rs @@ -1,6 +1,6 @@ use super::tool_prelude::*; use crate::consts::DEFAULT_BRUSH_SIZE; -use crate::messages::portfolio::document::graph_operation::transform_utils::{get_current_normalized_pivot, get_current_transform}; +use crate::messages::portfolio::document::graph_operation::transform_utils::get_current_transform; use crate::messages::portfolio::document::node_graph::document_node_definitions::resolve_document_node_type; use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier; use crate::messages::portfolio::document::utility_types::network_interface::FlowType; @@ -287,9 +287,7 @@ impl BrushToolData { } if *reference == Some("Transform".to_string()) { - let upstream = document.metadata().upstream_transform(node_id); - let pivot = DAffine2::from_translation(upstream.transform_point2(get_current_normalized_pivot(&node.inputs))); - self.transform = pivot * get_current_transform(&node.inputs) * pivot.inverse() * self.transform; + self.transform = get_current_transform(&node.inputs) * self.transform; } } diff --git a/node-graph/gbrush/src/brush.rs b/node-graph/gbrush/src/brush.rs index b2782adc..3a085c0a 100644 --- a/node-graph/gbrush/src/brush.rs +++ b/node-graph/gbrush/src/brush.rs @@ -403,7 +403,7 @@ mod test { blend_mode: BlendMode::Normal, }, }], - BrushCache::new_proto(), + BrushCache::default(), ) .await; assert_eq!(image.instance_ref_iter().next().unwrap().instance.width, 20); diff --git a/node-graph/gbrush/src/brush_cache.rs b/node-graph/gbrush/src/brush_cache.rs index 853a13ca..c5495534 100644 --- a/node-graph/gbrush/src/brush_cache.rs +++ b/node-graph/gbrush/src/brush_cache.rs @@ -6,11 +6,16 @@ use graphene_core::raster_types::CPU; use graphene_core::raster_types::Raster; use std::collections::HashMap; use std::hash::Hash; -use std::sync::Arc; -use std::sync::Mutex; +use std::hash::Hasher; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; -#[derive(Clone, Debug, PartialEq, DynAny, Default, serde::Serialize, serde::Deserialize)] +// TODO: This is a temporary hack, be sure to not reuse this when the brush is being rewritten. +static NEXT_BRUSH_CACHE_IMPL_ID: AtomicU64 = AtomicU64::new(0); + +#[derive(Clone, Debug, DynAny, serde::Serialize, serde::Deserialize)] struct BrushCacheImpl { + unique_id: u64, // The full previous input that was cached. prev_input: Vec, @@ -90,9 +95,29 @@ impl BrushCacheImpl { } } +impl Default for BrushCacheImpl { + fn default() -> Self { + Self { + unique_id: NEXT_BRUSH_CACHE_IMPL_ID.fetch_add(1, Ordering::SeqCst), + prev_input: Vec::new(), + background: Default::default(), + blended_image: Default::default(), + last_stroke_texture: Default::default(), + brush_texture_cache: HashMap::new(), + } + } +} + +impl PartialEq for BrushCacheImpl { + fn eq(&self, other: &Self) -> bool { + self.unique_id == other.unique_id + } +} + impl Hash for BrushCacheImpl { - // Zero hash. - fn hash(&self, _state: &mut H) {} + fn hash(&self, state: &mut H) { + self.unique_id.hash(state); + } } #[derive(Clone, Debug, Default)] @@ -103,46 +128,26 @@ pub struct BrushPlan { pub first_stroke_point_skip: usize, } -#[derive(Debug, DynAny, serde::Serialize, serde::Deserialize)] -pub struct BrushCache { - inner: Arc>, - proto: bool, -} - -impl Default for BrushCache { - fn default() -> Self { - Self::new_proto() - } -} +#[derive(Debug, Default, DynAny, serde::Serialize, serde::Deserialize)] +pub struct BrushCache(Arc>); // A bit of a cursed implementation to work around the current node system. // The original object is a 'prototype' that when cloned gives you a independent // new object. Any further clones however are all the same underlying cache object. impl Clone for BrushCache { fn clone(&self) -> Self { - if self.proto { - let inner_val = self.inner.lock().unwrap(); - Self { - inner: Arc::new(Mutex::new(inner_val.clone())), - proto: false, - } - } else { - Self { - inner: Arc::clone(&self.inner), - proto: false, - } - } + Self(Arc::new(Mutex::new(self.0.lock().unwrap().clone()))) } } impl PartialEq for BrushCache { fn eq(&self, other: &Self) -> bool { - if Arc::ptr_eq(&self.inner, &other.inner) { + if Arc::ptr_eq(&self.0, &other.0) { return true; } - let s = self.inner.lock().unwrap(); - let o = other.inner.lock().unwrap(); + let s = self.0.lock().unwrap(); + let o = other.0.lock().unwrap(); *s == *o } @@ -150,35 +155,28 @@ impl PartialEq for BrushCache { impl Hash for BrushCache { fn hash(&self, state: &mut H) { - self.inner.lock().unwrap().hash(state); + self.0.lock().unwrap().hash(state); } } impl BrushCache { - pub fn new_proto() -> Self { - Self { - inner: Default::default(), - proto: true, - } - } - pub fn compute_brush_plan(&self, background: Instance>, input: &[BrushStroke]) -> BrushPlan { - let mut inner = self.inner.lock().unwrap(); + let mut inner = self.0.lock().unwrap(); inner.compute_brush_plan(background, input) } pub fn cache_results(&self, input: Vec, blended_image: Instance>, last_stroke_texture: Instance>) { - let mut inner = self.inner.lock().unwrap(); + let mut inner = self.0.lock().unwrap(); inner.cache_results(input, blended_image, last_stroke_texture) } pub fn get_cached_brush(&self, style: &BrushStyle) -> Option> { - let inner = self.inner.lock().unwrap(); + let inner = self.0.lock().unwrap(); inner.brush_texture_cache.get(style).cloned() } pub fn store_brush(&self, style: BrushStyle, brush: Raster) { - let mut inner = self.inner.lock().unwrap(); + let mut inner = self.0.lock().unwrap(); inner.brush_texture_cache.insert(style, brush); } }