From e1bd897b8ea191ae3428943c824d5b3e88cba406 Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Wed, 29 Apr 2026 03:46:59 -0700 Subject: [PATCH 1/2] Fix hashing not being based on all attributes of Table --- node-graph/libraries/core-types/src/table.rs | 64 +++++++++++++------- node-graph/nodes/graphic/src/graphic.rs | 6 +- 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/node-graph/libraries/core-types/src/table.rs b/node-graph/libraries/core-types/src/table.rs index 8bca5d5689..c016457b6f 100644 --- a/node-graph/libraries/core-types/src/table.rs +++ b/node-graph/libraries/core-types/src/table.rs @@ -3,6 +3,7 @@ use crate::math::quad::Quad; use crate::transform::ApplyTransform; use dyn_any::{StaticType, StaticTypeSized}; use glam::DAffine2; +use graphene_hash::CacheHash; use std::fmt::Debug; // ===================================================================== @@ -83,7 +84,7 @@ trait AttributeValue: std::any::Any + Send + Sync { fn into_column(self: Box, preceding_defaults: usize) -> Box; } -impl AttributeValue for T { +impl AttributeValue for T { /// Clones this value into a new boxed trait object. fn clone_box(&self) -> Box { Box::new(self.clone()) @@ -164,6 +165,21 @@ trait AttributeColumn: std::any::Any + Send + Sync { /// Drains all values out of this column into a Vec of scalar attribute values. fn drain(self: Box) -> Vec>; + + /// Hashes every value in this column into the given hasher (object-safe wrapper around `CacheHash`). + fn cache_hash_dyn(&self, state: &mut dyn core::hash::Hasher); +} + +/// Adapts a `&mut dyn Hasher` so generic `CacheHash::cache_hash` calls (which require `H: Sized + Hasher`) can +/// drive a trait-object hasher. Forwards both `Hasher` methods to the inner `dyn Hasher`. +struct DynHasher<'a>(&'a mut dyn core::hash::Hasher); +impl core::hash::Hasher for DynHasher<'_> { + fn finish(&self) -> u64 { + self.0.finish() + } + fn write(&mut self, bytes: &[u8]) { + self.0.write(bytes) + } } impl Clone for Box { @@ -179,7 +195,7 @@ impl Clone for Box { /// Wraps a Vec for column-major attribute storage in a Table. struct Column(Vec); -impl AttributeColumn for Column { +impl AttributeColumn for Column { /// Clones this column into a new boxed trait object. fn clone_box(&self) -> Box { Box::new(Column(self.0.clone())) @@ -250,6 +266,13 @@ impl AttributeColumn for Col fn drain(self: Box) -> Vec> { self.0.into_iter().map(|v| Box::new(v) as Box).collect() } + + fn cache_hash_dyn(&self, state: &mut dyn core::hash::Hasher) { + let mut wrapped = DynHasher(state); + for value in &self.0 { + value.cache_hash(&mut wrapped); + } + } } // =============== @@ -278,7 +301,7 @@ impl AttributeValues { } /// Inserts an attribute with the given key and value, replacing any existing entry with the same key. - pub fn insert(&mut self, key: impl Into, value: T) { + pub fn insert(&mut self, key: impl Into, value: T) { let key = key.into(); for (existing_key, existing_value) in &mut self.0 { @@ -309,7 +332,7 @@ impl AttributeValues { } /// Gets a mutable reference to the value, inserting a default if it doesn't exist or has the wrong type. - pub fn get_or_insert_default_mut(&mut self, key: &str) -> &mut T { + pub fn get_or_insert_default_mut(&mut self, key: &str) -> &mut T { let needs_insert = match self.0.iter().position(|(existing_key, _)| existing_key == key) { Some(index) => { if (*self.0[index].1).as_any().downcast_ref::().is_some() { @@ -448,7 +471,7 @@ impl AttributeColumns { /// Finds or creates a column for the given key and type, returning its position. /// If a column with the key exists but has the wrong type, it is removed and replaced with a new column of the correct type, padded with defaults. /// A newly created column is filled with `T::default()` for all existing rows. - fn find_or_create_column(&mut self, key: &str) -> usize { + fn find_or_create_column(&mut self, key: &str) -> usize { match self.columns.iter().position(|(k, _)| k == key) { Some(position) => { if (*self.columns[position].1).as_any().downcast_ref::>().is_some() { @@ -467,7 +490,7 @@ impl AttributeColumns { } /// Gets a mutable reference to the value at the given index, creating the column if it doesn't exist or has the wrong type. - fn get_or_insert_default_value(&mut self, key: &str, index: usize) -> &mut T { + fn get_or_insert_default_value(&mut self, key: &str, index: usize) -> &mut T { let column_position = self.find_or_create_column::(key); let column = (*self.columns[column_position].1).as_any_mut().downcast_mut::>().unwrap(); &mut column.0[index] @@ -475,7 +498,7 @@ impl AttributeColumns { /// Sets the value at the given index in the column for the given key. /// Creates the column with defaults if it doesn't exist. - fn set_value(&mut self, key: impl Into, index: usize, value: T) { + fn set_value(&mut self, key: impl Into, index: usize, value: T) { let key = key.into(); let column_position = self.find_or_create_column::(&key); let column = (*self.columns[column_position].1).as_any_mut().downcast_mut::>().unwrap(); @@ -527,7 +550,7 @@ impl AttributeColumns { } /// Returns a mutable typed slice of the column for the given key, creating a new column filled with defaults if it doesn't exist. - fn get_or_create_column_slice_mut(&mut self, key: &str) -> &mut [T] { + fn get_or_create_column_slice_mut(&mut self, key: &str) -> &mut [T] { let position = self.find_or_create_column::(key); let column = (*self.columns[position].1).as_any_mut().downcast_mut::>().unwrap(); &mut column.0 @@ -667,7 +690,7 @@ impl Table { } /// Returns a mutable iterator over a typed attribute column, creating the column with default values if it doesn't exist or has the wrong type. - pub fn iter_attribute_values_mut_or_default(&mut self, key: &str) -> std::slice::IterMut<'_, U> { + pub fn iter_attribute_values_mut_or_default(&mut self, key: &str) -> std::slice::IterMut<'_, U> { self.attributes.get_or_create_column_slice_mut::(key).iter_mut() } @@ -705,13 +728,13 @@ impl Table { } /// Sets the attribute value at the given row index and key, creating the column with defaults if it doesn't exist. - pub fn set_attribute(&mut self, key: impl Into, index: usize, value: U) { + pub fn set_attribute(&mut self, key: impl Into, index: usize, value: U) { self.attributes.set_value(key, index, value); } /// Runs the given closure on a mutable reference to the attribute value at the given row index, /// creating the column with defaults if it doesn't exist, and returns the closure's result. - pub fn with_attribute_mut_or_default R>(&mut self, key: &str, index: usize, f: F) -> R { + pub fn with_attribute_mut_or_default R>(&mut self, key: &str, index: usize, f: F) -> R { f(self.attributes.get_or_insert_default_value::(key, index)) } @@ -732,7 +755,7 @@ impl Table { /// Returns disjoint mutable references to the element slice and a typed attribute column slice, /// creating the column with defaults if it doesn't exist. This enables simultaneous mutable /// access to elements and a single attribute column without borrowing conflicts. - pub fn element_and_attribute_slices_mut(&mut self, key: &str) -> (&mut [T], &mut [U]) { + pub fn element_and_attribute_slices_mut(&mut self, key: &str) -> (&mut [T], &mut [U]) { let Self { element, attributes } = self; let column_position = attributes.find_or_create_column::(key); let column = (*attributes.columns[column_position].1).as_any_mut().downcast_mut::>().unwrap(); @@ -839,16 +862,15 @@ impl Default for Table { } } -impl graphene_hash::CacheHash for Table { +impl CacheHash for Table { fn cache_hash(&self, state: &mut H) { for element in self.iter_element_values() { element.cache_hash(state); } - for transform in self.iter_attribute_values_or_default::(ATTR_TRANSFORM) { - graphene_hash::CacheHash::cache_hash(&transform, state); - } - for alpha_blending in self.iter_attribute_values_or_default::(ATTR_ALPHA_BLENDING) { - alpha_blending.cache_hash(state); + + for (key, column) in &self.attributes.columns { + std::hash::Hash::hash(key.as_str(), state); + column.cache_hash_dyn(state); } } } @@ -1016,17 +1038,17 @@ impl TableRow { } /// Returns a mutable reference to the attribute value for the given key, inserting a default value if absent or of a different type. - pub fn attribute_mut_or_insert_default(&mut self, key: &str) -> &mut U { + pub fn attribute_mut_or_insert_default(&mut self, key: &str) -> &mut U { self.attributes.get_or_insert_default_mut(key) } /// Sets the attribute value for the given key, replacing any existing entry with the same key. - pub fn set_attribute(&mut self, key: impl Into, value: U) { + pub fn set_attribute(&mut self, key: impl Into, value: U) { self.attributes.insert(key, value); } /// Sets the attribute value for the given key and returns the row, enabling builder-style chaining. - pub fn with_attribute(mut self, key: impl Into, value: U) -> Self { + pub fn with_attribute(mut self, key: impl Into, value: U) -> Self { self.set_attribute(key, value); self } diff --git a/node-graph/nodes/graphic/src/graphic.rs b/node-graph/nodes/graphic/src/graphic.rs index ad4d2fa493..5fa83a4cbf 100644 --- a/node-graph/nodes/graphic/src/graphic.rs +++ b/node-graph/nodes/graphic/src/graphic.rs @@ -2,7 +2,7 @@ use core_types::bounds::{BoundingBox, RenderBoundingBox}; use core_types::registry::types::{Angle, SignedInteger}; use core_types::table::{Table, TableRow}; use core_types::uuid::NodeId; -use core_types::{ATTR_EDITOR_LAYER_PATH, ATTR_TRANSFORM, AnyHash, CloneVarArgs, Color, Context, Ctx, ExtractAll, OwnedContextImpl}; +use core_types::{ATTR_EDITOR_LAYER_PATH, ATTR_TRANSFORM, AnyHash, CacheHash, CloneVarArgs, Color, Context, Ctx, ExtractAll, OwnedContextImpl}; use glam::{DAffine2, DVec2}; use graphic_types::Vector; use graphic_types::graphic::{Graphic, IntoGraphicTable}; @@ -107,7 +107,7 @@ pub fn extract_element( } #[node_macro::node(category("General"))] -async fn map( +async fn map( ctx: impl Ctx + CloneVarArgs + ExtractAll, #[implementations( Table, @@ -221,7 +221,7 @@ pub fn path_of_subgraph(_: impl Ctx, node_path: Table) -> Table /// context, so the upstream pipeline can return a different value per item that may be derived from the item's own data. /// If the attribute already exists, its values are replaced; if not, the attribute is added. #[node_macro::node(category("General"))] -async fn write_attribute( +async fn write_attribute( ctx: impl ExtractAll + CloneVarArgs + Ctx, /// The `Table` whose items will gain or have replaced the named attribute. #[implementations( From a7ce28ddaca74209b3083f83ed8d25d21ad6fde3 Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Wed, 29 Apr 2026 04:15:51 -0700 Subject: [PATCH 2/2] Cover all attributes in Table PartialEq and CacheHash Co-authored-by: Copilot --- node-graph/libraries/core-types/src/table.rs | 59 ++++++++++++-------- node-graph/nodes/graphic/src/graphic.rs | 2 +- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/node-graph/libraries/core-types/src/table.rs b/node-graph/libraries/core-types/src/table.rs index c016457b6f..909b3237cd 100644 --- a/node-graph/libraries/core-types/src/table.rs +++ b/node-graph/libraries/core-types/src/table.rs @@ -84,7 +84,7 @@ trait AttributeValue: std::any::Any + Send + Sync { fn into_column(self: Box, preceding_defaults: usize) -> Box; } -impl AttributeValue for T { +impl AttributeValue for T { /// Clones this value into a new boxed trait object. fn clone_box(&self) -> Box { Box::new(self.clone()) @@ -168,6 +168,10 @@ trait AttributeColumn: std::any::Any + Send + Sync { /// Hashes every value in this column into the given hasher (object-safe wrapper around `CacheHash`). fn cache_hash_dyn(&self, state: &mut dyn core::hash::Hasher); + + /// Compares this column to another for value-by-value equality (object-safe wrapper around `PartialEq`). + /// Returns `false` if the underlying types differ. + fn eq_dyn(&self, other: &dyn AttributeColumn) -> bool; } /// Adapts a `&mut dyn Hasher` so generic `CacheHash::cache_hash` calls (which require `H: Sized + Hasher`) can @@ -195,7 +199,7 @@ impl Clone for Box { /// Wraps a Vec for column-major attribute storage in a Table. struct Column(Vec); -impl AttributeColumn for Column { +impl AttributeColumn for Column { /// Clones this column into a new boxed trait object. fn clone_box(&self) -> Box { Box::new(Column(self.0.clone())) @@ -267,11 +271,14 @@ impl AttributeCo self.0.into_iter().map(|v| Box::new(v) as Box).collect() } + /// Hashes every value in this column into the given hasher (object-safe wrapper around `CacheHash`). fn cache_hash_dyn(&self, state: &mut dyn core::hash::Hasher) { - let mut wrapped = DynHasher(state); - for value in &self.0 { - value.cache_hash(&mut wrapped); - } + self.0.cache_hash(&mut DynHasher(state)); + } + + /// Compares this column to another for value-by-value equality (object-safe wrapper around `PartialEq`). + fn eq_dyn(&self, other: &dyn AttributeColumn) -> bool { + other.as_any().downcast_ref::().is_some_and(|other| self.0 == other.0) } } @@ -301,7 +308,7 @@ impl AttributeValues { } /// Inserts an attribute with the given key and value, replacing any existing entry with the same key. - pub fn insert(&mut self, key: impl Into, value: T) { + pub fn insert(&mut self, key: impl Into, value: T) { let key = key.into(); for (existing_key, existing_value) in &mut self.0 { @@ -332,7 +339,7 @@ impl AttributeValues { } /// Gets a mutable reference to the value, inserting a default if it doesn't exist or has the wrong type. - pub fn get_or_insert_default_mut(&mut self, key: &str) -> &mut T { + pub fn get_or_insert_default_mut(&mut self, key: &str) -> &mut T { let needs_insert = match self.0.iter().position(|(existing_key, _)| existing_key == key) { Some(index) => { if (*self.0[index].1).as_any().downcast_ref::().is_some() { @@ -471,7 +478,7 @@ impl AttributeColumns { /// Finds or creates a column for the given key and type, returning its position. /// If a column with the key exists but has the wrong type, it is removed and replaced with a new column of the correct type, padded with defaults. /// A newly created column is filled with `T::default()` for all existing rows. - fn find_or_create_column(&mut self, key: &str) -> usize { + fn find_or_create_column(&mut self, key: &str) -> usize { match self.columns.iter().position(|(k, _)| k == key) { Some(position) => { if (*self.columns[position].1).as_any().downcast_ref::>().is_some() { @@ -490,7 +497,7 @@ impl AttributeColumns { } /// Gets a mutable reference to the value at the given index, creating the column if it doesn't exist or has the wrong type. - fn get_or_insert_default_value(&mut self, key: &str, index: usize) -> &mut T { + fn get_or_insert_default_value(&mut self, key: &str, index: usize) -> &mut T { let column_position = self.find_or_create_column::(key); let column = (*self.columns[column_position].1).as_any_mut().downcast_mut::>().unwrap(); &mut column.0[index] @@ -498,7 +505,7 @@ impl AttributeColumns { /// Sets the value at the given index in the column for the given key. /// Creates the column with defaults if it doesn't exist. - fn set_value(&mut self, key: impl Into, index: usize, value: T) { + fn set_value(&mut self, key: impl Into, index: usize, value: T) { let key = key.into(); let column_position = self.find_or_create_column::(&key); let column = (*self.columns[column_position].1).as_any_mut().downcast_mut::>().unwrap(); @@ -550,7 +557,7 @@ impl AttributeColumns { } /// Returns a mutable typed slice of the column for the given key, creating a new column filled with defaults if it doesn't exist. - fn get_or_create_column_slice_mut(&mut self, key: &str) -> &mut [T] { + fn get_or_create_column_slice_mut(&mut self, key: &str) -> &mut [T] { let position = self.find_or_create_column::(key); let column = (*self.columns[position].1).as_any_mut().downcast_mut::>().unwrap(); &mut column.0 @@ -690,7 +697,7 @@ impl Table { } /// Returns a mutable iterator over a typed attribute column, creating the column with default values if it doesn't exist or has the wrong type. - pub fn iter_attribute_values_mut_or_default(&mut self, key: &str) -> std::slice::IterMut<'_, U> { + pub fn iter_attribute_values_mut_or_default(&mut self, key: &str) -> std::slice::IterMut<'_, U> { self.attributes.get_or_create_column_slice_mut::(key).iter_mut() } @@ -728,13 +735,13 @@ impl Table { } /// Sets the attribute value at the given row index and key, creating the column with defaults if it doesn't exist. - pub fn set_attribute(&mut self, key: impl Into, index: usize, value: U) { + pub fn set_attribute(&mut self, key: impl Into, index: usize, value: U) { self.attributes.set_value(key, index, value); } /// Runs the given closure on a mutable reference to the attribute value at the given row index, /// creating the column with defaults if it doesn't exist, and returns the closure's result. - pub fn with_attribute_mut_or_default R>(&mut self, key: &str, index: usize, f: F) -> R { + pub fn with_attribute_mut_or_default R>(&mut self, key: &str, index: usize, f: F) -> R { f(self.attributes.get_or_insert_default_value::(key, index)) } @@ -755,7 +762,7 @@ impl Table { /// Returns disjoint mutable references to the element slice and a typed attribute column slice, /// creating the column with defaults if it doesn't exist. This enables simultaneous mutable /// access to elements and a single attribute column without borrowing conflicts. - pub fn element_and_attribute_slices_mut(&mut self, key: &str) -> (&mut [T], &mut [U]) { + pub fn element_and_attribute_slices_mut(&mut self, key: &str) -> (&mut [T], &mut [U]) { let Self { element, attributes } = self; let column_position = attributes.find_or_create_column::(key); let column = (*attributes.columns[column_position].1).as_any_mut().downcast_mut::>().unwrap(); @@ -864,10 +871,10 @@ impl Default for Table { impl CacheHash for Table { fn cache_hash(&self, state: &mut H) { - for element in self.iter_element_values() { - element.cache_hash(state); - } + self.element.cache_hash(state); + // Hash every attribute column (key + values) rather than just the well-known ones, so changes to user-defined keys + // (e.g., gradient_type, spread_method) invalidate downstream graph caches as expected for (key, column) in &self.attributes.columns { std::hash::Hash::hash(key.as_str(), state); column.cache_hash_dyn(state); @@ -877,7 +884,15 @@ impl CacheHash for Table { impl PartialEq for Table { fn eq(&self, other: &Self) -> bool { + // Attributes participate in equality so the `a == b` ⇒ `hash(a) == hash(b)` contract holds with `cache_hash` self.element == other.element + && self.attributes.columns.len() == other.attributes.columns.len() + && self + .attributes + .columns + .iter() + .zip(&other.attributes.columns) + .all(|((self_key, self_column), (other_key, other_column))| self_key == other_key && self_column.eq_dyn(other_column.as_ref())) } } @@ -1038,17 +1053,17 @@ impl TableRow { } /// Returns a mutable reference to the attribute value for the given key, inserting a default value if absent or of a different type. - pub fn attribute_mut_or_insert_default(&mut self, key: &str) -> &mut U { + pub fn attribute_mut_or_insert_default(&mut self, key: &str) -> &mut U { self.attributes.get_or_insert_default_mut(key) } /// Sets the attribute value for the given key, replacing any existing entry with the same key. - pub fn set_attribute(&mut self, key: impl Into, value: U) { + pub fn set_attribute(&mut self, key: impl Into, value: U) { self.attributes.insert(key, value); } /// Sets the attribute value for the given key and returns the row, enabling builder-style chaining. - pub fn with_attribute(mut self, key: impl Into, value: U) -> Self { + pub fn with_attribute(mut self, key: impl Into, value: U) -> Self { self.set_attribute(key, value); self } diff --git a/node-graph/nodes/graphic/src/graphic.rs b/node-graph/nodes/graphic/src/graphic.rs index 5fa83a4cbf..54b7f89412 100644 --- a/node-graph/nodes/graphic/src/graphic.rs +++ b/node-graph/nodes/graphic/src/graphic.rs @@ -221,7 +221,7 @@ pub fn path_of_subgraph(_: impl Ctx, node_path: Table) -> Table /// context, so the upstream pipeline can return a different value per item that may be derived from the item's own data. /// If the attribute already exists, its values are replaced; if not, the attribute is added. #[node_macro::node(category("General"))] -async fn write_attribute( +async fn write_attribute( ctx: impl ExtractAll + CloneVarArgs + Ctx, /// The `Table` whose items will gain or have replaced the named attribute. #[implementations(