Skip to content

Fix hashing not being based on all attributes of Table #4079

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.

Sign up for GitHub

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

Merged
Keavon merged 2 commits into from
Apr 29, 2026
Merged

Fix hashing not being based on all attributes of Table #4079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1bd897
Fix hashing not being based on all attributes of Table
Keavon Apr 29, 2026
a7ce28d
Cover all attributes in Table PartialEq and CacheHash
Keavon Apr 29, 2026
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
85 changes: 61 additions & 24 deletions node-graph/libraries/core-types/src/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

// =====================================================================
Expand Down Expand Up @@ -83,7 +84,7 @@ trait AttributeValue: std::any::Any + Send + Sync {
fn into_column(self: Box<Self>, preceding_defaults: usize) -> Box<dyn AttributeColumn>;
}

impl<T: Clone + Send + Sync + Default + Sized + Debug + 'static> AttributeValue for T {
impl<T: Clone + Send + Sync + Default + Sized + Debug + PartialEq + CacheHash + 'static> AttributeValue for T {
/// Clones this value into a new boxed trait object.
fn clone_box(&self) -> Box<dyn AttributeValue> {
Box::new(self.clone())
Expand Down Expand Up @@ -164,6 +165,25 @@ 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<Self>) -> Vec<Box<dyn AttributeValue>>;

/// 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<H>` 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<dyn AttributeColumn> {
Expand All @@ -179,7 +199,7 @@ impl Clone for Box<dyn AttributeColumn> {
/// Wraps a Vec<T> for column-major attribute storage in a Table.
struct Column<T>(Vec<T>);

impl<T: Clone + Send + Sync + Default + Debug + 'static> AttributeColumn for Column<T> {
impl<T: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static> AttributeColumn for Column<T> {
/// Clones this column into a new boxed trait object.
fn clone_box(&self) -> Box<dyn AttributeColumn> {
Box::new(Column(self.0.clone()))
Expand Down Expand Up @@ -250,6 +270,16 @@ impl<T: Clone + Send + Sync + Default + Debug + 'static> AttributeColumn for Col
fn drain(self: Box<Self>) -> Vec<Box<dyn AttributeValue>> {
self.0.into_iter().map(|v| Box::new(v) as Box<dyn AttributeValue>).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) {
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::<Self>().is_some_and(|other| self.0 == other.0)
}
Comment on lines +275 to +282
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 29, 2026

Choose a reason for hiding this comment

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

medium

The manual iteration over the column values to hash them individually misses the opportunity to hash the column's length, which is important for distinguishing between different length columns (e.g., when one is a prefix of another). Since Vec<T> implements CacheHash (which includes the length), you can simplify this implementation by delegating directly to the inner vector.

	fn cache_hash_dyn(&self, state: &mut dyn core::hash::Hasher) {
		self.0.cache_hash(&mut DynHasher(state));
	}

}

// ===============
Expand Down Expand Up @@ -278,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<T: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: impl Into<String>, value: T) {
pub fn insert<T: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: impl Into<String>, value: T) {
let key = key.into();

for (existing_key, existing_value) in &mut self.0 {
Expand Down Expand Up @@ -309,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<T: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str) -> &mut T {
pub fn get_or_insert_default_mut<T: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&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::<T>().is_some() {
Expand Down Expand Up @@ -448,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<T: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str) -> usize {
fn find_or_create_column<T: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: &str) -> usize {
match self.columns.iter().position(|(k, _)| k == key) {
Some(position) => {
if (*self.columns[position].1).as_any().downcast_ref::<Column<T>>().is_some() {
Expand All @@ -467,15 +497,15 @@ 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<T: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str, index: usize) -> &mut T {
fn get_or_insert_default_value<T: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: &str, index: usize) -> &mut T {
let column_position = self.find_or_create_column::<T>(key);
let column = (*self.columns[column_position].1).as_any_mut().downcast_mut::<Column<T>>().unwrap();
&mut column.0[index]
}

/// 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<T: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: impl Into<String>, index: usize, value: T) {
fn set_value<T: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: impl Into<String>, index: usize, value: T) {
let key = key.into();
let column_position = self.find_or_create_column::<T>(&key);
let column = (*self.columns[column_position].1).as_any_mut().downcast_mut::<Column<T>>().unwrap();
Expand Down Expand Up @@ -527,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<T: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str) -> &mut [T] {
fn get_or_create_column_slice_mut<T: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: &str) -> &mut [T] {
let position = self.find_or_create_column::<T>(key);
let column = (*self.columns[position].1).as_any_mut().downcast_mut::<Column<T>>().unwrap();
&mut column.0
Expand Down Expand Up @@ -667,7 +697,7 @@ impl<T> Table<T> {
}

/// 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<U: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str) -> std::slice::IterMut<'_, U> {
pub fn iter_attribute_values_mut_or_default<U: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: &str) -> std::slice::IterMut<'_, U> {
self.attributes.get_or_create_column_slice_mut::<U>(key).iter_mut()
}

Expand Down Expand Up @@ -705,13 +735,13 @@ impl<T> Table<T> {
}

/// 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<U: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: impl Into<String>, index: usize, value: U) {
pub fn set_attribute<U: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: impl Into<String>, 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<U: Clone + Send + Sync + Default + Debug + 'static, R, F: FnOnce(&mut U) -> R>(&mut self, key: &str, index: usize, f: F) -> R {
pub fn with_attribute_mut_or_default<U: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static, R, F: FnOnce(&mut U) -> R>(&mut self, key: &str, index: usize, f: F) -> R {
f(self.attributes.get_or_insert_default_value::<U>(key, index))
}

Expand All @@ -732,7 +762,7 @@ impl<T> Table<T> {
/// 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<U: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str) -> (&mut [T], &mut [U]) {
pub fn element_and_attribute_slices_mut<U: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: &str) -> (&mut [T], &mut [U]) {
let Self { element, attributes } = self;
let column_position = attributes.find_or_create_column::<U>(key);
let column = (*attributes.columns[column_position].1).as_any_mut().downcast_mut::<Column<U>>().unwrap();
Expand Down Expand Up @@ -839,23 +869,30 @@ impl<T> Default for Table<T> {
}
}

impl<T: graphene_hash::CacheHash> graphene_hash::CacheHash for Table<T> {
impl<T: CacheHash> CacheHash for Table<T> {
fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) {
for element in self.iter_element_values() {
element.cache_hash(state);
}
for transform in self.iter_attribute_values_or_default::<DAffine2>(ATTR_TRANSFORM) {
graphene_hash::CacheHash::cache_hash(&transform, state);
}
for alpha_blending in self.iter_attribute_values_or_default::<crate::AlphaBlending>(ATTR_ALPHA_BLENDING) {
alpha_blending.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);
}
}
Comment on lines 873 to 882
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 29, 2026

Choose a reason for hiding this comment

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

medium

The manual iteration over self.iter_element_values() misses the length of the element vector. Using self.element.cache_hash(state) directly is more robust as it includes the length. Additionally, to maintain the Hash trait contract where a == b must imply hash(a) == hash(b), the hash should only include fields used in PartialEq. Since Table currently only compares the element field—and the attributes field is intentionally not serialized due to its type-erased nature—the hash should remain focused on the element data.

	fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) {
		self.element.cache_hash(state);
	}
References
  1. The attributes field in Table and TableRow is intentionally not serialized because it contains type-erased data, and serialization for this is not currently implemented.

}

impl<T: PartialEq> PartialEq for Table<T> {
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()))
}
}

