Split 'To Graphic' and 'Wrap Graphic' into the 'As Graphic' type assertion and 'Into Group' reducer nodes (#4441)

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

@@ -155,15 +155,15 @@ fn document_node_definitions() -> HashMap<DefinitionIdentifier, DocumentNodeDefi
 						// Primary (bottom) input type coercion
 						NodeTemplate {
 							inputs: vec![NodeInput::import(generic!(T), 0)],
-							implementation: NodeTemplateImplementation::ProtoNode(graphic::to_graphic::IDENTIFIER),
-							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(-21, -3)),
+							implementation: NodeTemplateImplementation::ProtoNode(graphic::as_graphic::IDENTIFIER),
+							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(-21, -2)),
 							..Default::default()
 						},
-						// Collection of the content into the layer's group; the wrap keeps
+						// Collection of the content into the layer's group; the group keeps
 						// the content level's element type for the legacy boundary.
 						NodeTemplate {
 							inputs: vec![NodeInput::import(generic!(T), 1)],
-							implementation: NodeTemplateImplementation::ProtoNode(graphic::wrap_graphic::IDENTIFIER),
+							implementation: NodeTemplateImplementation::ProtoNode(graphic::into_group::IDENTIFIER),
 							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(-21, -1)),
 							..Default::default()
 						},
@@ -171,7 +171,7 @@ fn document_node_definitions() -> HashMap<DefinitionIdentifier, DocumentNodeDefi
 						NodeTemplate {
 							inputs: vec![NodeInput::Reflection(graph_craft::document::DocumentNodeMetadata::DocumentNodePath)],
 							implementation: NodeTemplateImplementation::ProtoNode(graphic::path_of_subgraph::IDENTIFIER),
-							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(-21, 1)),
+							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(-21, 0)),
 							..Default::default()
 						},
 						// Stamp each item of the content with the parent layer's NodeId via the `editor:layer_path` attribute,
@@ -194,7 +194,7 @@ fn document_node_definitions() -> HashMap<DefinitionIdentifier, DocumentNodeDefi
 							call_argument: generic!(T),
 							inputs: vec![NodeInput::node(NodeId(0), 0), NodeInput::node(NodeId(4), 0)],
 							implementation: NodeTemplateImplementation::ProtoNode(list::extend::IDENTIFIER),
-							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(0, -3)),
+							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(0, -2)),
 							..Default::default()
 						},
 					]
@@ -269,7 +269,7 @@ fn document_node_definitions() -> HashMap<DefinitionIdentifier, DocumentNodeDefi
 						// Content coercion into a graphic level, evaluated within the artboard's footprint
 						NodeTemplate {
 							inputs: vec![NodeInput::import(generic!(T), 1)],
-							implementation: NodeTemplateImplementation::ProtoNode(graphic::to_graphic::IDENTIFIER),
+							implementation: NodeTemplateImplementation::ProtoNode(graphic::as_graphic::IDENTIFIER),
 							node_type_metadata: NodeTypePersistentMetadata::node(IVec2::new(-35, -3)),
 							..Default::default()
 						},

editor/src/messages/portfolio/document/storage_tests/test_support.rs

