Fix image and SVG import transform bugs (#3942)

* Fix half-pixel offset on imported images

* Break out reused function

* Fix SVG/image open flow placing content with unnecessary Transform nodes

* Fix redundant Transform nodes when opening SVG/image files as documents

* Offset the parent to its destination position not the child objects

* Fix SVG/image File > Open artboard dimensions, origin, and clipping

* Fix the SVG to drag in at the mouse position relative to its visible center

* Fix importing images into offset artboards so they don't get offset as well

* Code review

Author: Keavon

editor/src/consts.rs

@@ -8,6 +8,8 @@ pub const IMPORTS_TO_LEFT_EDGE_PIXEL_GAP: u32 = 120;
 pub const STACK_VERTICAL_GAP: i32 = 3;
 /// Horizontal grid indentation of a child layer relative to its parent layer.
 pub const LAYER_INDENT_OFFSET: i32 = 8;
+/// Horizontal grid width of a non-layer node in a chain.
+pub const NODE_CHAIN_WIDTH: i32 = 7;
 
 // VIEWPORT
 pub const VIEWPORT_ZOOM_WHEEL_RATE: f64 = (1. / 600.) * 3.;

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

@@ -9,7 +9,7 @@ use crate::messages::portfolio::document::utility_types::document_metadata::Laye
 use crate::messages::portfolio::document::utility_types::misc::{AlignAggregate, AlignAxis, FlipAxis, GridSnapping};
 use crate::messages::portfolio::utility_types::PanelType;
 use crate::messages::prelude::*;
-use glam::DAffine2;
+use glam::{DAffine2, IVec2};
 use graph_craft::document::NodeId;
 use graphene_std::Color;
 use graphene_std::raster::BlendMode;
@@ -105,12 +105,18 @@ pub enum DocumentMessage {
 		image: Image<Color>,
 		mouse: Option<(f64, f64)>,
 		parent_and_insert_index: Option<(LayerNodeIdentifier, usize)>,
+		/// When true (file-open flow), place the image at the document origin so `WrapContentInArtboard`
+		/// can wrap it without a content Transform node. When false, place at the cursor or viewport center.
+		place_at_origin: bool,
 	},
 	PasteSvg {
 		name: Option<String>,
 		svg: String,
 		mouse: Option<(f64, f64)>,
 		parent_and_insert_index: Option<(LayerNodeIdentifier, usize)>,
+		/// When true (file-open flow), place the SVG at the document origin so `WrapContentInArtboard`
+		/// can wrap it without a content Transform node. When false, place at the cursor or viewport center.
+		place_at_origin: bool,
 	},
 	Redo,
 	RenameDocument {
@@ -223,6 +229,9 @@ pub enum DocumentMessage {
 	SelectionStepForward,
 	WrapContentInArtboard {
 		place_artboard_at_origin: bool,
+		/// When `Some`, use this canvas (origin, dimensions) for the artboard instead of measuring the content bounding box.
+		/// The origin comes from the SVG viewBox's min-x/min-y values and the dimensions from its width/height.
+		artboard_canvas: Option<(IVec2, IVec2)>,
 	},
 	ZoomCanvasTo100Percent,
 	ZoomCanvasTo200Percent,

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

@@ -4,7 +4,9 @@ use super::utility_types::misc::{GroupFolderType, SNAP_FUNCTIONS_FOR_BOUNDING_BO
 use super::utility_types::network_interface::{self, NodeNetworkInterface, TransactionStatus};
 use super::utility_types::nodes::{CollapsedLayers, LayerStructureEntry, SelectedNodes};
 use crate::application::{GRAPHITE_GIT_COMMIT_HASH, generate_uuid};
-use crate::consts::{ASYMPTOTIC_EFFECT, COLOR_OVERLAY_GRAY, DEFAULT_DOCUMENT_NAME, FILE_EXTENSION, SCALE_EFFECT, SCROLLBAR_SPACING, VIEWPORT_ROTATE_SNAP_INTERVAL};
+use crate::consts::{
+	ASYMPTOTIC_EFFECT, COLOR_OVERLAY_GRAY, DEFAULT_DOCUMENT_NAME, FILE_EXTENSION, LAYER_INDENT_OFFSET, NODE_CHAIN_WIDTH, SCALE_EFFECT, SCROLLBAR_SPACING, VIEWPORT_ROTATE_SNAP_INTERVAL,
+};
 use crate::messages::input_mapper::utility_types::macros::action_shortcut;
 use crate::messages::layout::utility_types::widget_prelude::*;
 use crate::messages::portfolio::document::data_panel::{DataPanelMessageContext, DataPanelMessageHandler};
@@ -658,29 +660,35 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 				image,
 				mouse,
 				parent_and_insert_index,
+				place_at_origin,
 			} => {
 				// All the image's pixels have been converted to 0..=1, linear, and premultiplied by `Color::from_rgba8_srgb`
 
+				let layer_parent = self.new_layer_parent(true);
 				let image_size = DVec2::new(image.width as f64, image.height as f64);
 
-				// Align the layer with the mouse or center of viewport
-				let viewport_location = mouse.map_or(viewport.center_in_viewport_space().into_dvec2() + viewport.offset().into_dvec2(), |pos| pos.into());
-
-				let document_to_viewport = self.navigation_handler.calculate_offset_transform(viewport.center_in_viewport_space().into(), &self.document_ptz);
-				let center_in_viewport = DAffine2::from_translation(document_to_viewport.inverse().transform_point2(viewport_location - viewport.offset().into_dvec2()));
-				let center_in_viewport_layerspace = center_in_viewport;
-
-				// Make layer the size of the image
-				let fit_image_size = DAffine2::from_scale_angle_translation(image_size, 0., image_size / -2.);
-
-				let transform = center_in_viewport_layerspace * fit_image_size;
+				let mut transform = if place_at_origin {
+					// File-open flow: place at document origin without centering so `WrapContentInArtboard` can wrap it
+					DAffine2::from_scale(image_size)
+				} else {
+					// Clipboard paste or drag-drop: center at cursor or viewport center.
+					// Convert the document-space cursor to the parent's local coordinate space so that
+					// an artboard at a non-zero position does not offset the placement.
+					let parent_to_document = {
+						let metadata = self.metadata();
+						metadata.document_to_viewport.inverse() * metadata.transform_to_viewport(layer_parent)
+					};
+					let cursor_in_parent = parent_to_document.inverse() * self.document_transform_from_mouse(mouse, viewport);
+					cursor_in_parent * DAffine2::from_scale_angle_translation(image_size, 0., image_size / -2.)
+				};
+				transform.translation = transform.translation.round();
 
 				let layer_node_id = NodeId::new();
 				let layer_id = LayerNodeIdentifier::new_unchecked(layer_node_id);
 
 				responses.add(DocumentMessage::AddTransaction);
 
-				let layer = graph_modification_utils::new_image_layer(Table::new_from_element(Raster::new_cpu(image)), layer_node_id, self.new_layer_parent(true), responses);
+				let layer = graph_modification_utils::new_image_layer(Table::new_from_element(Raster::new_cpu(image)), layer_node_id, layer_parent, responses);
 
 				if let Some(name) = name {
 					responses.add(NodeGraphMessage::SetDisplayName {
@@ -715,17 +723,29 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 				svg,
 				mouse,
 				parent_and_insert_index,
+				place_at_origin,
 			} => {
-				let document_to_viewport = self.navigation_handler.calculate_offset_transform(viewport.center_in_viewport_space().into(), &self.document_ptz);
-				let viewport_location = mouse.map_or(viewport.center_in_viewport_space().into_dvec2() + viewport.offset().into_dvec2(), |pos| pos.into());
-				let center_in_viewport = DAffine2::from_translation(document_to_viewport.inverse().transform_point2(viewport_location - viewport.offset().into_dvec2()));
+				let layer_parent = self.new_layer_parent(true);
+				let transform = if place_at_origin {
+					// File-open flow: place at document origin so `WrapContentInArtboard` can wrap it without extra Transform nodes
+					DAffine2::IDENTITY
+				} else {
+					// Clipboard paste or drag-drop: center at cursor or viewport center.
+					// Convert the document-space cursor to the parent's local coordinate space so that
+					// an artboard at a non-zero position does not offset the placement.
+					let parent_to_document = {
+						let metadata = self.metadata();
+						metadata.document_to_viewport.inverse() * metadata.transform_to_viewport(layer_parent)
+					};
+					parent_to_document.inverse() * self.document_transform_from_mouse(mouse, viewport)
+				};
 
 				let layer_node_id = NodeId::new();
 				let layer_id = LayerNodeIdentifier::new_unchecked(layer_node_id);
 
 				responses.add(DocumentMessage::AddTransaction);
 
-				let layer = graph_modification_utils::new_svg_layer(svg, center_in_viewport, layer_node_id, self.new_layer_parent(true), responses);
+				let layer = graph_modification_utils::new_svg_layer(svg, transform, !place_at_origin, layer_node_id, layer_parent, responses);
 
 				if let Some(name) = name {
 					responses.add(NodeGraphMessage::SetDisplayName {
@@ -1347,27 +1367,46 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 				self.network_interface.selection_step_forward(&self.selection_network_path);
 				responses.add(EventMessage::SelectionChanged);
 			}
-			DocumentMessage::WrapContentInArtboard { place_artboard_at_origin } => {
-				// Get bounding box of all layers
+			DocumentMessage::WrapContentInArtboard {
+				place_artboard_at_origin,
+				artboard_canvas,
+			} => {
+				// Get bounding box of all layers (always needed to confirm there is content)
 				let bounds = self.network_interface.document_bounds_document_space(false);
 				let Some(bounds) = bounds else { return };
-				let bounds_rounded_dimensions = (bounds[1] - bounds[0]).round();
+
+				// When artboard_canvas is provided (SVG file-open flow), use the declared canvas origin and dimensions;
+				// no content-shift Transform node needed since the SVG was already placed at its natural coordinates.
+				let (artboard_location, artboard_dimensions, content_shift) = if let Some((origin, dimensions)) = artboard_canvas {
+					(origin, dimensions, DVec2::ZERO)
+				} else {
+					// No declared canvas (image or clipboard paste): derive location and dimensions from the content bounding box.
+					let location = if place_artboard_at_origin { IVec2::ZERO } else { bounds[0].round().as_ivec2() };
+					(location, (bounds[1] - bounds[0]).round().as_ivec2(), -bounds[0].round())
+				};
 
 				// Create an artboard and set its dimensions to the bounding box size and location
 				let node_id = NodeId::new();
 				let node_layer_id = LayerNodeIdentifier::new_unchecked(node_id);
 				let new_artboard_node = document_node_definitions::resolve_network_node_type("Artboard")
 					.expect("Failed to create artboard node")
-					.default_node_template();
+					// Enable clipping by default (input index 5) so imported content is masked to the artboard bounds
+					.node_template_input_override([None, None, None, None, None, Some(NodeInput::value(TaggedValue::Bool(true), false))]);
 				responses.add(NodeGraphMessage::InsertNode {
 					node_id,
 					node_template: Box::new(new_artboard_node),
 				});
-				responses.add(NodeGraphMessage::ShiftNodePosition { node_id, x: 15, y: -3 });
+				let needs_content_transform = !content_shift.abs_diff_eq(DVec2::ZERO, 1e-6);
+				// With a content Transform node: shift by the layer indent plus the node width. Without: use just the layer indent.
+				responses.add(NodeGraphMessage::ShiftNodePosition {
+					node_id,
+					x: if needs_content_transform { LAYER_INDENT_OFFSET + NODE_CHAIN_WIDTH } else { LAYER_INDENT_OFFSET },
+					y: -3,
+				});
 				responses.add(GraphOperationMessage::ResizeArtboard {
 					layer: LayerNodeIdentifier::new_unchecked(node_id),
-					location: if place_artboard_at_origin { IVec2::ZERO } else { bounds[0].round().as_ivec2() },
-					dimensions: bounds_rounded_dimensions.as_ivec2(),
+					location: artboard_location,
+					dimensions: artboard_dimensions,
 				});
 
 				// Connect the current output data to the artboard's input data, and the artboard's output to the document output
@@ -1377,10 +1416,10 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 					insert_node_input_index: 1,
 				});
 
-				// Shift the content by half its width and height so it gets centered in the artboard
+				// Shift the content to align its top-left to the artboard's origin (no-op when content is already at origin)
 				responses.add(GraphOperationMessage::TransformChange {
 					layer: node_layer_id,
-					transform: DAffine2::from_translation(bounds_rounded_dimensions / 2.),
+					transform: DAffine2::from_translation(content_shift),
 					transform_in: TransformIn::Local,
 					skip_rerender: false,
 				});
@@ -1474,6 +1513,13 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
 }
 
 impl DocumentMessageHandler {
+	/// Translates a viewport mouse position to a document-space transform, or uses the viewport center if no mouse position is given.
+	fn document_transform_from_mouse(&self, mouse: Option<(f64, f64)>, viewport: &ViewportMessageHandler) -> DAffine2 {
+		let viewport_pos: DVec2 = mouse.map_or_else(|| viewport.center_in_viewport_space().into_dvec2() + viewport.offset().into_dvec2(), |pos| pos.into());
+		let document_to_viewport = self.navigation_handler.calculate_offset_transform(viewport.center_in_viewport_space().into(), &self.document_ptz);
+		DAffine2::from_translation(document_to_viewport.inverse().transform_point2(viewport_pos - viewport.offset().into_dvec2()))
+	}
+
 	/// Runs an intersection test with all layers and a viewport space quad
 	pub fn intersect_quad<'a>(&'a self, viewport_quad: graphene_std::renderer::Quad, viewport: &ViewportMessageHandler) -> impl Iterator<Item = LayerNodeIdentifier> + use<'a> {
 		let document_to_viewport = self.navigation_handler.calculate_offset_transform(viewport.center_in_viewport_space().into(), &self.document_ptz);

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

@@ -112,5 +112,7 @@ pub enum GraphOperationMessage {
 		transform: DAffine2,
 		parent: LayerNodeIdentifier,
 		insert_index: usize,
+		/// When true, centers the SVG at the transform origin (clipboard paste / drag-drop). When false, keeps natural SVG coordinates (file-open flow).
+		center: bool,
 	},
 }

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

@@ -321,6 +321,7 @@ impl MessageHandler<GraphOperationMessage, GraphOperationMessageContext<'_>> for
 				transform,
 				parent,
 				insert_index,
+				center,
 			} => {
 				let tree = match usvg::Tree::from_str(&svg, &usvg::Options::default()) {
 					Ok(t) => t,
@@ -334,21 +335,38 @@ impl MessageHandler<GraphOperationMessage, GraphOperationMessageContext<'_>> for
 				};
 				let mut modify_inputs = ModifyInputsContext::new(network_interface, responses);
 
-				let size = tree.size();
-				let offset_to_center = DVec2::new(size.width() as f64, size.height() as f64) / -2.;
-				let transform = transform * DAffine2::from_translation(offset_to_center);
+				// The placement transform positions the root group in document space.
+				// When centering (paste at cursor/viewport), shift so the SVG is centered at the transform origin.
+				// When not centering (file-open flow), content stays at viewport coordinates (usvg's viewBox mapping
+				// already places it in [0, width] × [0, height]); the artboard's X/Y handles the viewBox origin offset.
+				let mut placement_transform = if center {
+					// Center on the actual rendered content bounds rather than the viewbox size.
+					// An SVG may have a viewbox larger than its content, so using viewport_size/2 would place the cursor
+					// in that empty region instead of on the content.
+					let bounds = tree.root().abs_bounding_box();
+					let visual_center = DVec2::new((bounds.left() + bounds.right()) as f64 / 2., (bounds.top() + bounds.bottom()) as f64 / 2.);
+					transform * DAffine2::from_translation(-visual_center)
+				} else {
+					transform
+				};
+				placement_transform.translation = placement_transform.translation.round();
 
 				let graphite_gradient_stops = extract_graphite_gradient_stops(&svg);
 
+				// Pass identity so each leaf layer receives only its SVG-native transform from `abs_transform`.
+				// The placement offset is then applied once to the root group layer below.
 				import_usvg_node(
 					&mut modify_inputs,
 					&usvg::Node::Group(Box::new(tree.root().clone())),
-					transform,
 					id,
 					parent,
 					insert_index,
 					&graphite_gradient_stops,
 				);
+
+				// After import, `layer_node` is set to the root group. Apply the placement transform to it
+				// (skipped automatically when identity, so file-open with content at origin creates no Transform node).
+				modify_inputs.transform_set(placement_transform, TransformIn::Local, false);
 			}
 		}
 	}
@@ -452,7 +470,6 @@ fn parse_hex_stop_color(hex: &str, opacity: f32) -> Option<Color> {
 fn import_usvg_node(
 	modify_inputs: &mut ModifyInputsContext,
 	node: &usvg::Node,
-	transform: DAffine2,
 	id: NodeId,
 	parent: LayerNodeIdentifier,
 	insert_index: usize,
@@ -477,7 +494,7 @@ fn import_usvg_node(
 			modify_inputs.import = true;
 
 			for child in group.children() {
-				let extent = import_usvg_node_inner(modify_inputs, child, transform, NodeId::new(), layer, 0, graphite_gradient_stops, &mut group_extents_map);
+				let extent = import_usvg_node_inner(modify_inputs, child, NodeId::new(), layer, 0, graphite_gradient_stops, &mut group_extents_map);
 				child_extents_svg_order.push(extent);
 			}
 
@@ -496,7 +513,7 @@ fn import_usvg_node(
 			modify_inputs.network_interface.unload_all_nodes_bounding_box(&[]);
 		}
 		usvg::Node::Path(path) => {
-			import_usvg_path(modify_inputs, node, path, transform, layer, graphite_gradient_stops);
+			import_usvg_path(modify_inputs, node, path, layer, graphite_gradient_stops);
 		}
 		usvg::Node::Image(_image) => {
 			warn!("Skip image");
@@ -517,7 +534,6 @@ fn import_usvg_node(
 fn import_usvg_node_inner(
 	modify_inputs: &mut ModifyInputsContext,
 	node: &usvg::Node,
-	transform: DAffine2,
 	id: NodeId,
 	parent: LayerNodeIdentifier,
 	insert_index: usize,
@@ -532,7 +548,7 @@ fn import_usvg_node_inner(
 		usvg::Node::Group(group) => {
 			let mut child_extents: Vec<u32> = Vec::new();
 			for child in group.children() {
-				let extent = import_usvg_node_inner(modify_inputs, child, transform, NodeId::new(), layer, 0, graphite_gradient_stops, group_extents_map);
+				let extent = import_usvg_node_inner(modify_inputs, child, NodeId::new(), layer, 0, graphite_gradient_stops, group_extents_map);
 				child_extents.push(extent);
 			}
 			modify_inputs.layer_node = Some(layer);
@@ -547,7 +563,7 @@ fn import_usvg_node_inner(
 			total_extent
 		}
 		usvg::Node::Path(path) => {
-			import_usvg_path(modify_inputs, node, path, transform, layer, graphite_gradient_stops);
+			import_usvg_path(modify_inputs, node, path, layer, graphite_gradient_stops);
 			0
 		}
 		usvg::Node::Image(_image) => {
@@ -564,29 +580,26 @@ fn import_usvg_node_inner(
 }
 
 /// Helper to apply path data (vector geometry, fill, stroke, transform) to a layer.
-fn import_usvg_path(
-	modify_inputs: &mut ModifyInputsContext,
-	node: &usvg::Node,
-	path: &usvg::Path,
-	transform: DAffine2,
-	layer: LayerNodeIdentifier,
-	graphite_gradient_stops: &HashMap<String, GradientStops>,
-) {
+fn import_usvg_path(modify_inputs: &mut ModifyInputsContext, node: &usvg::Node, path: &usvg::Path, layer: LayerNodeIdentifier, graphite_gradient_stops: &HashMap<String, GradientStops>) {
 	let subpaths = convert_usvg_path(path);
 	let bounds = subpaths.iter().filter_map(|subpath| subpath.bounding_box()).reduce(Quad::combine_bounds).unwrap_or_default();
 
-	modify_inputs.insert_vector(subpaths, layer, true, path.fill().is_some(), path.stroke().is_some());
+	// Skip creating a Transform node entirely when the SVG-native transform is identity.
+	let node_transform = usvg_transform(node.abs_transform());
+	let has_transform = node_transform != DAffine2::IDENTITY;
+
+	modify_inputs.insert_vector(subpaths, layer, has_transform, path.fill().is_some(), path.stroke().is_some());
 
-	if let Some(transform_node_id) = modify_inputs.existing_network_node_id("Transform", true) {
-		transform_utils::update_transform(modify_inputs.network_interface, &transform_node_id, transform * usvg_transform(node.abs_transform()));
+	if has_transform && let Some(transform_node_id) = modify_inputs.existing_network_node_id("Transform", false) {
+		transform_utils::update_transform(modify_inputs.network_interface, &transform_node_id, node_transform);
 	}
 
 	if let Some(fill) = path.fill() {
 		let bounds_transform = DAffine2::from_scale_angle_translation(bounds[1] - bounds[0], 0., bounds[0]);
 		apply_usvg_fill(fill, modify_inputs, bounds_transform, graphite_gradient_stops);
 	}
 	if let Some(stroke) = path.stroke() {
-		apply_usvg_stroke(stroke, modify_inputs, transform * usvg_transform(node.abs_transform()));
+		apply_usvg_stroke(stroke, modify_inputs, node_transform);
 	}
 }