Expand Down Expand Up @@ -1016,17 +1053,17 @@ impl<T> TableRow<T> {
}

/// 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<U: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str) -> &mut U {
pub fn attribute_mut_or_insert_default<U: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&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<U: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: impl Into<String>, value: U) {
pub fn set_attribute<U: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(&mut self, key: impl Into<String>, 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<U: Clone + Send + Sync + Default + Debug + 'static>(mut self, key: impl Into<String>, value: U) -> Self {
pub fn with_attribute<U: Clone + Send + Sync + Default + Debug + PartialEq + CacheHash + 'static>(mut self, key: impl Into<String>, value: U) -> Self {
self.set_attribute(key, value);
self
}
Expand Down
6 changes: 3 additions & 3 deletions node-graph/nodes/graphic/src/graphic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -107,7 +107,7 @@ pub fn extract_element<T: Clone + Default + Send + Sync + 'static>(
}

#[node_macro::node(category("General"))]
async fn map<Item: AnyHash + Send + Sync + core_types::CacheHash>(
async fn map<Item: AnyHash + Send + Sync + CacheHash>(
ctx: impl Ctx + CloneVarArgs + ExtractAll,
#[implementations(
Table<Graphic>,
Expand Down Expand Up @@ -221,7 +221,7 @@ pub fn path_of_subgraph(_: impl Ctx, node_path: Table<NodeId>) -> Table<NodeId>
/// 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<T: AnyHash + Clone + Send + Sync + core_types::CacheHash, U: Clone + Send + Sync + Default + std::fmt::Debug + 'static>(
async fn write_attribute<T: AnyHash + Clone + Send + Sync + CacheHash, U: Clone + Send + Sync + Default + std::fmt::Debug + PartialEq + CacheHash + 'static>(
ctx: impl ExtractAll + CloneVarArgs + Ctx,
/// The `Table` whose items will gain or have replaced the named attribute.
#[implementations(
Expand Down
Loading