Instance tables refactor part 5: unwrap GraphicGroup as multi-row Instance<GraphicElement> tables and move up transforms (#2363)

* Just group

* Partly working but without transforms

* Remove Transform/TransformMut from GraphicElement and GraphicGroupTable

* Fix layers and flattening

* Fix transform group handling on the remaining nodes

* Change collect metadata

* Add transform on vector data. TODO: Remove duplicate transform

* Small code tidying-up

* Add concatenate node?

* Remove ignore_modifications which is always false

* Improve transforms

* Mostly fix the nested transform cage angle (except leaf layers and skew)

* WIP attempt to integrate skew

* Fix nesting bounding box

* Avoid setting the transform

* Fix stroke transforms

* Renderer cleanup

* Fix tests for repeated elements not given unique point IDs

* Suppress cargo-deny warning

* Fix upgrade code for graphic group data

* Work around rendering issue in Isometric Fountain

---------

Co-authored-by: Adam <adamgerhant@gmail.com>
Co-authored-by: hypercube <0hypercube@gmail.com>

editor/src/dispatcher.rs

@@ -141,14 +141,18 @@ impl Dispatcher {
 					};
 
 					let graphene_std::renderer::RenderMetadata {
-						footprints,
+						upstream_footprints: footprints,
+						local_transforms,
 						click_targets,
 						clip_targets,
 					} = render_metadata;
 
 					// Run these update state messages immediately
 					let messages = [
-						DocumentMessage::UpdateUpstreamTransforms { upstream_transforms: footprints },
+						DocumentMessage::UpdateUpstreamTransforms {
+							upstream_footprints: footprints,
+							local_transforms,
+						},
 						DocumentMessage::UpdateClickTargets { click_targets },
 						DocumentMessage::UpdateClipTargets { clip_targets },
 					];

editor/src/messages/portfolio/document/document_message.rs

@@ -179,7 +179,8 @@ pub enum DocumentMessage {
 	ToggleOverlaysVisibility,
 	ToggleSnapping,
 	UpdateUpstreamTransforms {
-		upstream_transforms: HashMap<NodeId, (Footprint, DAffine2)>,
+		upstream_footprints: HashMap<NodeId, Footprint>,
+		local_transforms: HashMap<NodeId, DAffine2>,
 	},
 	UpdateClickTargets {
 		click_targets: HashMap<NodeId, Vec<ClickTarget>>,

editor/src/messages/portfolio/document/document_message_handler.rs

@@ -1233,8 +1233,11 @@ impl MessageHandler<DocumentMessage, DocumentMessageData<'_>> for DocumentMessag
 				self.snapping_state.snapping_enabled = !self.snapping_state.snapping_enabled;
 				responses.add(PortfolioMessage::UpdateDocumentWidgets);
 			}
-			DocumentMessage::UpdateUpstreamTransforms { upstream_transforms } => {
-				self.network_interface.update_transforms(upstream_transforms);
+			DocumentMessage::UpdateUpstreamTransforms {
+				upstream_footprints,
+				local_transforms,
+			} => {
+				self.network_interface.update_transforms(upstream_footprints, local_transforms);
 			}
 			DocumentMessage::UpdateClickTargets { click_targets } => {
 				// TODO: Allow non layer nodes to have click targets
@@ -1634,6 +1637,44 @@ impl DocumentMessageHandler {
 	pub fn deserialize_document(serialized_content: &str) -> Result<Self, EditorError> {
 		let document_message_handler = serde_json::from_str::<DocumentMessageHandler>(serialized_content)
 			.or_else(|_| {
+				// TODO: Eventually remove this document upgrade code
+				#[derive(Debug, serde::Serialize, serde::Deserialize)]
+				pub struct OldDocumentMessageHandler {
+					// ============================================
+					// Fields that are saved in the document format
+					// ============================================
+					//
+					/// The node graph that generates this document's artwork.
+					/// It recursively stores its sub-graphs, so this root graph is the whole snapshot of the document content.
+					pub network: OldNodeNetwork,
+					/// List of the [`NodeId`]s that are currently selected by the user.
+					pub selected_nodes: SelectedNodes,
+					/// List of the [`LayerNodeIdentifier`]s that are currently collapsed by the user in the Layers panel.
+					/// Collapsed means that the expansion arrow isn't set to show the children of these layers.
+					pub collapsed: CollapsedLayers,
+					/// The name of the document, which is displayed in the tab and title bar of the editor.
+					pub name: String,
+					/// The full Git commit hash of the Graphite repository that was used to build the editor.
+					/// We save this to provide a hint about which version of the editor was used to create the document.
+					pub commit_hash: String,
+					/// The current pan, tilt, and zoom state of the viewport's view of the document canvas.
+					pub document_ptz: PTZ,
+					/// The current mode that the document is in, which starts out as Design Mode. This choice affects the editing behavior of the tools.
+					pub document_mode: DocumentMode,
+					/// The current view mode that the user has set for rendering the document within the viewport.
+					/// This is usually "Normal" but can be set to "Outline" or "Pixels" to see the canvas differently.
+					pub view_mode: ViewMode,
+					/// Sets whether or not all the viewport overlays should be drawn on top of the artwork.
+					/// This includes tool interaction visualizations (like the transform cage and path anchors/handles), the grid, and more.
+					pub overlays_visible: bool,
+					/// Sets whether or not the rulers should be drawn along the top and left edges of the viewport area.
+					pub rulers_visible: bool,
+					/// Sets whether or not the node graph is drawn (as an overlay) on top of the viewport area, or otherwise if it's hidden.
+					pub graph_view_overlay_open: bool,
+					/// The current user choices for snapping behavior, including whether snapping is enabled at all.
+					pub snapping_state: SnappingState,
+				}
+
 				serde_json::from_str::<OldDocumentMessageHandler>(serialized_content).map(|old_message_handler| DocumentMessageHandler {
 					network_interface: NodeNetworkInterface::from_old_network(old_message_handler.network),
 					collapsed: old_message_handler.collapsed,
@@ -2639,41 +2680,3 @@ impl Iterator for ClickXRayIter<'_> {
 		None
 	}
 }
-
-// TODO: Eventually remove this document upgrade code
-#[derive(Debug, serde::Serialize, serde::Deserialize)]
-pub struct OldDocumentMessageHandler {
-	// ============================================
-	// Fields that are saved in the document format
-	// ============================================
-	//
-	/// The node graph that generates this document's artwork.
-	/// It recursively stores its sub-graphs, so this root graph is the whole snapshot of the document content.
-	pub network: OldNodeNetwork,
-	/// List of the [`NodeId`]s that are currently selected by the user.
-	pub selected_nodes: SelectedNodes,
-	/// List of the [`LayerNodeIdentifier`]s that are currently collapsed by the user in the Layers panel.
-	/// Collapsed means that the expansion arrow isn't set to show the children of these layers.
-	pub collapsed: CollapsedLayers,
-	/// The name of the document, which is displayed in the tab and title bar of the editor.
-	pub name: String,
-	/// The full Git commit hash of the Graphite repository that was used to build the editor.
-	/// We save this to provide a hint about which version of the editor was used to create the document.
-	pub commit_hash: String,
-	/// The current pan, tilt, and zoom state of the viewport's view of the document canvas.
-	pub document_ptz: PTZ,
-	/// The current mode that the document is in, which starts out as Design Mode. This choice affects the editing behavior of the tools.
-	pub document_mode: DocumentMode,
-	/// The current view mode that the user has set for rendering the document within the viewport.
-	/// This is usually "Normal" but can be set to "Outline" or "Pixels" to see the canvas differently.
-	pub view_mode: ViewMode,
-	/// Sets whether or not all the viewport overlays should be drawn on top of the artwork.
-	/// This includes tool interaction visualizations (like the transform cage and path anchors/handles), the grid, and more.
-	pub overlays_visible: bool,
-	/// Sets whether or not the rulers should be drawn along the top and left edges of the viewport area.
-	pub rulers_visible: bool,
-	/// Sets whether or not the node graph is drawn (as an overlay) on top of the viewport area, or otherwise if it's hidden.
-	pub graph_view_overlay_open: bool,
-	/// The current user choices for snapping behavior, including whether snapping is enabled at all.
-	pub snapping_state: SnappingState,
-}

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

@@ -237,47 +237,54 @@ impl<'a> ModifyInputsContext<'a> {
 			}
 		})
 	}
+
 	/// Gets the node id of a node with a specific reference that is upstream from the layer node, and optionally creates it if it does not exist.
 	/// The returned node is based on the selection dots in the layer. The right most dot will always insert/access the path that flows directly into the layer.
 	/// Each dot after that represents an existing path node. If there is an existing upstream node, then it will always be returned first.
-	pub fn existing_node_id(&mut self, reference: &'static str, create_if_nonexistent: bool) -> Option<NodeId> {
+	pub fn existing_node_id(&mut self, reference_name: &'static str, create_if_nonexistent: bool) -> Option<NodeId> {
 		// Start from the layer node or export
 		let output_layer = self.get_output_layer()?;
 
-		let upstream = self
-			.network_interface
-			.upstream_flow_back_from_nodes(vec![output_layer.to_node()], &[], network_interface::FlowType::HorizontalFlow);
-
-		// Take until another layer node is found (but not the first layer node)
-		let mut existing_node_id = None;
-		for upstream_node in upstream.collect::<Vec<_>>() {
-			// Check if this is the node we have been searching for.
-			if self
-				.network_interface
-				.reference(&upstream_node, &[])
-				.is_some_and(|node_reference| *node_reference == Some(reference.to_string()))
-			{
-				existing_node_id = Some(upstream_node);
-				break;
-			}
-
-			let is_traversal_start = |node_id: NodeId| {
-				self.layer_node.map(|layer| layer.to_node()) == Some(node_id) || self.network_interface.document_network().exports.iter().any(|export| export.as_node() == Some(node_id))
-			};
-
-			if !is_traversal_start(upstream_node) && (self.network_interface.is_layer(&upstream_node, &[])) {
-				break;
-			}
-		}
+		let existing_node_id = Self::locate_node_in_layer_chain(reference_name, output_layer, self.network_interface);
 
 		// Create a new node if the node does not exist and update its inputs
 		if create_if_nonexistent {
-			return existing_node_id.or_else(|| self.create_node(reference));
+			return existing_node_id.or_else(|| self.create_node(reference_name));
 		}
 
 		existing_node_id
 	}
 
+	/// Gets the node id of a node with a specific reference (name) that is upstream (leftward) from the layer node, but before reaching another upstream layer stack.
+	/// For example, if given a group layer, this would find a requested "Transform" or "Boolean Operation" node in its chain, between the group layer and its layer stack child contents.
+	/// It would also travel up an entire layer that's not fed by a stack until reaching the generator node, such as a "Rectangle" or "Path" layer.
+	pub fn locate_node_in_layer_chain(reference_name: &str, left_of_layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<NodeId> {
+		let upstream = network_interface.upstream_flow_back_from_nodes(vec![left_of_layer.to_node()], &[], network_interface::FlowType::HorizontalFlow);
+
+		// Look at all of the upstream nodes
+		for upstream_node in upstream {
+			// Check if this is the node we have been searching for.
+			if network_interface
+				.reference(&upstream_node, &[])
+				.is_some_and(|node_reference| *node_reference == Some(reference_name.to_string()))
+			{
+				if !network_interface.is_visible(&upstream_node, &[]) {
+					continue;
+				}
+
+				return Some(upstream_node);
+			}
+
+			// Take until another layer node is found (but not the first layer node)
+			let is_traversal_start = |node_id: NodeId| left_of_layer.to_node() == node_id || network_interface.document_network().exports.iter().any(|export| export.as_node() == Some(node_id));
+			if !is_traversal_start(upstream_node) && (network_interface.is_layer(&upstream_node, &[])) {
+				return None;
+			}
+		}
+
+		None
+	}
+
 	/// Create a new node inside the layer
 	pub fn create_node(&mut self, reference: &str) -> Option<NodeId> {
 		let output_layer = self.get_output_layer()?;

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

@@ -2255,7 +2255,15 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
 								..Default::default()
 							}),
 						),
-						PropertiesRow::with_override("Skew", WidgetOverride::Hidden),
+						PropertiesRow::with_override(
+							"Skew",
+							WidgetOverride::Vec2(Vec2InputSettings {
+								x: "X".to_string(),
+								y: "Y".to_string(),
+								unit: "°".to_string(),
+								..Default::default()
+							}),
+						),
 						PropertiesRow::with_override("Pivot", WidgetOverride::Hidden),
 					],
 					output_names: vec!["Data".to_string()],

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

@@ -1,3 +1,6 @@
+use crate::messages::portfolio::document::graph_operation::transform_utils;
+use crate::messages::portfolio::document::graph_operation::utility_types::ModifyInputsContext;
+
 use super::network_interface::NodeNetworkInterface;
 use graph_craft::document::NodeId;
 use graphene_core::renderer::ClickTarget;
@@ -17,7 +20,8 @@ use std::num::NonZeroU64;
 // TODO: it might be better to have a system that can query the state of the node network on demand.
 #[derive(Debug, Clone)]
 pub struct DocumentMetadata {
-	pub upstream_transforms: HashMap<NodeId, (Footprint, DAffine2)>,
+	pub upstream_footprints: HashMap<NodeId, Footprint>,
+	pub local_transforms: HashMap<NodeId, DAffine2>,
 	pub structure: HashMap<LayerNodeIdentifier, NodeRelations>,
 	pub click_targets: HashMap<LayerNodeIdentifier, Vec<ClickTarget>>,
 	pub clip_targets: HashSet<NodeId>,
@@ -29,7 +33,8 @@ pub struct DocumentMetadata {
 impl Default for DocumentMetadata {
 	fn default() -> Self {
 		Self {
-			upstream_transforms: HashMap::new(),
+			upstream_footprints: HashMap::new(),
+			local_transforms: HashMap::new(),
 			structure: HashMap::new(),
 			vector_modify: HashMap::new(),
 			click_targets: HashMap::new(),
@@ -77,14 +82,27 @@ impl DocumentMetadata {
 	}
 
 	pub fn transform_to_viewport(&self, layer: LayerNodeIdentifier) -> DAffine2 {
-		self.upstream_transforms
-			.get(&layer.to_node())
-			.map(|(footprint, transform)| footprint.transform * *transform)
-			.unwrap_or(self.document_to_viewport)
+		let footprint = self.upstream_footprints.get(&layer.to_node()).map(|footprint| footprint.transform).unwrap_or(self.document_to_viewport);
+		let local_transform = self.local_transforms.get(&layer.to_node()).copied().unwrap_or_default();
+
+		footprint * local_transform
+	}
+
+	pub fn transform_to_viewport_with_first_transform_node_if_group(&self, layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> DAffine2 {
+		let footprint = self.upstream_footprints.get(&layer.to_node()).map(|footprint| footprint.transform).unwrap_or(self.document_to_viewport);
+		let local_transform = self.local_transforms.get(&layer.to_node()).copied();
+
+		let transform = local_transform.unwrap_or_else(|| {
+			let transform_node_id = ModifyInputsContext::locate_node_in_layer_chain("Transform", layer, network_interface);
+			let transform_node = transform_node_id.and_then(|id| network_interface.document_node(&id, &[]));
+			transform_node.map(|node| transform_utils::get_current_transform(node.inputs.as_slice())).unwrap_or_default()
+		});
+
+		footprint * transform
 	}
 
 	pub fn upstream_transform(&self, node_id: NodeId) -> DAffine2 {
-		self.upstream_transforms.get(&node_id).copied().map(|(_, transform)| transform).unwrap_or(DAffine2::IDENTITY)
+		self.local_transforms.get(&node_id).copied().unwrap_or(DAffine2::IDENTITY)
 	}
 
 	pub fn downstream_transform_to_document(&self, layer: LayerNodeIdentifier) -> DAffine2 {
@@ -96,10 +114,10 @@ impl DocumentMetadata {
 			return self.transform_to_viewport(layer);
 		}
 
-		self.upstream_transforms
+		self.upstream_footprints
 			.get(&layer.to_node())
 			.copied()
-			.map(|(footprint, _)| footprint.transform)
+			.map(|footprint| footprint.transform)
 			.unwrap_or_else(|| self.transform_to_viewport(layer))
 	}
 }

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

@@ -3244,14 +3244,16 @@ impl NodeNetworkInterface {
 
 		let nodes: HashSet<NodeId> = self.document_network().nodes.keys().cloned().collect::<HashSet<_>>();
 
-		self.document_metadata.upstream_transforms.retain(|node, _| nodes.contains(node));
+		self.document_metadata.upstream_footprints.retain(|node, _| nodes.contains(node));
+		self.document_metadata.local_transforms.retain(|node, _| nodes.contains(node));
 		self.document_metadata.vector_modify.retain(|node, _| nodes.contains(node));
 		self.document_metadata.click_targets.retain(|layer, _| self.document_metadata.structure.contains_key(layer));
 	}
 
 	/// Update the cached transforms of the layers
-	pub fn update_transforms(&mut self, new_upstream_transforms: HashMap<NodeId, (Footprint, DAffine2)>) {
-		self.document_metadata.upstream_transforms = new_upstream_transforms;
+	pub fn update_transforms(&mut self, upstream_footprints: HashMap<NodeId, Footprint>, local_transforms: HashMap<NodeId, DAffine2>) {
+		self.document_metadata.upstream_footprints = upstream_footprints;
+		self.document_metadata.local_transforms = local_transforms;
 	}
 
 	/// Update the cached click targets of the layers

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

@@ -1,6 +1,7 @@
 use super::network_interface::NodeNetworkInterface;
 use crate::consts::{ROTATE_INCREMENT, SCALE_INCREMENT};
-use crate::messages::portfolio::document::graph_operation::utility_types::TransformIn;
+use crate::messages::portfolio::document::graph_operation::transform_utils;
+use crate::messages::portfolio::document::graph_operation::utility_types::{ModifyInputsContext, TransformIn};
 use crate::messages::portfolio::document::utility_types::document_metadata::{DocumentMetadata, LayerNodeIdentifier};
 use crate::messages::prelude::*;
 use crate::messages::tool::common_functionality::graph_modification_utils;
@@ -54,9 +55,15 @@ impl OriginalTransforms {
 		}
 	}
 
-	pub fn update<'a>(&mut self, selected: &'a [LayerNodeIdentifier], network_interface: &NodeNetworkInterface, shape_editor: Option<&'a ShapeState>) {
-		let document_metadata = network_interface.document_metadata();
+	/// Gets the transform from the most downstream transform node
+	fn get_layer_transform(layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<DAffine2> {
+		let transform_node_id = ModifyInputsContext::locate_node_in_layer_chain("Transform", layer, network_interface)?;
 
+		let document_node = network_interface.document_network().nodes.get(&transform_node_id)?;
+		Some(transform_utils::get_current_transform(&document_node.inputs))
+	}
+
+	pub fn update<'a>(&mut self, selected: &'a [LayerNodeIdentifier], network_interface: &NodeNetworkInterface, shape_editor: Option<&'a ShapeState>) {
 		match self {
 			OriginalTransforms::Layer(layer_map) => {
 				layer_map.retain(|layer, _| selected.contains(layer));
@@ -64,7 +71,8 @@ impl OriginalTransforms {
 					if layer == LayerNodeIdentifier::ROOT_PARENT {
 						continue;
 					}
-					layer_map.entry(layer).or_insert_with(|| document_metadata.upstream_transform(layer.to_node()));
+
+					layer_map.entry(layer).or_insert_with(|| Self::get_layer_transform(layer, network_interface).unwrap_or_default());
 				}
 			}
 			OriginalTransforms::Path(path_map) => {
@@ -550,7 +558,7 @@ impl<'a> Selected<'a> {
 			.unwrap_or(DAffine2::IDENTITY);
 
 		if transform.matrix2.determinant().abs() <= f64::EPSILON {
-			transform.matrix2 += DMat2::IDENTITY * 1e-4;
+			transform.matrix2 += DMat2::IDENTITY * 1e-4; // TODO: Is this the cleanest way to handle this?
 		}
 
 		let bounds = self

editor/src/messages/tool/tool_messages/select_tool.rs

@@ -523,16 +523,16 @@ impl Fsm for SelectToolFsmState {
 				}
 
 				// Update bounds
-				let transform = document
+				let mut transform = document
 					.network_interface
 					.selected_nodes()
 					.selected_visible_and_unlocked_layers(&document.network_interface)
 					.find(|layer| !document.network_interface.is_artboard(&layer.to_node(), &[]))
-					.map(|layer| document.metadata().transform_to_viewport(layer));
+					.map(|layer| document.metadata().transform_to_viewport_with_first_transform_node_if_group(layer, &document.network_interface))
+					.unwrap_or_default();
 
-				let mut transform = transform.unwrap_or(DAffine2::IDENTITY);
-				let mut transform_tampered = false;
 				// Check if the matrix is not invertible
+				let mut transform_tampered = false;
 				if transform.matrix2.determinant() == 0. {
 					transform.matrix2 += DMat2::IDENTITY * 1e-4; // TODO: Is this the cleanest way to handle this?
 					transform_tampered = true;
@@ -549,6 +549,7 @@ impl Fsm for SelectToolFsmState {
 							.bounding_box_with_transform(layer, transform.inverse() * document.metadata().transform_to_viewport(layer))
 					})
 					.reduce(graphene_core::renderer::Quad::combine_bounds);
+
 				if let Some(bounds) = bounds {
 					let bounding_box_manager = tool_data.bounding_box_manager.get_or_insert(BoundingBoxManager::default());
 

editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs

@@ -525,7 +525,6 @@ impl MessageHandler<TransformLayerMessage, TransformData<'_>> for TransformLayer
 					responses.add(PenToolMessage::Abort);
 					responses.add(ToolMessage::UpdateHints);
 				} else {
-					selected.revert_operation();
 					selected.original_transforms.clear();
 					self.typing.clear();
 					self.transform_operation = TransformOperation::None;

editor/src/node_graph_executor.rs

@@ -10,7 +10,7 @@ use graph_craft::proto::GraphErrors;
 use graph_craft::wasm_application_io::EditorPreferences;
 use graphene_core::application_io::{NodeGraphUpdateMessage, NodeGraphUpdateSender, RenderConfig};
 use graphene_core::memo::IORecord;
-use graphene_core::renderer::{GraphicElementRendered, ImageRenderMode, RenderParams, SvgRender};
+use graphene_core::renderer::{GraphicElementRendered, RenderParams, SvgRender};
 use graphene_core::renderer::{RenderSvgSegmentList, SvgSegment};
 use graphene_core::text::FontCache;
 use graphene_core::transform::Footprint;
@@ -323,7 +323,7 @@ impl NodeRuntime {
 		let bounds = graphic_element.bounding_box(DAffine2::IDENTITY);
 
 		// Render the thumbnail from a `GraphicElement` into an SVG string
-		let render_params = RenderParams::new(ViewMode::Normal, ImageRenderMode::Base64, bounds, true, false, false);
+		let render_params = RenderParams::new(ViewMode::Normal, bounds, true, false, false);
 		let mut render = SvgRender::new();
 		graphic_element.render_svg(&mut render, &render_params);
 
@@ -655,7 +655,7 @@ impl NodeGraphExecutor {
 	fn debug_render(render_object: impl GraphicElementRendered, transform: DAffine2, responses: &mut VecDeque<Message>) {
 		// Setup rendering
 		let mut render = SvgRender::new();
-		let render_params = RenderParams::new(ViewMode::Normal, ImageRenderMode::Base64, None, false, false, false);
+		let render_params = RenderParams::new(ViewMode::Normal, None, false, false, false);
 
 		// Render SVG
 		render_object.render_svg(&mut render, &render_params);

editor/src/test_utils.rs

@@ -266,7 +266,7 @@ pub mod test_prelude {
 	pub use graph_craft::document::DocumentNode;
 	pub use graphene_core::raster::{Color, Image};
 	pub use graphene_core::{InputAccessor, InputAccessorSource};
-	pub use graphene_std::{transform::Footprint, GraphicGroup};
+	pub use graphene_std::transform::Footprint;
 
 	#[macro_export]
 	macro_rules! float_eq {