Make VectorModification properly implement CacheHash

Author: Keavon

node-graph/libraries/graphene-hash/src/lib.rs

@@ -202,6 +202,30 @@ impl<T: CacheHash + ?Sized> CacheHash for std::sync::Arc<T> {
 	}
 }
 
+#[cfg(feature = "std")]
+impl<K: CacheHash, V: CacheHash> CacheHash for std::collections::BTreeMap<K, V> {
+	#[inline]
+	fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) {
+		core::hash::Hash::hash(&self.len(), state);
+		for (key, value) in self {
+			key.cache_hash(state);
+			value.cache_hash(state);
+		}
+	}
+}
+
+#[cfg(feature = "std")]
+impl<T: CacheHash> CacheHash for std::collections::BTreeSet<T> {
+	#[inline]
+	fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) {
+		core::hash::Hash::hash(&self.len(), state);
+		for item in self {
+			item.cache_hash(state);
+		}
+	}
+}
+
+
 impl<T: CacheHash + ?Sized> CacheHash for &T {
 	#[inline]
 	fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) {

node-graph/libraries/vector-types/src/vector/vector_modification.rs

@@ -1,26 +1,18 @@
 use super::*;
 use crate::subpath::BezierHandles;
 use crate::vector::misc::{HandleId, HandleType, point_to_dvec2};
-use core_types::uuid::generate_uuid;
 use dyn_any::DynAny;
 use glam::DVec2;
 use kurbo::{BezPath, PathEl, Point};
-use std::collections::{HashMap, HashSet};
-use std::hash::BuildHasher;
+use std::collections::{BTreeMap, BTreeSet, HashSet};
 
 /// Represents a procedural change to the [`PointDomain`] in [`Vector`].
-#[derive(Clone, Debug, Default, PartialEq, serde::Serialize, serde::Deserialize)]
+#[derive(Clone, Debug, Default, PartialEq, graphene_hash::CacheHash, serde::Serialize, serde::Deserialize)]
 pub struct PointModification {
 	add: Vec<PointId>,
-	remove: HashSet<PointId>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	delta: HashMap<PointId, DVec2>,
-}
-
-impl Hash for PointModification {
-	fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-		generate_uuid().hash(state)
-	}
+	remove: BTreeSet<PointId>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	delta: BTreeMap<PointId, DVec2>,
 }
 
 impl PointModification {
@@ -62,7 +54,7 @@ impl PointModification {
 	pub fn create_from_vector<Upstream>(vector: &Vector<Upstream>) -> Self {
 		Self {
 			add: vector.point_domain.ids().to_vec(),
-			remove: HashSet::new(),
+			remove: BTreeSet::new(),
 			delta: vector.point_domain.ids().iter().copied().zip(vector.point_domain.positions().iter().cloned()).collect(),
 		}
 	}
@@ -80,20 +72,20 @@ impl PointModification {
 }
 
 /// Represents a procedural change to the [`SegmentDomain`] in [`Vector`].
-#[derive(Clone, Debug, Default, PartialEq, serde::Serialize, serde::Deserialize)]
+#[derive(Clone, Debug, Default, PartialEq, graphene_hash::CacheHash, serde::Serialize, serde::Deserialize)]
 pub struct SegmentModification {
 	add: Vec<SegmentId>,
-	remove: HashSet<SegmentId>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	start_point: HashMap<SegmentId, PointId>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	end_point: HashMap<SegmentId, PointId>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	handle_primary: HashMap<SegmentId, Option<DVec2>>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	handle_end: HashMap<SegmentId, Option<DVec2>>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	stroke: HashMap<SegmentId, StrokeId>,
+	remove: BTreeSet<SegmentId>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	start_point: BTreeMap<SegmentId, PointId>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	end_point: BTreeMap<SegmentId, PointId>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	handle_primary: BTreeMap<SegmentId, Option<DVec2>>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	handle_end: BTreeMap<SegmentId, Option<DVec2>>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	stroke: BTreeMap<SegmentId, StrokeId>,
 }
 
 impl SegmentModification {
@@ -219,7 +211,7 @@ impl SegmentModification {
 		let point_id = |(&segment, &index)| (segment, vector.point_domain.ids()[index]);
 		Self {
 			add: vector.segment_domain.ids().to_vec(),
-			remove: HashSet::new(),
+			remove: BTreeSet::new(),
 			start_point: vector.segment_domain.ids().iter().zip(vector.segment_domain.start_point()).map(point_id).collect(),
 			end_point: vector.segment_domain.ids().iter().zip(vector.segment_domain.end_point()).map(point_id).collect(),
 			handle_primary: vector.segment_bezier_iter().map(|(id, b, _, _)| (id, b.handle_start().map(|handle| handle - b.start))).collect(),
@@ -250,14 +242,14 @@ impl SegmentModification {
 }
 
 /// Represents a procedural change to the [`RegionDomain`] in [`Vector`].
-#[derive(Clone, Debug, Default, PartialEq, serde::Serialize, serde::Deserialize)]
+#[derive(Clone, Debug, Default, PartialEq, graphene_hash::CacheHash, serde::Serialize, serde::Deserialize)]
 pub struct RegionModification {
 	add: Vec<RegionId>,
-	remove: HashSet<RegionId>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	segment_range: HashMap<RegionId, std::ops::RangeInclusive<SegmentId>>,
-	#[serde(serialize_with = "serialize_hashmap", deserialize_with = "deserialize_hashmap")]
-	fill: HashMap<RegionId, FillId>,
+	remove: BTreeSet<RegionId>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	segment_range: BTreeMap<RegionId, std::ops::RangeInclusive<SegmentId>>,
+	#[serde(deserialize_with = "deserialize_btreemap")]
+	fill: BTreeMap<RegionId, FillId>,
 }
 
 impl RegionModification {
@@ -286,21 +278,21 @@ impl RegionModification {
 	pub fn create_from_vector<Upstream>(vector: &Vector<Upstream>) -> Self {
 		Self {
 			add: vector.region_domain.ids().to_vec(),
-			remove: HashSet::new(),
+			remove: BTreeSet::new(),
 			segment_range: vector.region_domain.ids().iter().copied().zip(vector.region_domain.segment_range().iter().cloned()).collect(),
 			fill: vector.region_domain.ids().iter().copied().zip(vector.region_domain.fill().iter().cloned()).collect(),
 		}
 	}
 }
 
 /// Represents a procedural change to the [`Vector`].
-#[derive(Clone, Debug, Default, PartialEq, DynAny, serde::Serialize, serde::Deserialize)]
+#[derive(Clone, Debug, Default, PartialEq, graphene_hash::CacheHash, DynAny, serde::Serialize, serde::Deserialize)]
 pub struct VectorModification {
 	points: PointModification,
 	segments: SegmentModification,
 	regions: RegionModification,
-	add_g1_continuous: HashSet<[HandleId; 2]>,
-	remove_g1_continuous: HashSet<[HandleId; 2]>,
+	add_g1_continuous: BTreeSet<[HandleId; 2]>,
+	remove_g1_continuous: BTreeSet<[HandleId; 2]>,
 }
 
 /// A modification type that can be added to a [`VectorModification`].
@@ -506,33 +498,70 @@ impl VectorModification {
 			segments: SegmentModification::create_from_vector(vector),
 			regions: RegionModification::create_from_vector(vector),
 			add_g1_continuous: vector.colinear_manipulators.iter().copied().collect(),
-			remove_g1_continuous: HashSet::new(),
+			remove_g1_continuous: BTreeSet::new(),
 		}
 	}
 }
 
-impl Hash for VectorModification {
-	fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-		generate_uuid().hash(state)
+// Deserializes a BTreeMap from either a sequence of tuples (old document format) or a standard map.
+// TODO: Eventually remove this document upgrade code (the sequence-of-tuples path)
+use serde::de::{MapAccess, SeqAccess, Visitor};
+use serde::ser::SerializeSeq;
+use serde::{Deserialize, Deserializer, Serialize, Serializer};
+use std::fmt;
+use std::hash::{BuildHasher, Hash};
+fn deserialize_btreemap<'de, K, V, D>(deserializer: D) -> Result<BTreeMap<K, V>, D::Error>
+where
+	K: Deserialize<'de> + Ord,
+	V: Deserialize<'de>,
+	D: Deserializer<'de>,
+{
+	struct BTreeMapVisitor<K, V> {
+		marker: std::marker::PhantomData<fn() -> BTreeMap<K, V>>,
 	}
-}
 
-// Intentionally non-deterministic: fields contain HashMaps with non-deterministic iteration order,
-// so we use a UUID to always bust the cache and force re-evaluation when any modification is present
-impl graphene_hash::CacheHash for VectorModification {
-	fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) {
-		core::hash::Hash::hash(&generate_uuid(), state);
+	impl<'de, K, V> Visitor<'de> for BTreeMapVisitor<K, V>
+	where
+		K: Deserialize<'de> + Ord,
+		V: Deserialize<'de>,
+	{
+		type Value = BTreeMap<K, V>;
+
+		fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+			formatter.write_str("a map or a sequence of key-value tuples")
+		}
+
+		// Old document format: serialized as a sequence of (key, value) tuples
+		fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
+		where
+			A: SeqAccess<'de>,
+		{
+			let mut map = BTreeMap::new();
+			while let Some((key, value)) = seq.next_element()? {
+				map.insert(key, value);
+			}
+			Ok(map)
+		}
+
+		// New format: standard map serialization from BTreeMap
+		fn visit_map<A>(self, mut map_access: A) -> Result<Self::Value, A::Error>
+		where
+			A: MapAccess<'de>,
+		{
+			let mut map = BTreeMap::new();
+			while let Some((key, value)) = map_access.next_entry()? {
+				map.insert(key, value);
+			}
+			Ok(map)
+		}
 	}
+
+	deserializer.deserialize_any(BTreeMapVisitor { marker: std::marker::PhantomData })
 }
 
-// Do we want to enforce that all serialized/deserialized hashmaps are a vec of tuples?
-// TODO: Eventually remove this document upgrade code
-use serde::de::{SeqAccess, Visitor};
-use serde::ser::SerializeSeq;
-use serde::{Deserialize, Deserializer, Serialize, Serializer};
-use std::fmt;
-use std::hash::Hash;
-pub fn serialize_hashmap<K, V, S, H>(hashmap: &HashMap<K, V, H>, serializer: S) -> Result<S::Ok, S::Error>
+// Serializes a HashMap as a sequence of (key, value) tuples for stable document serialization.
+// Used externally by graph-craft and the editor for HashMap fields in serialized documents.
+pub fn serialize_hashmap<K, V, S, H>(hashmap: &std::collections::HashMap<K, V, H>, serializer: S) -> Result<S::Ok, S::Error>
 where
 	K: Serialize + Eq + Hash,
 	V: Serialize,
@@ -546,7 +575,7 @@ where
 	seq.end()
 }
 
-pub fn deserialize_hashmap<'de, K, V, D, H>(deserializer: D) -> Result<HashMap<K, V, H>, D::Error>
+pub fn deserialize_hashmap<'de, K, V, D, H>(deserializer: D) -> Result<std::collections::HashMap<K, V, H>, D::Error>
 where
 	K: Deserialize<'de> + Eq + Hash,
 	V: Deserialize<'de>,
@@ -555,7 +584,7 @@ where
 {
 	struct HashMapVisitor<K, V, H> {
 		#[allow(clippy::type_complexity)]
-		marker: std::marker::PhantomData<fn() -> HashMap<K, V, H>>,
+		marker: std::marker::PhantomData<fn() -> std::collections::HashMap<K, V, H>>,
 	}
 
 	impl<'de, K, V, H> Visitor<'de> for HashMapVisitor<K, V, H>
@@ -564,7 +593,7 @@ where
 		V: Deserialize<'de>,
 		H: BuildHasher + Default,
 	{
-		type Value = HashMap<K, V, H>;
+		type Value = std::collections::HashMap<K, V, H>;
 
 		fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
 			formatter.write_str("a sequence of tuples")
@@ -574,7 +603,7 @@ where
 		where
 			A: SeqAccess<'de>,
 		{
-			let mut hashmap = HashMap::default();
+			let mut hashmap = std::collections::HashMap::default();
 			while let Some((key, value)) = seq.next_element()? {
 				hashmap.insert(key, value);
 			}