Use ResourceId for referring to Resources

[timon-schelling]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces a migration from referencing resources by hash to referencing them by a unique ID (ResourceId), backed by a new ResourceRegistry to manage resource sources and garbage collection. Key feedback focuses on making the JSON migration parser more robust against pretty-printed or multi-field objects, removing a strict check that could cause deserialization failures on documents with missing embedded resources, and optimizing the resource garbage collection pipeline by passing HashSet references directly to avoid redundant conversions and allocations.

[gemini-code-assist]

used_resources collects the used resource IDs into a HashSet, but then converts it to a Vec and then a Box<[ResourceId]>. When this is passed to garbage_collect, the first thing garbage_collect does is collect it back into a HashSet.

This results in redundant allocations and conversions (HashSet -> Vec -> Box -> HashSet). Returning HashSet<ResourceId> directly from used_resources and updating garbage_collect to accept &HashSet<ResourceId> avoids these redundant operations.

Suggested change

	pub fn used_resources(&self, include_history: bool) -> Box<[ResourceId]> {
	let mut resources = HashSet::new();
	self.network_interface.collect_used_resources(&mut resources);
	if include_history {
	self.document_undo_history.iter().for_each(|interface| interface.collect_used_resources(&mut resources));
	self.document_redo_history.iter().for_each(|interface| interface.collect_used_resources(&mut resources));
	}
	resources
	resources.into_iter().collect::<Vec<_>>().into_boxed_slice()
	}
	pub fn used_resources(&self, include_history: bool) -> HashSet<ResourceId> {
	let mut resources = HashSet::new();
	self.network_interface.collect_used_resources(&mut resources);
	if include_history {
	self.document_undo_history.iter().for_each(|interface| interface.collect_used_resources(&mut resources));
	self.document_redo_history.iter().for_each(|interface| interface.collect_used_resources(&mut resources));
	}
	resources
	}

[gemini-code-assist]

If used_resources is updated to return HashSet<ResourceId>, we should update this call to pass &used_resources instead of used_resources.as_ref() to match the updated garbage_collect signature.

Suggested change

	let used_resources = self.used_resources(false);
	let embedded_resource_hashes = Vec::from_iter(used_resources.iter().filter_map(|id| match self.resources.registry.info(id) {
	Some(ResourceInfo { hash: Some(hash), sources, .. }) if sources.contains(&DataSource::Embedded) => Some(hash),
	_ => None,
	}))
	.into_boxed_slice();
	let mut document = self.clone();
	document.resources.registry.garbage_collect(used_resources.as_ref());
	let used_resources = self.used_resources(false);
	let embedded_resource_hashes = Vec::from_iter(used_resources.iter().filter_map(|id| match self.resources.registry.info(id) {
	Some(ResourceInfo { hash: Some(hash), sources, .. }) if sources.contains(&DataSource::Embedded) => Some(hash),
	_ => None,
	}))
	.into_boxed_slice();
	let mut document = self.clone();
	document.resources.registry.garbage_collect(&used_resources);

[gemini-code-assist]

Accepting &HashSet<ResourceId> directly in garbage_collect avoids the need to re-collect a slice into a HashSet on every call, eliminating redundant allocations and hashing operations.

	pub fn garbage_collect(&mut self, used: &std::collections::HashSet<ResourceId>) {
		self.sources.retain(|id, _| used.contains(id));
		self.hashes.retain(|id, _| used.contains(id));
	}

[cubic-dev-ai]

2 issues found across 30 files

Confidence score: 2/5

• There is a high-confidence regression risk: in node-graph/graph-craft/src/document/value.rs, switching TaggedValue::Resource from ResourceHash to ResourceId without a legacy "Resource" migration is likely to break deserialization of existing documents.
• editor/src/node_graph_executor.rs has a concrete runtime correctness risk because GraphUpdate is still gated by network/inspect hash checks and can miss resource changes, leaving stale resources in use.
• Given one severe backward-compatibility break plus another medium-to-high runtime staleness issue, this looks risky to merge until both behaviors are addressed.
• Pay close attention to node-graph/graph-craft/src/document/value.rs and editor/src/node_graph_executor.rs - deserialization compatibility and resource-change invalidation are the key risk areas.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="node-graph/graph-craft/src/document/value.rs">

<violation number="1" location="node-graph/graph-craft/src/document/value.rs:399">
P1: Changing `TaggedValue::Resource` from `ResourceHash` to `ResourceId` without a legacy `"Resource"` migration breaks backward deserialization for existing documents.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: Changing TaggedValue::Resource from ResourceHash to ResourceId without a legacy "Resource" migration breaks backward deserialization for existing documents.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/graph-craft/src/document/value.rs, line 399:

<comment>Changing `TaggedValue::Resource` from `ResourceHash` to `ResourceId` without a legacy `"Resource"` migration breaks backward deserialization for existing documents.</comment>

<file context>
@@ -396,7 +396,8 @@ tagged_value! {
 	VectorModification(Box<VectorModification>),
 	ImageData(Image<Color>),
-	Resource(graphene_application_io::resource::ResourceHash),
+	Resource(graphene_application_io::resource::ResourceId),
+	ResourceHash(graphene_application_io::resource::ResourceHash),
 	// ==========
</file context>