From fb268bb6752cf400717d25c31ddfc21b9cc1fbd1 Mon Sep 17 00:00:00 2001 From: Dennis Kobert Date: Tue, 22 Sep 2026 12:11:04 +0200 Subject: [PATCH 1/2] Make gdd based undo syncronous by caching protonode declarations --- document/format/src/lib.rs | 24 ++------ document/format/src/persist.rs | 10 ++-- document/graph-storage/src/from_runtime.rs | 27 +++------ document/graph-storage/src/session.rs | 43 ++++++++++++-- .../graph-storage/src/tests/round_trip.rs | 8 +-- .../portfolio/document/document_history.rs | 59 ++++++++++++------- .../document/document_message_handler.rs | 44 ++++++-------- .../document/storage_tests/metadata_tests.rs | 4 +- .../storage_tests/round_trip_tests.rs | 4 +- .../messages/portfolio/document_storage_io.rs | 35 +++-------- .../messages/portfolio/portfolio_message.rs | 24 +++----- .../portfolio/portfolio_message_handler.rs | 26 ++++---- 12 files changed, 144 insertions(+), 164 deletions(-) diff --git a/document/format/src/lib.rs b/document/format/src/lib.rs index 027a7622280..7b5797bbde3 100644 --- a/document/format/src/lib.rs +++ b/document/format/src/lib.rs @@ -292,34 +292,22 @@ impl Gdd { (working, self.layout) } - /// Resolve the proto-node declarations referenced by the registry into a [`document_graph_storage::Declarations`] - /// map, loading each `ProtoNode`'s bytes from `byte_store` (the global cache in the editor, the - /// working-copy container for standalone). Only resources referenced by `Implementation::ProtoNode` - /// are visited, so image/font resources are skipped. Cold-path (open / `to_runtime`); async - /// because resource loads are. + /// Resolve every proto-node declaration referenced by the registry or its history into a + /// [`document_graph_storage::Declarations`] map, loading each `ProtoNode`'s bytes from `byte_store` + /// (the global cache in the editor, the working-copy container for standalone). Covers history so + /// the map can serve any undo/redo target. Cold-path (mount / open); async because resource loads are. #[cfg(feature = "conversion")] pub async fn declarations(&self, byte_store: &dyn LoadResource) -> document_graph_storage::Declarations { - use document_graph_storage::Implementation; - - let registry = self.session.registry(); let mut declarations = document_graph_storage::Declarations::new(); - for node in registry.node_instances.values() { - let Implementation::ProtoNode(id) = node.implementation() else { continue }; - if declarations.contains_key(id) { - continue; - } - let Some(hash) = registry.resources.get(id).and_then(|entry| entry.hash) else { - log::error!("Declaration resource {id} has no resolved hash; cannot load ProtoNode"); - continue; - }; + for (id, hash) in self.session.all_declaration_resources() { let Some(resource) = byte_store.load(hash).await else { log::error!("Declaration bytes for {id} (hash {hash}) missing from byte store"); continue; }; match document_graph_storage::decode_declaration(resource.as_ref()) { Ok(proto) => { - declarations.insert(*id, proto); + declarations.insert(id, proto); } Err(error) => log::error!("Failed to deserialize ProtoNode for {id}: {error}"), } diff --git a/document/format/src/persist.rs b/document/format/src/persist.rs index a4c4b5476c4..f6ceba85e02 100644 --- a/document/format/src/persist.rs +++ b/document/format/src/persist.rs @@ -47,7 +47,7 @@ impl Gdd { /// declaration bytes. The working registry reflects the edit immediately, but nothing enters durable /// retired history until [`retire_pending_interaction`](Self::retire_pending_interaction). Staging on /// every edit while retiring only at interaction boundaries lets several edits coalesce into one retired - /// interaction. + /// interaction. Returns the decoded declarations the snapshot references, for the caller's cache. /// /// # Errors /// [`Error::Commit`] if the runtime diff is rejected by the session. On an [`Error::Container`] / @@ -60,8 +60,8 @@ impl Gdd { metadata: &M, resources: &graphene_resource::ResourceRegistry, byte_store: &dyn ResourceStorage, - ) -> Result<(), Error> { - let (hot_ops, declaration_bytes) = self.session.stage_from_runtime(network, metadata, resources)?; + ) -> Result { + let (hot_ops, conversion) = self.session.stage_from_runtime(network, metadata, resources)?; for hot_op in &hot_ops { self.append_hot_frame(hot_op)?; @@ -70,10 +70,10 @@ impl Gdd { // Persist proto-node declaration content to the byte store (the global cache in the editor, // the working-copy container for standalone export). Content-addressed, so re-storing // identical bytes on every commit is an idempotent no-op. - for bytes in declaration_bytes.values() { + for bytes in conversion.declaration_bytes.values() { byte_store.store(bytes); } - Ok(()) + Ok(conversion.declarations) } /// Retire every pending hot op into durable history as a single interaction (marking the batch's last diff --git a/document/graph-storage/src/from_runtime.rs b/document/graph-storage/src/from_runtime.rs index 110f60200cc..f820eb969ab 100644 --- a/document/graph-storage/src/from_runtime.rs +++ b/document/graph-storage/src/from_runtime.rs @@ -89,33 +89,17 @@ impl TryFrom<&NodeNetwork> for Registry { pub type DeclarationBytes = HashMap>; /// A `from_runtime` conversion result: the reference-only [`Registry`] plus the proto-node -/// declaration *bytes* it extracted, keyed by content hash. `document-graph-storage` doesn't own a byte -/// store, so the caller (the `Gdd`) persists these into its content store; the registry only holds -/// the `ResourceId`/`ResourceHash` references. +/// declarations it extracted, as bytes keyed by content hash (for the caller's byte store) and as +/// decoded [`ProtoNode`]s keyed by id (for the caller's declaration cache). pub struct RuntimeConversion { pub registry: Registry, pub declaration_bytes: DeclarationBytes, + pub declarations: crate::Declarations, /// Each network's runtime `metadata_path` mapped to its stable storage `NetworkId`, for associating /// per-network, per-peer view state (`session.json`) without re-deriving ids. pub network_ids: HashMap, NetworkId>, } -impl RuntimeConversion { - /// Rebuild the [`Declarations`](crate::Declarations) map (`ResourceId` → [`ProtoNode`]) from the - /// extracted bytes, for callers that keep the bytes in hand instead of routing them through a - /// byte store (tests, the round-trip CLI). Editor/`Gdd` paths persist the bytes and resolve via - /// their byte store instead. - pub fn declarations(&self) -> Result { - self.declaration_bytes - .iter() - .map(|(hash, bytes)| { - let proto = decode_declaration(bytes).map_err(|error| ConversionError::SerializationError(format!("declaration {hash}: {error}")))?; - Ok((ResourceId::from_hash(hash), proto)) - }) - .collect() - } -} - /// Encode a [`ProtoNode`] declaration to its content-addressed bytes: through a self-describing /// `serde_json::Value` (so serde aliases keep working and the on-disk shape stays migratable), then /// rmp-serialized (which encodes the intermediate `Value` compactly). Paired with [`decode_declaration`]. @@ -149,6 +133,7 @@ impl Registry { let mut ctx = ConversionContext { declaration_ids: HashMap::new(), declaration_bytes: HashMap::new(), + declarations: HashMap::new(), network_ids: HashMap::new(), metadata, peer, @@ -167,6 +152,7 @@ impl Registry { Ok(RuntimeConversion { registry, declaration_bytes: ctx.declaration_bytes, + declarations: ctx.declarations, network_ids: ctx.network_ids, }) } @@ -254,6 +240,8 @@ struct ConversionContext<'m, M: NodeMetadataSource + ?Sized> { declaration_ids: HashMap, /// Extracted declaration content keyed by hash, handed back for the caller's byte store. declaration_bytes: DeclarationBytes, + /// The same declarations decoded, keyed by id, handed back for the caller's declaration cache. + declarations: crate::Declarations, /// Maps each network's runtime `metadata_path` to its stable storage `NetworkId`, so the caller can /// associate per-network, per-peer view state (in `session.json`) with networks without re-deriving ids. network_ids: HashMap, NetworkId>, @@ -555,6 +543,7 @@ fn convert_implementation( register_declaration_resource(registry, id, hash, ctx.peer); ctx.declaration_bytes.insert(hash, bytes); + ctx.declarations.insert(id, proto); ctx.declaration_ids.insert(identifier_str, id); Implementation::ProtoNode(id) diff --git a/document/graph-storage/src/session.rs b/document/graph-storage/src/session.rs index 8edfee51ca3..e276003d99d 100644 --- a/document/graph-storage/src/session.rs +++ b/document/graph-storage/src/session.rs @@ -2,7 +2,7 @@ use crate::NodeMetadataSource; #[cfg(any(feature = "conversion", test))] use crate::from_runtime; -use crate::{ApplyMode, Delta, Document, History, LamportClock, NetworkId, NodeId, PeerId, Registry, RegistryDelta, RegistryTarget, ResourceEntry, Rev, TimeStamp, UserId}; +use crate::{ApplyMode, Delta, Document, History, Implementation, LamportClock, NetworkId, NodeId, PeerId, Registry, RegistryDelta, RegistryTarget, ResourceEntry, Rev, TimeStamp, UserId}; use graphene_resource::{ResourceHash, ResourceId}; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; @@ -64,8 +64,7 @@ impl Session { /// Diff the current registry against a fresh conversion of `network`, then commit each emitted /// op as its own `Delta` on the local chain. One `clock.tick()` per op (strictly causal within /// a commit). Returns the new `Rev`s in commit order (empty if nothing changed) plus the - /// proto-node declaration bytes the conversion extracted, keyed by content hash, for the caller - /// to persist into its byte store (`document-graph-storage` itself is byte-unaware). + /// conversion, whose extracted declarations the caller persists and caches. /// /// Stages the diff as hot ops rather than retired deltas: each op is applied to the registry and /// pushed onto the hot log. The caller persists the returned hot frames and then calls `retire` @@ -76,11 +75,11 @@ impl Session { network: &graph_craft::document::NodeNetwork, metadata: &M, resources: &graphene_resource::ResourceRegistry, - ) -> Result<(Vec, from_runtime::DeclarationBytes), CommitError> { + ) -> Result<(Vec, from_runtime::RuntimeConversion), CommitError> { let conversion = Registry::convert_from_runtime(network, metadata, resources, self.document.peer)?; let ops = crate::delta::compute_deltas(&self.document.working_registry, &conversion.registry); let hot_ops = self.stage_ops(ops)?; - Ok((hot_ops, conversion.declaration_bytes)) + Ok((hot_ops, conversion)) } /// Resolve each runtime `network_path` to its stable [`NetworkId`] for this document's peer, so the @@ -473,6 +472,40 @@ impl Session { hashes } + /// Every proto-node declaration resource referenced by the current registry or anywhere in history, + /// with its content hash. + pub fn all_declaration_resources(&self) -> HashMap { + let registry = &self.document.working_registry; + + let mut hashes: HashMap = registry.resources.iter().filter_map(|(id, entry)| Some((*id, entry.hash?))).collect(); + for delta in self.document.history.iter() { + match &delta.kind { + RegistryDelta::AddResource { id, entry } => hashes.extend(entry.hash.map(|hash| (*id, hash))), + RegistryDelta::RemoveResource { id, snapshot } => hashes.extend(snapshot.hash.map(|hash| (*id, hash))), + RegistryDelta::SetResourceHash { id, hash: Some(hash) } => { + hashes.insert(*id, *hash); + } + _ => {} + } + } + + let current_nodes = registry.node_instances.values(); + let historic_nodes = self.document.history.iter().filter_map(|delta| match &delta.kind { + RegistryDelta::AddNode { node, .. } => Some(node), + RegistryDelta::RemoveNode { snapshot, .. } => Some(snapshot), + _ => None, + }); + + current_nodes + .chain(historic_nodes) + .filter_map(|node| match node.implementation() { + Implementation::ProtoNode(id) => Some(*id), + Implementation::Network(_) => None, + }) + .filter_map(|id| Some((id, *hashes.get(&id)?))) + .collect() + } + pub fn hot_log(&self) -> &[HotOp] { &self.document.hot_log } diff --git a/document/graph-storage/src/tests/round_trip.rs b/document/graph-storage/src/tests/round_trip.rs index 2fa8af06840..0175bc7afbb 100644 --- a/document/graph-storage/src/tests/round_trip.rs +++ b/document/graph-storage/src/tests/round_trip.rs @@ -20,12 +20,10 @@ fn verify_network_compiles(network: &NodeNetwork) -> Result<(), String> { /// Convert a runtime network to a storage `Registry`, returning the declarations alongside it. /// Proto-node declaration content is no longer stored in the registry (it lives in a byte store); -/// these tests have no byte store, so they keep the extracted bytes in hand and rebuild a -/// `Declarations` map for the back-conversion. +/// these tests have no byte store, so they keep the extracted `Declarations` in hand for the back-conversion. fn to_registry(network: &NodeNetwork) -> (Registry, crate::Declarations) { let conversion = Registry::convert_from_runtime(network, &crate::NoMetadata, &Default::default(), PeerId(0)).expect("Failed to convert NodeNetwork to Registry"); - let declarations = conversion.declarations().expect("rebuild declarations"); - (conversion.registry, declarations) + (conversion.registry, conversion.declarations) } /// A one-node network whose single node references `id` via a `TaggedValue::Resource` input, so @@ -477,7 +475,7 @@ fn test_ui_metadata_round_trip() { ); let conversion = Registry::convert_from_runtime(&network, &metadata, &Default::default(), PeerId(0)).expect("Failed to convert to Registry with metadata"); - let declarations = conversion.declarations().expect("rebuild declarations"); + let declarations = conversion.declarations; let registry = conversion.registry; let (converted, entries) = registry.to_runtime_with_metadata(&declarations).expect("Failed to convert Registry back with metadata"); diff --git a/editor/src/messages/portfolio/document/document_history.rs b/editor/src/messages/portfolio/document/document_history.rs index 9758fd38467..f2957f9ea98 100644 --- a/editor/src/messages/portfolio/document/document_history.rs +++ b/editor/src/messages/portfolio/document/document_history.rs @@ -1,11 +1,11 @@ use std::collections::VecDeque; use std::collections::{BTreeMap, HashSet}; -use document_graph_storage::Registry; +use document_graph_storage::{Declarations, Registry}; use graph_craft::application_io::resource::{ResourceId, ResourceRegistry, ResourceStorage}; use super::utility_types::network_interface::NodeNetworkInterface; -use super::utility_types::network_interface::storage_metadata::{StorageMetadataView, collect_network_view_settings}; +use super::utility_types::network_interface::storage_metadata::{StorageMetadataView, build_interface_from_storage, collect_network_view_settings}; /// Per-document undo/redo state: the legacy snapshot stacks plus the `Gdd` working-copy cursor that is /// becoming the authoritative history. Owns the dual-stack bookkeeping push/pop/clear and the cursor's stage/retire/move/verify @@ -24,6 +24,10 @@ pub struct DocumentHistory { /// future built by `load_document` resolves. #[derivative(Debug = "ignore")] storage: Option, + /// Decoded proto-node declarations for every registry state the cursor can reach, filled at mount + /// and extended on each staging, so a cursor rebuild never touches the byte store. + #[derivative(Debug = "ignore")] + declarations: Declarations, } impl DocumentHistory { @@ -79,9 +83,10 @@ impl DocumentHistory { self.storage.as_mut() } - /// Attach (or clear) the `Gdd` working copy once the mount future resolves. - pub fn set_storage(&mut self, storage: Option) { - self.storage = storage; + /// Attach the `Gdd` working copy once the mount future resolves, with the declarations it references. + pub fn set_storage(&mut self, storage: document_format::GddV1, declarations: Declarations) { + self.storage = Some(storage); + self.declarations = declarations; } /// Retire the pending staged hot ops into durable Gdd history as one undo unit. Called at each undo-step @@ -119,9 +124,12 @@ impl DocumentHistory { // Stage without retiring: a tool drag fires several `CommitTransaction`s but is one legacy undo // step, so the deltas accumulate as hot ops and coalesce at the next undo-step boundary. - if let Err(error) = storage.stage_runtime_snapshot(network, &metadata_view, registry, byte_store) { - log::error!("Storage snapshot staging failed: {error}"); - return; + match storage.stage_runtime_snapshot(network, &metadata_view, registry, byte_store) { + Ok(declarations) => self.declarations.extend(declarations), + Err(error) => { + log::error!("Storage snapshot staging failed: {error}"); + return; + } } if let Err(error) = storage.set_view_settings(view_settings) { @@ -142,10 +150,9 @@ impl DocumentHistory { } /// Move the `Gdd` undo/redo cursor along the retired interaction chain, flushing any open interaction - /// first. Returns a clone of the post-move `Gdd` (`Arc`-shared) so a `'static` rebuild future can read - /// the rewound state while the live document keeps its cursor. `None` when there is nothing to move to, - /// unmounted, or the move failed. - pub fn move_cursor(&mut self, undo: bool) -> Option { + /// first, and rebuild the interface from the rewound registry using the declaration cache. `None` when + /// there is nothing to move to, unmounted, or the move failed. A failed rebuild moves the cursor back. + pub fn move_cursor(&mut self, undo: bool) -> Option { self.retire_storage_interaction(); let storage = self.storage.as_mut()?; @@ -166,7 +173,23 @@ impl DocumentHistory { return None; } - Some(storage.clone()) + let rebuilt = storage + .registry() + .to_runtime_with_full_metadata(&self.declarations) + .map_err(|error| error.to_string()) + .and_then(|(network, node_entries, network_entries)| build_interface_from_storage(network, node_entries, network_entries).map_err(|error| error.to_string())); + + match rebuilt { + Ok(interface) => Some(interface), + Err(error) => { + log::error!("Storage undo/redo rebuild failed, reverting cursor move: {error}"); + let reverted = if undo { storage.redo() } else { storage.undo() }; + if let Err(error) = reverted { + log::error!("Storage undo/redo cursor revert failed: {error}"); + } + None + } + } } // Soak round-trip verification (runtime-gated by `validate_storage_round_trip`) @@ -193,13 +216,7 @@ impl DocumentHistory { } }; let target = &conversion.registry; - let declarations = match conversion.declarations() { - Ok(declarations) => declarations, - Err(error) => { - log::error!("storage round-trip: declaration rebuild failed: {error}"); - return; - } - }; + let declarations = &conversion.declarations; let stored = storage.registry(); if !stored.value_equal(target) { @@ -213,7 +230,7 @@ impl DocumentHistory { panic!("storage round-trip: timestamp order inconsistent between stored and target"); } - let (round_tripped, _entries) = match stored.to_runtime_with_metadata(&declarations) { + let (round_tripped, _entries) = match stored.to_runtime_with_metadata(declarations) { Ok(result) => result, Err(error) => { log::error!("storage round-trip: to_runtime failed: {error}"); diff --git a/editor/src/messages/portfolio/document/document_message_handler.rs b/editor/src/messages/portfolio/document/document_message_handler.rs index 2520d760396..fe262501ed3 100644 --- a/editor/src/messages/portfolio/document/document_message_handler.rs +++ b/editor/src/messages/portfolio/document/document_message_handler.rs @@ -32,6 +32,7 @@ use crate::messages::tool::tool_messages::select_tool::SelectToolPointerKeys; use crate::messages::tool::tool_messages::tool_prelude::Key; use crate::messages::tool::utility_types::ToolType; use crate::node_graph_executor::NodeGraphExecutor; +use document_graph_storage::Declarations; use glam::{DAffine2, DVec2}; use graph_craft::application_io::resource::ResourceId; use graph_craft::application_io::wgpu_available; @@ -406,8 +407,8 @@ impl MessageHandler> for DocumentMes responses.add(NodeGraphMessage::SelectedNodesSet { nodes: vec![] }); self.layer_range_selection_reference = None; } - DocumentMessage::DocumentHistoryBackward => self.undo_with_history(document_id, viewport, resource_storage, responses), - DocumentMessage::DocumentHistoryForward => self.redo_with_history(document_id, viewport, resource_storage, responses), + DocumentMessage::DocumentHistoryBackward => self.undo_with_history(viewport, preferences.validate_storage_round_trip, responses), + DocumentMessage::DocumentHistoryForward => self.redo_with_history(viewport, preferences.validate_storage_round_trip, responses), DocumentMessage::DocumentStructureChanged => { if layers_panel_open { self.network_interface.load_structure(); @@ -1808,7 +1809,7 @@ impl MessageHandler> for DocumentMes impl DocumentMessageHandler { /// Build a document handler from a `.gdd` working copy. - pub fn from_storage(interface: NodeNetworkInterface, storage: document_format::GddV1, name: String, path: Option) -> Self { + pub fn from_storage(interface: NodeNetworkInterface, storage: document_format::GddV1, declarations: Declarations, name: String, path: Option) -> Self { let mut document = Self { network_interface: interface, name, @@ -1821,7 +1822,7 @@ impl DocumentMessageHandler { Ok(resource_registry) => document.resources.registry = resource_registry, Err(error) => log::error!("Opening .gdd: failed to rebuild resource registry: {error}"), } - document.history.set_storage(Some(storage)); + document.history.set_storage(storage, declarations); document } @@ -2016,9 +2017,9 @@ impl DocumentMessageHandler { self.history.storage_mut() } - /// Attach (or clear) the `Gdd` working copy once the mount future resolves. - pub fn set_storage(&mut self, storage: Option) { - self.history.set_storage(storage); + /// Attach the `Gdd` working copy once the mount future resolves, with the declarations it references. + pub fn set_storage(&mut self, storage: document_format::GddV1, declarations: Declarations) { + self.history.set_storage(storage, declarations); } /// Retire the pending staged hot ops into durable Gdd history as one undo unit. @@ -2082,24 +2083,15 @@ impl DocumentMessageHandler { } } - /// Move the `Gdd` undo/redo cursor and spawn the async future that rebuilds the - /// interface from the cursor and swaps it in. `had_oracle` records whether the legacy snapshot already - /// applied, so the completion can compare; it travels with the spawned message. Returns whether the - /// cursor moved, so callers know a rebuild is pending. - fn drive_storage_undo_redo(&mut self, document_id: DocumentId, resource_storage: &ResourceStorageMessageHandler, had_oracle: bool, undo: bool, responses: &mut VecDeque) -> bool { - let Some(gdd) = self.history.move_cursor(undo) else { return false }; - - responses.add(crate::messages::portfolio::document_storage_io::rebuild_gdd_cursor( - gdd, - resource_storage.resources_mut(), - document_id, - had_oracle, - )); - true + /// Move the `Gdd` undo/redo cursor and swap in the interface rebuilt from it. `had_oracle` records + /// whether the legacy snapshot already applied, so the rebuild can be compared against it. + fn drive_storage_undo_redo(&mut self, had_oracle: bool, undo: bool, validate: bool, responses: &mut VecDeque) { + let Some(rebuilt) = self.history.move_cursor(undo) else { return }; + self.apply_gdd_cursor_rebuild(rebuilt, had_oracle, validate, responses); } /// Swap in the interface rebuilt from the `Gdd` cursor. Always overwrites the interface. - pub(crate) fn apply_gdd_cursor_rebuild(&mut self, mut rebuilt: NodeNetworkInterface, had_oracle: bool, validate: bool, responses: &mut VecDeque) { + fn apply_gdd_cursor_rebuild(&mut self, mut rebuilt: NodeNetworkInterface, had_oracle: bool, validate: bool, responses: &mut VecDeque) { rebuilt.copy_all_transient_view_state(&self.network_interface); std::mem::swap(&mut rebuilt.resolved_types, &mut self.network_interface.resolved_types); rebuilt.load_structure(); @@ -2410,7 +2402,7 @@ impl DocumentMessageHandler { paths } - pub fn undo_with_history(&mut self, document_id: DocumentId, viewport: &ViewportMessageHandler, resource_storage: &ResourceStorageMessageHandler, responses: &mut VecDeque) { + pub fn undo_with_history(&mut self, viewport: &ViewportMessageHandler, validate: bool, responses: &mut VecDeque) { let legacy_applied = if let Some(previous_network) = self.undo(viewport, responses) { self.history.push_redo(previous_network); true @@ -2418,7 +2410,7 @@ impl DocumentMessageHandler { false }; - self.drive_storage_undo_redo(document_id, resource_storage, legacy_applied, true, responses); + self.drive_storage_undo_redo(legacy_applied, true, validate, responses); } /// Installs a history snapshot as the active network interface, carrying over the current view state and structure load, and returns the replaced interface. @@ -2452,7 +2444,7 @@ impl DocumentMessageHandler { Some(previous_network) } - pub fn redo_with_history(&mut self, document_id: DocumentId, viewport: &ViewportMessageHandler, resource_storage: &ResourceStorageMessageHandler, responses: &mut VecDeque) { + pub fn redo_with_history(&mut self, viewport: &ViewportMessageHandler, validate: bool, responses: &mut VecDeque) { let legacy_applied = if let Some(previous_network) = self.redo(viewport, responses) { self.history.push_undo(previous_network); true @@ -2460,7 +2452,7 @@ impl DocumentMessageHandler { false }; - self.drive_storage_undo_redo(document_id, resource_storage, legacy_applied, false, responses); + self.drive_storage_undo_redo(legacy_applied, false, validate, responses); } pub fn redo(&mut self, viewport: &ViewportMessageHandler, responses: &mut VecDeque) -> Option { diff --git a/editor/src/messages/portfolio/document/storage_tests/metadata_tests.rs b/editor/src/messages/portfolio/document/storage_tests/metadata_tests.rs index 1c0b2a31f08..5d794d119fa 100644 --- a/editor/src/messages/portfolio/document/storage_tests/metadata_tests.rs +++ b/editor/src/messages/portfolio/document/storage_tests/metadata_tests.rs @@ -28,7 +28,7 @@ fn editor_metadata_round_trip_against_demo() { let network = interface.document_network().clone(); let conversion = Registry::convert_from_runtime(&network, &source, &Default::default(), PeerId(0)).expect("convert_from_runtime failed"); - let declarations = conversion.declarations().expect("rebuild declarations"); + let declarations = conversion.declarations; let registry = conversion.registry; let (_converted_network, entries) = registry.to_runtime_with_metadata(&declarations).expect("to_runtime_with_metadata failed"); @@ -153,7 +153,7 @@ fn editor_interface_rebuild_round_trip() { let network = original.document_network().clone(); let conversion = Registry::convert_from_runtime(&network, &original_view, &Default::default(), PeerId(0)).expect("convert_from_runtime failed"); - let declarations = conversion.declarations().expect("rebuild declarations"); + let declarations = conversion.declarations; let registry = conversion.registry; let (rebuilt_network, node_entries, network_entries) = registry.to_runtime_with_full_metadata(&declarations).expect("to_runtime_with_full_metadata failed"); diff --git a/editor/src/messages/portfolio/document/storage_tests/round_trip_tests.rs b/editor/src/messages/portfolio/document/storage_tests/round_trip_tests.rs index 0f9255ea857..5c6e85a597b 100644 --- a/editor/src/messages/portfolio/document/storage_tests/round_trip_tests.rs +++ b/editor/src/messages/portfolio/document/storage_tests/round_trip_tests.rs @@ -176,7 +176,7 @@ async fn edit_after_open_commits_cleanly() { { let document = editor.active_document_mut(); document.network_interface = rebuilt; - document.set_storage(Some(reopened)); + document.set_storage(reopened, declarations); document.finalize_storage_load(); } @@ -678,7 +678,7 @@ async fn mount_in_memory_storage(editor: &mut EditorTestUtils) -> HashMapResourc let gdd = GddV1::create_in(AnyContainer::Memory(MemoryBackend::new()), GddV1Layout, PeerId(1), 0x5EED, "test".into(), "test".into()) .await .expect("create_in"); - editor.active_document_mut().set_storage(Some(gdd)); + editor.active_document_mut().set_storage(gdd, Default::default()); HashMapResourceStorage::new() } diff --git a/editor/src/messages/portfolio/document_storage_io.rs b/editor/src/messages/portfolio/document_storage_io.rs index 7389bd14fc8..d68d65c8837 100644 --- a/editor/src/messages/portfolio/document_storage_io.rs +++ b/editor/src/messages/portfolio/document_storage_io.rs @@ -1,11 +1,12 @@ //! Asynchronous `.gdd` working-copy IO, spawned by `PortfolioMessageHandler` as `FutureMessage`s: -//! building/opening containers, opening `.gdd` archives into documents, and rebuilding the interface -//! from the undo/redo cursor. The `validate` flag is the `validate_storage_round_trip` preference; when +//! building/opening containers and opening `.gdd` archives into documents. +//! The `validate` flag is the `validate_storage_round_trip` preference; when //! set, the registry build is compared against the legacy oracle (logged, not fatal) for the soak. use document_container::AnyContainer; use document_format::{Error as DocumentFormatError, GddV1, GddV1Layout}; -use graph_craft::application_io::resource::{LoadResource, ResourceStorage}; +use document_graph_storage::Declarations; +use graph_craft::application_io::resource::ResourceStorage; use graph_craft::document::NodeNetwork; use super::document::DocumentMessageHandler; @@ -86,27 +87,6 @@ pub(super) async fn open_gdd_document( }) } -/// `FutureMessage` that rebuilds a document's interface from a post-move `Gdd` cursor snapshot and -/// delivers it via [`PortfolioMessage::GddUndoRedoRebuilt`] (`None` interface on failure, logged here). -pub(crate) async fn rebuild_gdd_cursor(gdd: GddV1, store_handle: ResourcesHandle, document_id: DocumentId, had_oracle: bool) -> Message { - let declarations = gdd.declarations(&store_handle).await; - let interface = match gdd.registry().to_runtime_with_full_metadata(&declarations) { - Ok((network, node_entries, network_entries)) => match build_interface_from_storage(network, node_entries, network_entries) { - Ok(interface) => Some(Box::new(interface)), - Err(error) => { - log::error!("Gdd undo/redo rebuild for {document_id:?}: failed to build interface: {error}"); - None - } - }, - Err(error) => { - log::error!("Gdd undo/redo rebuild for {document_id:?}: failed to convert registry to runtime: {error}"); - None - } - }; - - Message::Portfolio(PortfolioMessage::GddUndoRedoRebuilt { document_id, had_oracle, interface }) -} - /// Core of the `.gdd` open: archive -> working copy -> `Gdd` -> runtime interface. The registry build is /// authoritative; the embedded legacy blob is the soak oracle and the fallback if the build fails. /// Returns `None` only if neither the build nor the legacy fallback worked. @@ -184,7 +164,7 @@ async fn build_document_from_gdd(path: Option<&std::path::Path>, content: &[u8], ); } } - return Some(DocumentMessageHandler::from_storage(interface, gdd, String::new(), None)); + return Some(DocumentMessageHandler::from_storage(interface, gdd, declarations, String::new(), None)); } log::warn!("Opening .gdd for {document_id:?}: registry build failed, falling back to embedded legacy document"); @@ -196,9 +176,8 @@ async fn build_document_from_gdd(path: Option<&std::path::Path>, content: &[u8], /// Soak check that the reopened `.gdd`'s stored registry, converted back to a runtime network, matches /// the legacy load. Logs divergence only (legacy stays authoritative); runs once per open. -pub(super) async fn compare_storage_against_runtime(gdd: &GddV1, legacy_network: &NodeNetwork, byte_store: &dyn LoadResource, document_id: DocumentId) { - let declarations = gdd.declarations(byte_store).await; - let mut candidate = match gdd.registry().to_runtime_with_metadata(&declarations) { +pub(super) fn compare_storage_against_runtime(gdd: &GddV1, legacy_network: &NodeNetwork, declarations: &Declarations, document_id: DocumentId) { + let mut candidate = match gdd.registry().to_runtime_with_metadata(declarations) { Ok((network, _entries)) => network, Err(error) => { log::error!("Compare-on-open for {document_id:?}: .gdd registry failed to convert to runtime: {error}"); diff --git a/editor/src/messages/portfolio/portfolio_message.rs b/editor/src/messages/portfolio/portfolio_message.rs index bbed19141c9..0a894538cae 100644 --- a/editor/src/messages/portfolio/portfolio_message.rs +++ b/editor/src/messages/portfolio/portfolio_message.rs @@ -45,17 +45,18 @@ pub enum PortfolioMessage { DeleteDocument { document_id: DocumentId, }, - /// Delivers an asynchronously-built `Gdd` working copy into its document, emitted by the mount future - /// spawned in `load_document`. The `gdd` payload is non-serializable and a clone carries none - /// (`clone_to_none`), so it travels exactly once. `reopened` is true when an existing working copy was - /// opened: the persisted cursor is trusted as-is and the mount-time re-commit is skipped, since - /// re-committing would stack a spurious interaction on the restored cursor and make the first undo a no-op. + /// Delivers an asynchronously-built `Gdd` working copy, with the proto-node declarations its history + /// references, into its document. Emitted by the mount future spawned in `load_document`. The `mounted` + /// payload is non-serializable and a clone carries none (`clone_to_none`), so it travels exactly once. + /// `reopened` is true when an existing working copy was opened: the persisted cursor is trusted as-is + /// and the mount-time re-commit is skipped, since re-committing would stack a spurious interaction on + /// the restored cursor and make the first undo a no-op. DocumentStorageMounted { document_id: DocumentId, reopened: bool, #[serde(skip, default)] #[derivative(Debug = "ignore", PartialEq = "ignore", Clone(clone_with = "clone_to_none"))] - gdd: Option, + mounted: Option<(document_format::GddV1, document_graph_storage::Declarations)>, }, DestroyAllDocuments, EditorPreferences, @@ -96,17 +97,6 @@ pub enum PortfolioMessage { #[derivative(Debug = "ignore", PartialEq = "ignore", Clone(clone_with = "clone_to_none"))] document: Option>, }, - /// Delivers the interface rebuilt from the `Gdd` undo/redo cursor so the async rebuild can swap into the - /// live document. `interface` is `None` if the rebuild failed (logged at the source). `had_oracle` records - /// whether the legacy snapshot applied synchronously, so the swap can debug-compare against it. Travels - /// once like [`DocumentStorageMounted`](Self::DocumentStorageMounted). - GddUndoRedoRebuilt { - document_id: DocumentId, - had_oracle: bool, - #[serde(skip, default)] - #[derivative(Debug = "ignore", PartialEq = "ignore", Clone(clone_with = "clone_to_none"))] - interface: Option>, - }, LoadDocument { document_id: DocumentId, document_name: Option, diff --git a/editor/src/messages/portfolio/portfolio_message_handler.rs b/editor/src/messages/portfolio/portfolio_message_handler.rs index 8b9c11ead6b..4a4e6f468ef 100644 --- a/editor/src/messages/portfolio/portfolio_message_handler.rs +++ b/editor/src/messages/portfolio/portfolio_message_handler.rs @@ -309,12 +309,16 @@ impl MessageHandler> for Portfolio responses.add(PortfolioMessage::SelectDocument { document_id }); } } - PortfolioMessage::DocumentStorageMounted { document_id, reopened, gdd } => { + PortfolioMessage::DocumentStorageMounted { document_id, reopened, mounted } => { let Some(document) = self.documents.get_mut(&document_id) else { // Document was closed before its working copy finished mounting. return; }; - document.set_storage(gdd); + let Some((gdd, declarations)) = mounted else { + log::error!("DocumentStorageMounted for {document_id:?} arrived without its payload"); + return; + }; + document.set_storage(gdd, declarations); if !reopened { document.commit_storage_snapshot(&resource_storage.resources_mut(), preferences.validate_storage_round_trip); document.retire_storage_interaction(); @@ -547,18 +551,6 @@ impl MessageHandler> for Portfolio self.load_document(document, document_id, resource_storage, preferences.validate_storage_round_trip, responses); responses.add(PortfolioMessage::SelectDocument { document_id }); } - PortfolioMessage::GddUndoRedoRebuilt { document_id, had_oracle, interface } => { - let Some(document) = self.documents.get_mut(&document_id) else { - // Document was closed before its undo/redo rebuild completed; drop the payload. - return; - }; - let Some(interface) = interface.map(|boxed| *boxed) else { - // The rebuild failed and already logged; leave the live document untouched. - return; - }; - - document.apply_gdd_cursor_rebuild(interface, had_oracle, preferences.validate_storage_round_trip, responses); - } PortfolioMessage::ToggleResetNodesToDefinitionsOnOpen => { self.reset_node_definitions_on_open = !self.reset_node_definitions_on_open; responses.add(MenuBarMessage::SendLayout); @@ -1318,14 +1310,16 @@ impl PortfolioMessageHandler { } }; + let declarations = gdd.declarations(byte_store.as_ref()).await; + if validate && reopened { - compare_storage_against_runtime(&gdd, &legacy_network, byte_store.as_ref(), document_id).await; + compare_storage_against_runtime(&gdd, &legacy_network, &declarations, document_id); } Message::Portfolio(PortfolioMessage::DocumentStorageMounted { document_id, reopened, - gdd: Some(gdd), + mounted: Some((gdd, declarations)), }) }; future.into() From 5505df86d06932e021263f272001c8c803888c3f Mon Sep 17 00:00:00 2001 From: Dennis Kobert Date: Tue, 22 Sep 2026 13:27:24 +0200 Subject: [PATCH 2/2] Address review comments --- document/format/src/lib.rs | 4 ++ document/graph-storage/src/session.rs | 6 +-- .../portfolio/document/document_history.rs | 49 ++++++++++++------- .../document/document_message_handler.rs | 14 ++++-- editor/src/messages/portfolio/document/mod.rs | 2 +- .../messages/portfolio/portfolio_message.rs | 3 +- 6 files changed, 51 insertions(+), 27 deletions(-) diff --git a/document/format/src/lib.rs b/document/format/src/lib.rs index 7b5797bbde3..63f49b74ccf 100644 --- a/document/format/src/lib.rs +++ b/document/format/src/lib.rs @@ -301,6 +301,10 @@ impl Gdd { let mut declarations = document_graph_storage::Declarations::new(); for (id, hash) in self.session.all_declaration_resources() { + let Some(hash) = hash else { + log::error!("Declaration resource {id} has no resolved hash; cannot load ProtoNode"); + continue; + }; let Some(resource) = byte_store.load(hash).await else { log::error!("Declaration bytes for {id} (hash {hash}) missing from byte store"); continue; diff --git a/document/graph-storage/src/session.rs b/document/graph-storage/src/session.rs index e276003d99d..d14b768847d 100644 --- a/document/graph-storage/src/session.rs +++ b/document/graph-storage/src/session.rs @@ -473,8 +473,8 @@ impl Session { } /// Every proto-node declaration resource referenced by the current registry or anywhere in history, - /// with its content hash. - pub fn all_declaration_resources(&self) -> HashMap { + /// with its content hash, or `None` where the hash never resolved. + pub fn all_declaration_resources(&self) -> HashMap> { let registry = &self.document.working_registry; let mut hashes: HashMap = registry.resources.iter().filter_map(|(id, entry)| Some((*id, entry.hash?))).collect(); @@ -502,7 +502,7 @@ impl Session { Implementation::ProtoNode(id) => Some(*id), Implementation::Network(_) => None, }) - .filter_map(|id| Some((id, *hashes.get(&id)?))) + .map(|id| (id, hashes.get(&id).copied())) .collect() } diff --git a/editor/src/messages/portfolio/document/document_history.rs b/editor/src/messages/portfolio/document/document_history.rs index f2957f9ea98..f58d83cd50e 100644 --- a/editor/src/messages/portfolio/document/document_history.rs +++ b/editor/src/messages/portfolio/document/document_history.rs @@ -30,6 +30,15 @@ pub struct DocumentHistory { declarations: Declarations, } +/// Why [`DocumentHistory::move_cursor`] produced no interface. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum CursorMoveError { + /// Nothing to move to, unmounted, or the move itself failed. The cursor did not move. + NotMoved, + /// The cursor moved but the rebuild from it failed. It stays moved unless the caller reverts it. + RebuildFailed, +} + impl DocumentHistory { // ===== Legacy snapshot stacks ===== @@ -150,45 +159,47 @@ impl DocumentHistory { } /// Move the `Gdd` undo/redo cursor along the retired interaction chain, flushing any open interaction - /// first, and rebuild the interface from the rewound registry using the declaration cache. `None` when - /// there is nothing to move to, unmounted, or the move failed. A failed rebuild moves the cursor back. - pub fn move_cursor(&mut self, undo: bool) -> Option { + /// first, and rebuild the interface from the rewound registry using the declaration cache. + pub fn move_cursor(&mut self, undo: bool) -> Result { self.retire_storage_interaction(); - let storage = self.storage.as_mut()?; + let storage = self.storage.as_mut().ok_or(CursorMoveError::NotMoved)?; let moved = if undo { if !storage.can_undo() { - return None; + return Err(CursorMoveError::NotMoved); } storage.undo().map(|_| ()) } else { if !storage.can_redo() { - return None; + return Err(CursorMoveError::NotMoved); } storage.redo().map(|_| ()) }; if let Err(error) = moved { log::error!("Storage undo/redo cursor move failed: {error}"); - return None; + return Err(CursorMoveError::NotMoved); } - let rebuilt = storage + storage .registry() .to_runtime_with_full_metadata(&self.declarations) .map_err(|error| error.to_string()) - .and_then(|(network, node_entries, network_entries)| build_interface_from_storage(network, node_entries, network_entries).map_err(|error| error.to_string())); + .and_then(|(network, node_entries, network_entries)| build_interface_from_storage(network, node_entries, network_entries).map_err(|error| error.to_string())) + .map_err(|error| { + log::error!("Storage undo/redo rebuild failed: {error}"); + CursorMoveError::RebuildFailed + }) + } - match rebuilt { - Ok(interface) => Some(interface), - Err(error) => { - log::error!("Storage undo/redo rebuild failed, reverting cursor move: {error}"); - let reverted = if undo { storage.redo() } else { storage.undo() }; - if let Err(error) = reverted { - log::error!("Storage undo/redo cursor revert failed: {error}"); - } - None - } + /// Step the cursor back the other way, undoing a [`move_cursor`](Self::move_cursor) in the `undo` + /// direction whose rebuild failed. + pub fn revert_cursor(&mut self, undo: bool) { + let Some(storage) = self.storage.as_mut() else { return }; + + let reverted = if undo { storage.redo() } else { storage.undo() }; + if let Err(error) = reverted { + log::error!("Storage undo/redo cursor revert failed: {error}"); } } diff --git a/editor/src/messages/portfolio/document/document_message_handler.rs b/editor/src/messages/portfolio/document/document_message_handler.rs index fe262501ed3..4b998f8b733 100644 --- a/editor/src/messages/portfolio/document/document_message_handler.rs +++ b/editor/src/messages/portfolio/document/document_message_handler.rs @@ -1,10 +1,10 @@ -use super::DocumentHistory; use super::document_diff::diff_networks; use super::node_graph::document_node_definitions; use super::utility_types::error::EditorError; use super::utility_types::misc::{GroupFolderType, SNAP_FUNCTIONS_FOR_BOUNDING_BOXES, SNAP_FUNCTIONS_FOR_PATHS, SnappingOptions, SnappingState}; use super::utility_types::network_interface::{self, NodeNetworkInterface, TransactionStatus}; use super::utility_types::nodes::{CollapsedLayers, LayerStructureEntry, SelectedNodes}; +use super::{CursorMoveError, DocumentHistory}; use crate::application::{GRAPHITE_GIT_COMMIT_HASH, generate_uuid}; use crate::consts::{ ASYMPTOTIC_EFFECT, BLEND_COUNT_PER_LAYER, COLOR_OVERLAY_GRAY, DEFAULT_DOCUMENT_NAME, FILE_EXTENSION, GDD_FILE_EXTENSION, LAYER_INDENT_OFFSET, NODE_CHAIN_WIDTH, SCALE_EFFECT, SCROLLBAR_SPACING, @@ -2086,8 +2086,16 @@ impl DocumentMessageHandler { /// Move the `Gdd` undo/redo cursor and swap in the interface rebuilt from it. `had_oracle` records /// whether the legacy snapshot already applied, so the rebuild can be compared against it. fn drive_storage_undo_redo(&mut self, had_oracle: bool, undo: bool, validate: bool, responses: &mut VecDeque) { - let Some(rebuilt) = self.history.move_cursor(undo) else { return }; - self.apply_gdd_cursor_rebuild(rebuilt, had_oracle, validate, responses); + match self.history.move_cursor(undo) { + Ok(rebuilt) => self.apply_gdd_cursor_rebuild(rebuilt, had_oracle, validate, responses), + Err(CursorMoveError::NotMoved) => {} + // Without the legacy snapshot the interface stayed put, so the cursor has to follow it back. + Err(CursorMoveError::RebuildFailed) => { + if !had_oracle { + self.history.revert_cursor(undo); + } + } + } } /// Swap in the interface rebuilt from the `Gdd` cursor. Always overwrites the interface. diff --git a/editor/src/messages/portfolio/document/mod.rs b/editor/src/messages/portfolio/document/mod.rs index fb8749ce732..69f590da5b9 100644 --- a/editor/src/messages/portfolio/document/mod.rs +++ b/editor/src/messages/portfolio/document/mod.rs @@ -15,7 +15,7 @@ pub mod resource; pub mod utility_types; pub(crate) use document_diff::diff_networks; -pub(crate) use document_history::DocumentHistory; +pub(crate) use document_history::{CursorMoveError, DocumentHistory}; #[doc(inline)] pub use document_message::{DocumentMessage, DocumentMessageDiscriminant}; #[doc(inline)] diff --git a/editor/src/messages/portfolio/portfolio_message.rs b/editor/src/messages/portfolio/portfolio_message.rs index 0a894538cae..7a4dbabf6b2 100644 --- a/editor/src/messages/portfolio/portfolio_message.rs +++ b/editor/src/messages/portfolio/portfolio_message.rs @@ -148,7 +148,8 @@ pub enum PortfolioMessage { UpdateOpenDocumentsList, } -/// Clone helper for the non-serializable `gdd` payload: a cloned mount message carries no `Gdd`. +/// Clone helper for non-serializable payloads: a cloned message carries none of the `Gdd` working copy, +/// its declarations, or the built document. fn clone_to_none(_: &Option) -> Option { None }