Fix how transforms work with footprints and remove a redundant transforms field (#1484)

* Prune unused thumbnails in node graph executor

* Fix transform downcasting failure for GraphicElementData

* Remove more warnings

* Revert upstream transform calculation change

* Use footprint to calculate layer transforms

* Fix artboards

* Move artwork with artboard

* Remove Keavon's warnings

* Prevent misordered FrontendMessages failing to reach JS handlers

---------

Co-authored-by: 0hypercube <[email protected]>
Co-authored-by: Keavon Chambers <[email protected]>

Author: 3 people

demo-artwork/just-a-potted-cactus-v2.graphite

@@ -1,5 +1,6 @@
 use glam::{DAffine2, DVec2};
 use graphene_core::renderer::ClickTarget;
+use graphene_core::transform::Footprint;
 use std::collections::{HashMap, HashSet};
 use std::num::NonZeroU64;
 
@@ -9,8 +10,7 @@ use graphene_core::renderer::Quad;
 
 #[derive(Debug, Clone)]
 pub struct DocumentMetadata {
-	transforms: HashMap<LayerNodeIdentifier, DAffine2>,
-	upstream_transforms: HashMap<NodeId, DAffine2>,
+	upstream_transforms: HashMap<NodeId, (Footprint, DAffine2)>,
 	structure: HashMap<LayerNodeIdentifier, NodeRelations>,
 	artboards: HashSet<LayerNodeIdentifier>,
 	folders: HashSet<LayerNodeIdentifier>,
@@ -23,7 +23,6 @@ pub struct DocumentMetadata {
 impl Default for DocumentMetadata {
 	fn default() -> Self {
 		Self {
-			transforms: HashMap::new(),
 			upstream_transforms: HashMap::new(),
 			click_targets: HashMap::new(),
 			structure: HashMap::from_iter([(LayerNodeIdentifier::ROOT, NodeRelations::default())]),
@@ -207,7 +206,6 @@ impl DocumentMetadata {
 
 		self.selected_nodes.retain(|node| graph.nodes.contains_key(node));
 		self.upstream_transforms.retain(|node, _| graph.nodes.contains_key(node));
-		self.transforms.retain(|layer, _| self.structure.contains_key(layer));
 		self.click_targets.retain(|layer, _| self.structure.contains_key(layer));
 	}
 }
@@ -222,38 +220,29 @@ fn sibling_below<'a>(graph: &'a NodeNetwork, node: &DocumentNode) -> Option<(&'a
 // transforms
 impl DocumentMetadata {
 	/// Update the cached transforms of the layers
-	pub fn update_transforms(&mut self, mut new_transforms: HashMap<LayerNodeIdentifier, DAffine2>, new_upstream_transforms: HashMap<NodeId, DAffine2>) {
-		let mut stack = vec![(LayerNodeIdentifier::ROOT, DAffine2::IDENTITY)];
-		while let Some((layer, transform)) = stack.pop() {
-			for child in layer.children(self) {
-				let Some(new_transform) = new_transforms.get_mut(&child) else { continue };
-				*new_transform = transform * *new_transform;
-
-				stack.push((child, *new_transform));
-			}
-		}
-		self.transforms = new_transforms;
+	pub fn update_transforms(&mut self, new_upstream_transforms: HashMap<NodeId, (Footprint, DAffine2)>) {
 		self.upstream_transforms = new_upstream_transforms;
 	}
 
 	/// Access the cached transformation to document space from layer space
 	pub fn transform_to_document(&self, layer: LayerNodeIdentifier) -> DAffine2 {
-		self.transforms.get(&layer).copied().unwrap_or_else(|| {
-			warn!("Tried to access transform of bad layer {layer:?}");
-			DAffine2::IDENTITY
-		})
-	}
-
-	pub fn local_transform(&self, layer: LayerNodeIdentifier) -> DAffine2 {
-		self.transform_to_document(layer.parent(self).unwrap_or_default()).inverse() * self.transform_to_document(layer)
+		self.document_to_viewport.inverse() * self.transform_to_viewport(layer)
 	}
 
 	pub fn transform_to_viewport(&self, layer: LayerNodeIdentifier) -> DAffine2 {
-		self.document_to_viewport * self.transform_to_document(layer)
+		self.upstream_transforms
+			.get(&layer.to_node())
+			.copied()
+			.map(|(footprint, transform)| footprint.transform * transform)
+			.unwrap_or(DAffine2::IDENTITY)
 	}
 
 	pub fn upstream_transform(&self, node_id: NodeId) -> DAffine2 {
-		self.upstream_transforms.get(&node_id).copied().unwrap_or(DAffine2::IDENTITY)
+		self.upstream_transforms.get(&node_id).copied().map(|(_, transform)| transform).unwrap_or(DAffine2::IDENTITY)
+	}
+
+	pub fn downstream_transform_to_viewport(&self, node_id: NodeId) -> DAffine2 {
+		self.upstream_transforms.get(&node_id).copied().map(|(footprint, _)| footprint.transform).unwrap_or(DAffine2::IDENTITY)
 	}
 }
 

demo-artwork/valley-of-spires-v2.graphite

@@ -576,23 +576,11 @@ impl MessageHandler<GraphOperationMessage, (&mut Document, &mut NodeGraphMessage
 				skip_rerender,
 			} => {
 				let layer_identifier = LayerNodeIdentifier::new(*layer.last().unwrap(), &document.document_network);
-				let parent_transform = document
-					.metadata
-					.transform_to_viewport(layer_identifier.parent(&document.metadata).unwrap_or(LayerNodeIdentifier::ROOT));
+				let parent_transform = document.metadata.downstream_transform_to_viewport(layer_identifier.to_node());
 				let bounds = LayerBounds::new(document, &layer);
 				if let Some(mut modify_inputs) = ModifyInputsContext::new_layer(&layer, document, node_graph, responses) {
 					modify_inputs.transform_change(transform, transform_in, parent_transform, bounds, skip_rerender);
 				}
-
-				let transform = transform.to_cols_array();
-				responses.add(match transform_in {
-					TransformIn::Local => Operation::TransformLayer { path: layer, transform },
-					TransformIn::Scope { scope } => {
-						let scope = scope.to_cols_array();
-						Operation::TransformLayerInScope { path: layer, transform, scope }
-					}
-					TransformIn::Viewport => Operation::TransformLayerInViewport { path: layer, transform },
-				});
 			}
 			GraphOperationMessage::TransformSet {
 				layer,
@@ -601,9 +589,7 @@ impl MessageHandler<GraphOperationMessage, (&mut Document, &mut NodeGraphMessage
 				skip_rerender,
 			} => {
 				let layer_identifier = LayerNodeIdentifier::new(*layer.last().unwrap(), &document.document_network);
-				let parent_transform = document
-					.metadata
-					.transform_to_viewport(layer_identifier.parent(&document.metadata).unwrap_or(LayerNodeIdentifier::ROOT));
+				let parent_transform = document.metadata.downstream_transform_to_viewport(layer_identifier.to_node());
 
 				let current_transform = Some(document.metadata.transform_to_viewport(layer_identifier));
 				let bounds = LayerBounds::new(document, &layer);

document-legacy/src/document_metadata.rs

@@ -258,7 +258,7 @@ fn static_nodes() -> Vec<DocumentNodeBlueprint> {
 		DocumentNodeBlueprint {
 			name: "Artboard",
 			category: "General",
-			identifier: NodeImplementation::proto("graphene_core::ConstructArtboardNode<_, _, _, _>"),
+			identifier: NodeImplementation::proto("graphene_core::ConstructArtboardNode<_, _, _, _, _>"),
 			inputs: vec![
 				DocumentInputType::value("Graphic Group", TaggedValue::GraphicGroup(GraphicGroup::EMPTY), true),
 				DocumentInputType::value("Location", TaggedValue::IVec2(glam::IVec2::ZERO), false),
@@ -268,6 +268,7 @@ fn static_nodes() -> Vec<DocumentNodeBlueprint> {
 			],
 			outputs: vec![DocumentOutputType::new("Out", FrontendGraphDataType::Artboard)],
 			properties: node_properties::artboard_properties,
+			manual_composition: Some(concrete!(Footprint)),
 			..Default::default()
 		},
 		DocumentNodeBlueprint {

editor/src/messages/portfolio/document/node_graph/graph_operation_message_handler.rs

@@ -34,7 +34,7 @@ impl OriginalTransforms {
 		match self {
 			OriginalTransforms::Layer(layer_map) => {
 				for &layer in selected {
-					layer_map.entry(layer).or_insert_with(|| document.metadata.local_transform(layer));
+					layer_map.entry(layer).or_insert_with(|| document.metadata.upstream_transform(layer.to_node()));
 				}
 			}
 			OriginalTransforms::Path(path_map) => {
@@ -368,8 +368,7 @@ impl<'a> Selected<'a> {
 
 	fn transform_layer(document: &Document, layer: LayerNodeIdentifier, original_transform: Option<&DAffine2>, transformation: DAffine2, responses: &mut VecDeque<Message>) {
 		let Some(&original_transform) = original_transform else { return };
-		let parent = layer.parent(&document.metadata);
-		let to = parent.map(|parent| document.metadata.transform_to_viewport(parent)).unwrap_or(document.metadata.document_to_viewport);
+		let to = document.metadata.downstream_transform_to_viewport(layer.to_node());
 		let new = to.inverse() * transformation * to * original_transform;
 		responses.add(GraphOperationMessage::TransformSet {
 			layer: layer.to_path(),

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler/document_node_types.rs

@@ -691,7 +691,6 @@ impl PortfolioMessageHandler {
 
 	pub fn poll_node_graph_evaluation(&mut self, responses: &mut VecDeque<Message>) {
 		let Some(active_document) = self.active_document_id.and_then(|id| self.documents.get_mut(&id)) else {
-			warn!("Polling node graph with no document");
 			return;
 		};
 

editor/src/messages/portfolio/document/utility_types/transformation.rs

@@ -14,14 +14,15 @@ use graph_craft::graphene_compiler::Compiler;
 use graph_craft::imaginate_input::ImaginatePreferences;
 use graph_craft::{concrete, Type};
 use graphene_core::application_io::{ApplicationIo, NodeGraphUpdateMessage, NodeGraphUpdateSender, RenderConfig};
+use graphene_core::memo::IORecord;
 use graphene_core::raster::{Image, ImageFrame};
 use graphene_core::renderer::{ClickTarget, GraphicElementRendered, SvgSegment, SvgSegmentList};
 use graphene_core::text::FontCache;
 use graphene_core::transform::{Footprint, Transform};
 use graphene_core::vector::style::ViewMode;
 use graphene_core::vector::VectorData;
 
-use graphene_core::{Color, SurfaceFrame, SurfaceId};
+use graphene_core::{Color, GraphicElementData, SurfaceFrame, SurfaceId};
 use graphene_std::wasm_application_io::{WasmApplicationIo, WasmEditorApi};
 use interpreted_executor::dynamic_executor::DynamicExecutor;
 
@@ -56,10 +57,10 @@ pub struct NodeRuntime {
 	imaginate_preferences: ImaginatePreferences,
 	pub(crate) thumbnails: HashMap<NodeId, SvgSegmentList>,
 	pub(crate) click_targets: HashMap<NodeId, Vec<ClickTarget>>,
-	pub(crate) transforms: HashMap<NodeId, DAffine2>,
-	pub(crate) upstream_transforms: HashMap<NodeId, DAffine2>,
+	pub(crate) upstream_transforms: HashMap<NodeId, (Footprint, DAffine2)>,
 	graph_hash: Option<u64>,
 	canvas_cache: HashMap<Vec<LayerId>, SurfaceId>,
+	monitor_nodes: Vec<Vec<NodeId>>,
 }
 
 enum NodeRuntimeMessage {
@@ -81,8 +82,7 @@ pub(crate) struct GenerationResponse {
 	updates: VecDeque<Message>,
 	new_thumbnails: HashMap<NodeId, SvgSegmentList>,
 	new_click_targets: HashMap<LayerNodeIdentifier, Vec<ClickTarget>>,
-	new_transforms: HashMap<LayerNodeIdentifier, DAffine2>,
-	new_upstream_transforms: HashMap<NodeId, DAffine2>,
+	new_upstream_transforms: HashMap<NodeId, (Footprint, DAffine2)>,
 	transform: DAffine2,
 }
 
@@ -109,8 +109,6 @@ thread_local! {
 	pub(crate) static NODE_RUNTIME: Rc<RefCell<Option<NodeRuntime>>> = Rc::new(RefCell::new(None));
 }
 
-type MonitorNodes = Vec<Vec<NodeId>>;
-
 impl NodeRuntime {
 	fn new(receiver: Receiver<NodeRuntimeMessage>, sender: Sender<NodeGraphUpdate>) -> Self {
 		let executor = DynamicExecutor::default();
@@ -124,9 +122,9 @@ impl NodeRuntime {
 			wasm_io: None,
 			canvas_cache: HashMap::new(),
 			click_targets: HashMap::new(),
-			transforms: HashMap::new(),
 			graph_hash: None,
 			upstream_transforms: HashMap::new(),
+			monitor_nodes: Vec::new(),
 		}
 	}
 	pub async fn run(&mut self) {
@@ -151,19 +149,18 @@ impl NodeRuntime {
 					..
 				}) => {
 					let transform = render_config.viewport.transform;
-					let (result, monitor_nodes) = self.execute_network(&path, graph, render_config).await;
+					let result = self.execute_network(&path, graph, render_config).await;
 					let mut responses = VecDeque::new();
-					if let Some(ref monitor_nodes) = monitor_nodes {
-						self.update_thumbnails(&path, monitor_nodes, &mut responses);
-						self.update_upstream_transforms(monitor_nodes);
-					}
+
+					self.update_thumbnails(&path, &mut responses);
+					self.update_upstream_transforms();
+
 					let response = GenerationResponse {
 						generation_id,
 						result,
 						updates: responses,
 						new_thumbnails: self.thumbnails.clone(),
 						new_click_targets: self.click_targets.clone().into_iter().map(|(id, targets)| (LayerNodeIdentifier::new_unchecked(id), targets)).collect(),
-						new_transforms: self.transforms.clone().into_iter().map(|(id, transform)| (LayerNodeIdentifier::new_unchecked(id), transform)).collect(),
 						new_upstream_transforms: self.upstream_transforms.clone(),
 						transform,
 					};
@@ -173,7 +170,7 @@ impl NodeRuntime {
 		}
 	}
 
-	async fn execute_network<'a>(&'a mut self, path: &[LayerId], graph: NodeNetwork, render_config: RenderConfig) -> (Result<TaggedValue, String>, Option<MonitorNodes>) {
+	async fn execute_network<'a>(&'a mut self, path: &[LayerId], graph: NodeNetwork, render_config: RenderConfig) -> Result<TaggedValue, String> {
 		if self.wasm_io.is_none() {
 			self.wasm_io = Some(WasmApplicationIo::new().await);
 		}
@@ -200,12 +197,10 @@ impl NodeRuntime {
 			self.graph_hash = None;
 		}
 
-		let mut cached_monitor_nodes = None;
-
 		if self.graph_hash.is_none() {
 			let scoped_network = wrap_network_in_scope(graph, font_hash_code);
 
-			let monitor_nodes = scoped_network
+			self.monitor_nodes = scoped_network
 				.recursive_nodes()
 				.filter(|(_, node)| node.implementation == DocumentNodeImplementation::proto("graphene_core::memo::MonitorNode<_, _, _>"))
 				.map(|(_, node)| node.path.clone().unwrap_or_default())
@@ -216,16 +211,15 @@ impl NodeRuntime {
 			let c = Compiler {};
 			let proto_network = match c.compile_single(scoped_network) {
 				Ok(network) => network,
-				Err(e) => return (Err(e), Some(monitor_nodes)),
+				Err(e) => return Err(e),
 			};
 
 			assert_ne!(proto_network.nodes.len(), 0, "No protonodes exist?");
 			if let Err(e) = self.executor.update(proto_network).await {
 				error!("Failed to update executor:\n{e}");
-				return (Err(e), Some(monitor_nodes));
+				return Err(e);
 			}
 
-			cached_monitor_nodes = Some(monitor_nodes);
 			self.graph_hash = Some(hash_code);
 		}
 
@@ -239,7 +233,7 @@ impl NodeRuntime {
 		};
 		let result = match result {
 			Ok(value) => value,
-			Err(e) => return (Err(e), cached_monitor_nodes),
+			Err(e) => return Err(e),
 		};
 
 		if let TaggedValue::SurfaceFrame(SurfaceFrame { surface_id, transform: _ }) = result {
@@ -252,13 +246,14 @@ impl NodeRuntime {
 				}
 			}
 		}
-		(Ok(result), cached_monitor_nodes)
+		Ok(result)
 	}
 
 	/// Recomputes the thumbnails for the layers in the graph, modifying the state and updating the UI.
-	pub fn update_thumbnails(&mut self, layer_path: &[LayerId], monitor_nodes: &[Vec<u64>], responses: &mut VecDeque<Message>) {
+	pub fn update_thumbnails(&mut self, layer_path: &[LayerId], responses: &mut VecDeque<Message>) {
 		let mut image_data: Vec<_> = Vec::new();
-		for node_path in monitor_nodes {
+		self.thumbnails.retain(|id, _| self.monitor_nodes.iter().any(|node_path| node_path.contains(id)));
+		for node_path in &self.monitor_nodes {
 			let Some(node_id) = node_path.get(node_path.len() - 2).copied() else {
 				warn!("Monitor node has invalid node id");
 				continue;
@@ -268,8 +263,7 @@ impl NodeRuntime {
 				continue;
 			};
 
-			let Some(io_data) = value.downcast_ref::<graphene_core::memo::IORecord<Footprint, graphene_core::GraphicElementData>>() else {
-				warn!("Failed to downcast thumbnail to graphic element data");
+			let Some(io_data) = value.downcast_ref::<IORecord<Footprint, graphene_core::GraphicElementData>>() else {
 				continue;
 			};
 			let graphic_element_data = &io_data.output;
@@ -283,10 +277,9 @@ impl NodeRuntime {
 
 			let click_targets = self.click_targets.entry(node_id).or_default();
 			click_targets.clear();
+			// Add the graphic element data's click targets to the click targets vector
 			graphic_element_data.add_click_targets(click_targets);
 
-			self.transforms.insert(node_id, graphic_element_data.transform());
-
 			let old_thumbnail = self.thumbnails.entry(node_id).or_default();
 			if *old_thumbnail != render.svg {
 				responses.add(FrontendMessage::UpdateNodeThumbnail {
@@ -305,8 +298,8 @@ impl NodeRuntime {
 		}
 	}
 
-	pub fn update_upstream_transforms(&mut self, monitor_nodes: &[Vec<u64>]) {
-		for node_path in monitor_nodes {
+	pub fn update_upstream_transforms(&mut self) {
+		for node_path in &self.monitor_nodes {
 			let Some(node_id) = node_path.get(node_path.len() - 2).copied() else {
 				warn!("Monitor node has invalid node id");
 				continue;
@@ -315,14 +308,16 @@ impl NodeRuntime {
 				warn!("Failed to introspect monitor node for upstream transforms");
 				continue;
 			};
-			let Some(transform) = value
-				.downcast_ref::<graphene_core::memo::IORecord<Footprint, VectorData>>()
-				.map(|vector_data| vector_data.output.transform())
-				.or_else(|| {
-					value
-						.downcast_ref::<graphene_core::memo::IORecord<Footprint, ImageFrame<Color>>>()
-						.map(|image| image.output.transform())
-				})
+
+			fn try_downcast<T: Transform + 'static>(value: &dyn std::any::Any) -> Option<(Footprint, DAffine2)> {
+				let io_data = value.downcast_ref::<IORecord<Footprint, T>>()?;
+				let transform = io_data.output.transform();
+				Some((io_data.input, transform))
+			}
+
+			let Some(transform) = try_downcast::<VectorData>(value.as_ref())
+				.or_else(|| try_downcast::<ImageFrame<Color>>(value.as_ref()))
+				.or_else(|| try_downcast::<GraphicElementData>(value.as_ref()))
 			else {
 				warn!("Failed to downcast transform input");
 				continue;
@@ -527,7 +522,6 @@ impl NodeGraphExecutor {
 					updates,
 					new_thumbnails,
 					new_click_targets,
-					new_transforms,
 					new_upstream_transforms,
 					transform,
 				}) => {
@@ -566,7 +560,7 @@ impl NodeGraphExecutor {
 						});
 					}
 					self.thumbnails = new_thumbnails;
-					document.metadata.update_transforms(new_transforms, new_upstream_transforms);
+					document.metadata.update_transforms(new_upstream_transforms);
 					document.metadata.update_click_targets(new_click_targets);
 					let node_graph_output = result.map_err(|e| format!("Node graph evaluation failed: {e:?}"))?;
 					let execution_context = self.futures.remove(&generation_id).ok_or_else(|| "Invalid generation ID".to_string())?;