@@ -24,7 +24,9 @@ pub fn load_demo(file_name: &str) -> DocumentMessageHandler {
 #[test]
 #[ignore = "dev tool: set DEMO_OUT and run explicitly"]
 fn migrate_demo_artwork_into_demo_out() {
-	use crate::messages::portfolio::document_migration::{document_migration_reset_node_definition, document_migration_string_preprocessing, document_migration_upgrades};
+	use crate::messages::portfolio::document_migration::{
+		document_migration_reset_layer_definitions, document_migration_reset_node_definition, document_migration_string_preprocessing, document_migration_upgrades,
+	};
 	let out_dir = std::env::var("DEMO_OUT").expect("set DEMO_OUT to the output directory");
 	for entry in std::fs::read_dir("../demo-artwork").unwrap() {
 		let path = entry.unwrap().path();
@@ -34,8 +36,9 @@ fn migrate_demo_artwork_into_demo_out() {
 		let content = std::fs::read_to_string(&path).unwrap();
 		let content = document_migration_string_preprocessing(content);
 		let reset = document_migration_reset_node_definition(&content);
+		let reset_layers = document_migration_reset_layer_definitions(&content);
 		let mut document = DocumentMessageHandler::deserialize_document(&content).unwrap_or_else(|e| panic!("Failed to deserialize {}: {e:?}", path.display()));
-		document_migration_upgrades(&mut document, reset);
+		document_migration_upgrades(&mut document, reset, reset_layers);
 		let out = format!("{out_dir}/{}", path.file_name().unwrap().to_string_lossy());
 		std::fs::write(out, document.serialize_document()).unwrap();
 	}

editor/src/messages/portfolio/document_migration.rs

@@ -45,6 +45,14 @@ pub struct NodeReplacement<'a> {
 	aliases: &'a [&'a str],
 }
 
+/// Every name the Merge layer network's two type-coercion nodes have gone by, which is every alias of the node they both converged on.
+fn into_group_aliases() -> impl Iterator<Item = &'static &'static str> {
+	NODE_REPLACEMENTS
+		.iter()
+		.filter(|replacement| replacement.node == graphene_std::graphic::into_group::IDENTIFIER)
+		.flat_map(|replacement| replacement.aliases)
+}
+
 const NODE_REPLACEMENTS: &[NodeReplacement<'static>] = &[
 	// ================================
 	// blending
@@ -200,22 +208,20 @@ const NODE_REPLACEMENTS: &[NodeReplacement<'static>] = &[
 		],
 	},
 	NodeReplacement {
-		node: graphene_std::graphic::to_graphic::IDENTIFIER,
+		node: graphene_std::graphic::into_group::IDENTIFIER,
 		aliases: &[
-			"graphene_core::ToGraphicGroupNode",
-			"graphene_core::graphic_element::ToGroupNode",
-			"graphene_core::graphic_types::ToGroupNode",
-			"graphene_core::graphic::ToGraphicNode",
-		],
-	},
-	NodeReplacement {
-		node: graphene_std::graphic::wrap_graphic::IDENTIFIER,
-		aliases: &[
-			// Converted from "To Element"
+			// Converted from "To Element", then "Wrap Graphic"
 			"graphene_core::ToGraphicElementNode",
 			"graphene_core::graphic_element::ToElementNode",
 			"graphene_core::graphic_types::ToElementNode",
 			"graphene_core::graphic::WrapGraphicNode",
+			"graphic_nodes::graphic::WrapGraphicNode",
+			// Converted from "To Graphic", whose grouping of non-graphical content this node now carries alone
+			"graphene_core::ToGraphicGroupNode",
+			"graphene_core::graphic_element::ToGroupNode",
+			"graphene_core::graphic_types::ToGroupNode",
+			"graphene_core::graphic::ToGraphicNode",
+			"graphic_nodes::graphic::ToGraphicNode",
 		],
 	},
 	// ================================
@@ -1085,6 +1091,21 @@ pub fn document_migration_reset_node_definition(document_serialized_content: &st
 	false
 }
 
+/// Whether the layer networks built from the coercion nodes that became "As Graphic" and "Into Group" need their definitions reset.
+///
+/// A document still naming either node by an alias stores the pre-split plumbing, which the alias migration alone would mangle:
+/// it maps both names onto the reducer, leaving a reducer where the layer wants the assertion. Resetting those two layer
+/// definitions installs the current plumbing instead. This stays narrower than a full
+/// [`document_migration_reset_node_definition`], whose one-shot input reorderings would re-apply to documents already carrying them.
+pub fn document_migration_reset_layer_definitions(document_serialized_content: &str) -> bool {
+	into_group_aliases().any(|alias| document_serialized_content.contains(alias))
+}
+
+/// The layer networks whose definitions [`document_migration_reset_layer_definitions`] resets.
+fn is_coercion_layer_definition(reference: &DefinitionIdentifier) -> bool {
+	matches!(reference, DefinitionIdentifier::Network(name) if name == "Merge" || name == "Artboard")
+}
+
 pub fn document_migration_replace_resources_referenced_by_hash(document_serialized_content: String) -> (String, HashMap<ResourceHash, ResourceId>) {
 	fn collect_resources_referenced_by_hash(s: &str) -> HashMap<ResourceHash, Vec<Range<usize>>> {
 		let mut out: HashMap<ResourceHash, Vec<Range<usize>>> = HashMap::new();
@@ -1156,7 +1177,7 @@ pub fn document_migration_replace_resources_referenced_by_hash(document_serializ
 	(out, hash_to_id)
 }
 
-pub fn document_migration_upgrades(document: &mut DocumentMessageHandler, reset_node_definitions_on_open: bool) {
+pub fn document_migration_upgrades(document: &mut DocumentMessageHandler, reset_node_definitions_on_open: bool, reset_layer_definitions: bool) {
 	document.network_interface.migrate_path_modify_node();
 
 	let network = document.network_interface.document_network().clone();
@@ -1323,7 +1344,7 @@ pub fn document_migration_upgrades(document: &mut DocumentMessageHandler, reset_
 		.map(|(node_id, node, path)| (*node_id, node.clone(), path))
 		.collect::<Vec<(NodeId, graph_craft::document::DocumentNode, Vec<NodeId>)>>();
 	for (node_id, node, network_path) in &nodes {
-		migrate_node(node_id, node, network_path, document, reset_node_definitions_on_open);
+		migrate_node(node_id, node, network_path, document, reset_node_definitions_on_open, reset_layer_definitions);
 	}
 
 	// The old geometry-producing "Text" node was split into the current "Text" (`String[]`) -> converter pair, which reuses the same proto
@@ -1441,12 +1462,21 @@ fn fold_gradient_spread_into_ramp_input(input: &NodeInput, gradient_spread: Grad
 	}
 }
 
-fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId], document: &mut DocumentMessageHandler, reset_node_definitions_on_open: bool) -> Option<()> {
+fn migrate_node(
+	node_id: &NodeId,
+	node: &DocumentNode,
+	network_path: &[NodeId],
+	document: &mut DocumentMessageHandler,
+	reset_node_definitions_on_open: bool,
+	reset_layer_definitions: bool,
+) -> Option<()> {
 	// Must run before the reset block below: a node referencing a removed catalog entry would otherwise abort
 	// `migrate_node` via the `?` on `resolve_document_node_type`, preventing subsequent migration blocks from running.
 	migrate_removed_catalog_definitions(node_id, node, network_path, document);
 
-	if reset_node_definitions_on_open && let Some(reference) = document.network_interface.reference(node_id, network_path) {
+	if let Some(reference) = document.network_interface.reference(node_id, network_path)
+		&& (reset_node_definitions_on_open || (reset_layer_definitions && is_coercion_layer_definition(&reference)))
+	{
 		let node_definition = resolve_document_node_type(&reference)?;
 		document.network_interface.replace_implementation(node_id, network_path, &mut node_definition.default_node_template());
 
@@ -2986,6 +3016,25 @@ mod tests {
 		));
 	}
 
+	// Migrating a Merge network's coercion nodes by alias would leave a reducer in the primary slot, so every alias must reset instead
+	#[test]
+	fn every_into_group_alias_resets_the_merge_definition() {
+		let aliases = into_group_aliases().collect::<Vec<_>>();
+		assert!(!aliases.is_empty(), "the reset is driven by these aliases, so losing them all would disable it unnoticed");
+
+		for alias in aliases {
+			let document = format!(r#""implementation":{{"ProtoNode":"{alias}"}}"#);
+			assert!(
+				document_migration_reset_layer_definitions(&document),
+				"a document referencing `{alias}` should reset its layer definitions"
+			);
+			assert!(
+				!document_migration_reset_node_definition(&document),
+				"`{alias}` alone must not reset every definition, which would re-apply the one-shot input reorderings"
+			);
+		}
+	}
+
 	#[test]
 	fn test_no_duplicate_node_replacements() {
 		let mut hashmap = HashMap::<ProtoNodeIdentifier, u32>::new();

editor/src/messages/portfolio/portfolio_message_handler.rs

@@ -828,6 +828,8 @@ impl MessageHandler<PortfolioMessage, PortfolioMessageContext<'_>> for Portfolio
 			} => {
 				// Upgrade the document being opened to use fresh copies of all nodes
 				let reset_node_definitions_on_open = reset_node_definitions_on_open || document_migration_reset_node_definition(&document_serialized_content);
+				// Reinstall just the layer networks whose coercion nodes were split into "As Graphic" and "Into Group"
+				let reset_layer_definitions = document_migration_reset_layer_definitions(&document_serialized_content);
 				// Upgrade the document being opened with string replacements on the original JSON
 				let document_serialized_content = document_migration_string_preprocessing(document_serialized_content);
 				// Upgrade resources from being referend by hash to beeing referened by ID
@@ -878,7 +880,7 @@ impl MessageHandler<PortfolioMessage, PortfolioMessageContext<'_>> for Portfolio
 				};
 
 				// Upgrade the document's nodes to be compatible with the latest version
-				document_migration_upgrades(&mut document, reset_node_definitions_on_open);
+				document_migration_upgrades(&mut document, reset_node_definitions_on_open, reset_layer_definitions);
 
 				// Load the document's embedded resources into the resource storage
 				std::mem::take(&mut document.resources.embedded).into_iter().for_each(|(hash, resource)| {