Repository navigation
Conversation
|
I did a bit of an automated review, and uncovered a bunch of cases this approach does not track or clone, and in principle cant do (re-entrancy, etc) Apologies for the pasted LLM 'review' that below that enumerates issues caught. I did some musing about the problem with Codex, and it does seem that simply decoding instances of the same graph, conceptually fixes a bunch of the problems the cloning approach cant (or perhaps, currently cannot) - the only thing i need to vet is that strictly speaking subgraph a and b would have colloding UUIDs for subnodes. I dont think that is a problem. The goal of the UUIDs is not that for the entire document, every node needs a unique instance, but that every unique node has a unique id. The Clone having duplicated IDs is in fact a signal and semantically correct i would argue. Secondly, lookup and editing would do what you expect (each subgraph clone finds the same node, which is what it should do). And now since we dont have feedback cache, the only issue is if graph execution does anything dumb - but subgraphs are isolated from their parent graph as they are executed with their own graph renderer, so i think its ok. Secondly, decoding the same graph resolves the cloning problem as each decode instance is its own instance. Overall this the cloning approach is super non trivial, has a lot of cases not accounted for, and potentially not possible to account for due to the flexibility of how some nodes can define their own internal state. My gut is syncronization is not the solution, but rather decoding each graph, and on edit, re-loading the graph and (but not the renderer or input times). This also resolves issues like out of sync during edit, in sync during fresh loading (ie the integrator nuance we spoke about) In short, every part of my engineering brain says we should re-use the serialization / deserialization mechanism already available, that already works and not try to get fancy. The only issue is how we save these graphs, if we embed them in the root graph, or if we let them be sidecar files. I'll try to propose an alternate design (not a PR, just a written proposal) later tonight. LLM Review below: |
|
I hadn’t done an automated review, expecting that would be the first thing you did, and if with a different genie then even better. So, not surprised that a bunch of things came up, but a bit surprised on the take-down of the approach in general. I suspect there will still need to be a similar adversarial review on whatever you come up with, as e.g. "Per-instance runtime values/state are stored separately from design state” sounds like coming up with a whole new execution model. That sounds like a massive trade for what I read as issues that of the more dealbreaker-looking actually point more to shoring up Fabric’s data model than a fundamental wrong concept. But mainly just happy we’re good with the UX and that’s locked! |
|
Im not suggesting we do that, thats just one option The thing I want to go with, which a separate convo seems to imply adversarially as much less work to implement, is simply encoding / decoding the graph and instantiating it when changes occur. The instance being edited wont need serialization loop - but other ones would. It also woldnt need to de-duplicate UUIDs as each graph is being exceuted by the sub graphs node unique graph renderer instance - and semantically they are clones, so duplicate IDs is semantically correct by my measure. So the issue then is simply storage - and timing the execution / serialization loop so things change in a predetermined way. This also scopes the logic to literally only be changes to Subgraph. Literally nothing else would need to change, if im pondering it correctly. |
If there’s a way to make that not "fail to be simple" in practice, then that’s worth exploring. It does come with a real degradation to the UX as the state in all other clones will be lost. I’m not sure it's quantifiable how bad that is, will depend on what’s going on in the subgraph. A trivial example: you tweak something and all the movies except one restart. Also I forgot to comment that the observation that the UUID being the same across each clone set sub graphs’s nodes could be a feature not a dealbreaker is a good one. That could also bring the peer-to-peer model back into play I think. The other thing I forgot to call out was hell no on sidecar files being how this lands. I’m 100% that the core UX has to be seamless in-document. Import/export, a setting to back that in-memory clone graph with a sidecar file, any and all are quite conceivable but have to be additional in my view. (If we had bundle documents I might be persuaded) |
|
I’ve had another look, and there’ll be some commits coming. Note not as doubling-down, just the spirit of a better bake-off. On which note, I do think you should land on an implementation rather than just design doc. Maybe your genie is better than mine, but I find docs never quite survive contact with the reality.
Forgot to flag that this is deliberate not an omission. These ports are how you specialise each clone. To me the clearer model is that everything inside the node is cloned, and the ports you see in the regular graph are just duplicated as expected and then behave normally (no magic). |
A clone set is an object on the document's root graph: a name and the template, the encoded form of a member's sub graph with every id rewritten to the template's own, held as canonical JSON and saved with the document. SubgraphNode carries the set id and a record mapping template ids to its local ids, for nodes, ports, wires and nested graphs alike. The record's keys are template ids, which a UUID rewrite never touches, and its values are local ids, which a rewrite maps along with the nodes, so a member duplicated as a clone gets a correct record from the duplication itself and Node needs no clone field. Graph.rootGraph and ownerNode let any graph reach the document's sets. duplicateAsClone starts a set with the source's design as template where needed, refreshes the template and the source's record, then duplicates with the set id preserved; a plain duplicate strips every clone link. unlinkClone gives sets nested in the node copies of their templates under fresh names, so their records keep resolving while they detach. Names live on the set; renaming is undoable and an empty name restores a system one. Members derive the set name as subtitle and the clone glyph, with the set's details on hover, as their title icon. Sets no member refers to are dropped on save. Via Claude Fable 5.1
A sync refreshes the set's template from the edited member and then brings each sibling in line with it, node for node through the two records: the source's local ids to template ids, template ids to the sibling's local ids. Surplus nodes go, missing ones are decoded from the source with every id mapped through the records, existing or newly assigned, and matched nodes take the source's layout, rename, published state and the values of inlets that are neither published nor wired, so published inlet values stay per member and runtime state is never touched. A matched node whose class or settings signature differs is replaced, keeping the ids the sibling recorded for it, so the parent's wires onto its proxies and the host's lookups survive the replacement. Wires are diffed by the sibling's own port ids, proxies included since a proxy carries its inner port's id; dangling wires are pruned. Nested sub graphs reconcile first so the proxies the parent wires land on exist. Both records grow with anything new. Nothing here registers undo. A member can also be made from the template alone, in the set's member class, with every id fresh and recorded: for a new member with no live source, for recovery of a member whose record is missing, which is rebuilt in place with the parent's wires re-bound to its published ports by name, and for a template updated from outside the document, which is applied through a transient member then discarded. Template JSON is parsed through the decoder's AnyCodable path rather than JSONSerialization, whose NSNumbers re-encode zeros as booleans and would have corrupted saved templates. Via Claude Fable 5.1
Graph gains a content revision and noteContentChanged(), bumped by its own mutation API: the topology change it already marks, enabling and disabling a wire, and notes. Everything nodes and ports publish is watched by a CloneMemberObserver the member owns while it is in a set, through the signals that already exist: the offset and ports-changed subjects, the subtitle subject read for the rename alone since derived subtitles can follow a port every frame, observation tracking on a port's publish state, and each parameter's value publisher, counted only for an unwired, unpublished inlet written from the main thread. Nothing on Node or Port changes. When the edited graph sits inside a set, the root graph's coordinator queues it and, once edits pause for the debounce interval, syncs every set around it with that graph as the source, then has the members re-subscribe to their node trees. Writes made while reconciling schedule nothing, so a sync never triggers another, and undo on the source flows to the siblings the same way. Leaving a canvas syncs as a safety net and the editor flushes before saving. The editor wiring comes across from the previous clone branch: the Subgraph node menu with Duplicate as Clone, Rename Clone Set, Select Sibling Clones and Unlink from Clones, and breadcrumb entries showing a node's title icon before its name. Via Claude Fable 5.1
The default was below the slider's minimum, so a saved document loaded the port clamped to zero instead of the value it was saved with. Surfaced by the clone set fidelity test. Via Claude Fable 5.1
…te model Instantiates every core node that needs no device, permission, download or file, clones the member holding them, and expects every copy to match its source's settings signature, a reconcile pass to change nothing, and a member made from the template alone to match as well. Encode/decode is what defines a node's design for clone sets, so a node that fails here would drift silently between siblings. The glossary and engineering spec describe the template, the record and where clone-specific code may live. Via Claude Fable 5.1
…aft as typed The alert's text was filled in by an onChange that ran after presentation, and a macOS alert takes its text field's content at presentation, so the field opened empty and what OK committed was not what was shown. The rename is now a request carrying the draft name, created with the set's current name when the menu item is chosen; the text field edits the draft in place and OK commits it, an emptied field included, which restores a system name. Via Claude Fable 5.1
The member observer's sinks ran on whatever thread published, and a node's subtitle subject can fire every frame from the render thread, racing the observer's rename cache and the graph's content revision. Every sink and observation callback now drops anything not on the main thread, which is where edits happen. The coordinator's lock covered half its state; it is gone, the coordinator is main-thread state, and a call from elsewhere, to schedule or to flush, is handed to the main actor instead of run in place. settle() awaits a pending debounce so the debounce test no longer sleeps a fixed time, and the view model mirror test polls for delivery the way the naming tests do. A stray ObservationIgnored on a non-observable class is removed. Via Claude Fable 5.1
…main thread, and let nodes report settings changes Review on the PR found that two members edited in one debounce window synced from the first, overwriting the later edit: the coordinator now keeps one pending source per set, the last graph edited, dropping earlier ones that share a set. A member arriving in a graph that already sits inside a member of its own set is unlinked on arrival, with a diagnostic, rather than left in a set it can never satisfy. The save-time flush asserts the main thread, as does flush itself, instead of silently deferring and encoding stale members. Settings that change without touching ports were invisible to the member observer. Node gains settingsDidChange(), a call a node makes after changing any encoded state that is not a port, and the observer listens to it. Every core node with such state now calls it: expressions, strategies, scripts and execution modes, shader source, timelines, format strings, model settings, OSC, MIDI, HID, keyboard and game controller selections, and the deferred subgraph's MRT flag. The review checklist gains the rule. Via Claude Fable 5.1
…ave nothing behind on failure duplicateAsClone, unlinkClone, renameCloneSet, instantiateCloneSetMember, applyCloneTemplate and reconcileCloneMember throw FabricErrors (graph kinds cloneSetNotFound, notACloneMember, cloneOperationFailed, alongside nodeNotInGraph) where they used to return nil or do nothing. A Duplicate as Clone that fails after starting a set removes the set and restores the source's membership, with or without an undo manager. The template's JSON form is no longer part of the public surface except as the set's own property and the input to applying an external template. Via Claude Fable 5.1
…ngs encode for nodes without settings A document now holds a set's template once. A member whose design matches the template, compared with every published inlet's value left out since those are each member's own, is written as its set id, its record and those values, and materialised from the template on load through the record, so every id the document knew, and every parent wire onto its proxies, comes back as it was. A member that has drifted from the template is written in full so nothing is lost. Editors from before clone sets cannot open members saved this way. The editor's save opts in through the encoder's userInfo; duplication, the clipboard and templates keep writing members in full. Along the way: a nested member's set and record now travel with the design when a sibling is reconciled in place, recovery of a member without a record carries its published inlet values over by name, and the settings signature is only computed for nodes that have settings to compare. Via Claude Fable 5.1
With the title icon PR reworked to statuses, a member now reports NodeStatus.linked with its set and member count as the message, the lowest severity so an error or warning on the same node shows first. The view layer maps each status to its glyph and colour in one place, NodeStatusGlyph, used by the node title and the editor's breadcrumb, which now shows a node's most severe status and lists them all on hover. The registry-derived subgraph type list is merged in, so Embed Selection In offers plugin subgraph types here too. Via Claude Fable 5.1
… work per sync and save The member observer's observation registrations could not be cancelled and re-registered themselves, so each sync added another live watcher per port; they now carry a generation and stand down once a refresh has moved past them. Parameter values are watched only on inlets that are unwired and unpublished, the ones whose values are design, rather than filtering every frame's traffic on the render thread. Changing a node's clone set or record now counts as a settings change, so unlinking a member nested inside a member reaches the siblings. Leaving a canvas syncs only when something was edited since entering. Per sync, ports are looked up through one table per graph instead of a flattened scan per wire, and a source node's settings signature is computed once for all siblings. Per save, the template's comparable form is computed once per set, and only a root graph walks for live sets. A force unwrap in the node diff and four in the tests are gone, the breadcrumb uses default stack spacing, and a rewritten line uses the Swift-native string replace. Via Claude Fable 5.1
…e, nested sets sync from the timer, recovery and duplicate leave no stray undo, badges follow nested membership, paste strips links A compact save wrote a member's record and values but not its proxies, whose published state and names in the parent graph exist nowhere else, so a member's proxy published onward, a document input in Spark Stage, was lost on reload; proxies are written either way now. Per-member values were collected one level deep while the design comparison and the live sync treat published inlets at every depth as the member's own; both now cover the whole sub graph. The coordinator dropped a pending graph nested inside a member when the outer member's graph was noted later, so an edit inside a nested member could miss its own set; a pending graph nested in the newcomer now stands, since its sync walks up through every enclosing set, and only pending graphs whose sets the newcomer fully covers are dropped. Recovery of a member without a record ran the instantiation outside undo suppression and left an Add Node step; the whole recovery is suppressed. Duplicate as Clone registered undo actions before it could fail; it now works without registration and registers one symmetric step on success. Membership changes refresh the badges of every set in the subtree that arrived, left or moved, nested sets included. Pasting a member strips its links, as duplicating already did. Via Claude Fable 5.1
The code review's simplification pass. No behaviour changes beyond the leave-canvas trigger, which now just flushes the coordinator's pending syncs instead of keeping its own per-entry content revisions and re-deriving what was edited: the coordinator already knows. CloneSyncContext no longer mints template ids for the source. The source's record is completed up front (completeCloneRecord, split out of cloneTemplateJSON; the public reconcileCloneMember does it itself) so the context's source side is a fixed dictionary, the lookups are plain, and the only allocating call is targetLocalID(allocatingFor:). Graph.ancestors replaces four hand-rolled walks up the owner chain (rootGraph, enclosingCloneMembers, isDescendant, reconcileCloneSets(enclosing:)). Graph.decodeNode(from:remap:preservingKeys:) is the one rewrite-then-decode step shared by duplicate, paste and clone syncs. Dictionary.inverted replaces the repeated uniquingKeysWith inversions and the SubgraphNode template-id cache; CloneSet drops its comparable-JSON cache and the templateObject setter. The coordinator is internal and asserts the main thread rather than hopping to it, since Graph already does the hop. Recovery reads published inlets through publishedInputPorts(). The two canvas-leave tests now build their fixtures off the main thread and run only the navigation on it. Via Claude Fable 5.1
SwiftUI asks a FileDocument for its file wrapper on a background queue, so the save's flush tripped the main-thread precondition (EXC_BREAKPOINT in CloneSetCoordinator.flush from FabricDocument.fileWrapper). The sync is a graph edit and stays on the main thread; flushPendingCloneSync now hops there synchronously when called from elsewhere, since the save has to encode the synced members. Test covers a detached-task flush after a main-thread edit. Via Claude Fable 5.1
bcfabcd to
d06e5d6
Compare
|
I have pushed the post-review work I did at the time, rebased onto current main. That I think is as good as this approach is going to get without me tearing it all apart manually. I have a genie written summary of it all, but I’ll hold off on the detail in favour of – I’m keen to get this feature in, so I might kick off an agent to try the approach you/vade are suggesting, and then there can be a bake-off, with competing UX, competing audit, and competing human review of code. |
2 participants
An implementation for #199. The premise is that you can duplicate any subgraph type node as a clone, and then any edit within one of that clone set changes them all. Key here is that this is Fabric level functionality, not the introduction of a specialised node. For example, third-party SPK Stage Object nodes can be cloned.
This is the current new document spinning cube embedded in a Sub Graph node. Now subgraph type nodes have Duplicate as clone in their context menu –
Now cloned, there are two spinning cubes. Note their execution is per-instance, and cloning operations maintain state on a running subgraph, so here we see the two cubes spinning out of sync with each other as the Number Integrator has been running for a different amount of time in each. If this document were saved and opened afresh, those two integrator nodes would start integrating at the same time, and the cubes would be in sync.
To control a cloned subgraph individually, publish ports. Here, we got the Spin toggle and Speed value from the new document graph for free, and e.g. toggling spin off on one will do what Fabric has taught you to expect: that node’s cube will stop spinning while the other node’s cube continues.
This implementation likely can be improved but I think is the right direction. A simpler, more brutal approach of replacing each other members’ subgraph with the updated one fails to be simple in practice, requiring new subgraphs to be minted from the latest so that only shape not execution is cloned, and then any published ports to be remapped. Accepting some kind of change propagation machinery amongst subgraphs of any clone set, you arrive at either a peer-to-peer approach or a template-backed one. This implementation takes the template-backed approach, in practice serialising the desired subgraph shape to JSON. This has the advantage of being amenable to being backed by sidecar files or some kind of repository. This has the disadvantage documents saved with this will not degrade to uncloned duplicates if opened by earlier Fabric Editor, as the peer-to-peer model does. I don’t think that matters, and only having one copy of a cloned subgraph in a document is a filesize win. The clincher for me was it also allows this subgraph-scoped functionality to be contained to the Subgraph base type; the peer-to-peer implementation required clone tracking on the Node base type.
From the genie –