From 9f8628cfb243bedb7e8495752553893174f4025a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Fri, 5 Jun 2026 13:30:47 +0200 Subject: [PATCH] =?UTF-8?q?fix(eeschema):=20collab=20converges=20on=20big?= =?UTF-8?q?=20drags=20=E2=80=94=20emit=20a=20post-settle=20snapshot=20diff?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batched-emit fix still lost segments on a large connected drag (peer dropped the P3-C1 wire). Deeper cause: the SCHEMATIC_LISTENER fires in pushSchEdit BEFORE RecalculateConnections (sch_commit.cpp ~402 vs ~430), so every emit was pre-cleanup RAW geometry; the cleanup that follows (merge collinear wires, drop/split junctions) was never broadcast. The peer rebuilt the raw edit and ran its own cleanup over a different dirty scope, so the two peers cleaned up differently and the peer lost segments. Replace the listener-list emit with a post-settle full-model snapshot DIFF (snapshotByUuid), flushed via CallAfter once Push (cleanup included) returns — capturing tab A's final, already-clean geometry. The peer applies that and re-cleaning already-clean geometry is idempotent, so they converge. The native listener is now just a change trigger. g_baseline holds the last-broadcast state; doApply and kicadCollabSnapshot rebaseline so applied/seed items aren't re-broadcast (echo). Mirrors pl_editor's snapshot-differ; no kicad-fork change. Verified two-tab, rigorously (real edit: tabA state changed AND tabA===tabB byte-for-byte): a wire reroute plus U1A/U1B/C2 symbol drags all converge exactly. eeschema-collab + eeschema-ui suites green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../0007-eeschema-essential-ops-findings.md | 26 ++- kicad | 2 +- wasm/bindings/eeschema_embind.cpp | 198 +++++++++++------- 3 files changed, 139 insertions(+), 87 deletions(-) diff --git a/features/yjs-bridge/0007-eeschema-essential-ops-findings.md b/features/yjs-bridge/0007-eeschema-essential-ops-findings.md index abda4c4..61b45fa 100644 --- a/features/yjs-bridge/0007-eeschema-essential-ops-findings.md +++ b/features/yjs-bridge/0007-eeschema-essential-ops-findings.md @@ -103,13 +103,25 @@ recompute. A `G`-drag of U1A emitted `{added:[junction]}`, `{removed:[wire]}`, it**. Result: tab A 75 items / 8 junctions, tab B 74 / 7 (the junction lost). Simple translates (`M` tool) always converged — they touch only `changed`. -**Fix (`wasm/bindings/eeschema_embind.cpp`):** `COLLAB_LISTENER` now buffers the three -categories (serializing items in each synchronous callback) and flushes **one combined -delta** after Push returns, coalesced via `CallAfter`. The peer's `doApply` applies a -combined delta removed→changed→added in a single `SCH_COMMIT` with one recompute, so the -junction is added after its wires are in place and survives. **Verified:** the same G-drag -now emits 1 delta `{a:1,c:4,r:1}` and both tabs converge identically (75 items, 8 junctions, -zero wire/junction diff). Embind-only build; eeschema-collab + eeschema-ui suites green. +**First fix (batched emit) was insufficient.** Combining the three callbacks into one delta +fixed the junction-add case, but the user could still break it: a *large* connected drag +made the peer lose the P3↔C1 wire. **Deeper root cause:** the SCHEMATIC_LISTENER fires in +`pushSchEdit` *before* `RecalculateConnections` (sch_commit.cpp ~402 vs ~430), so the emit +was always **pre-cleanup raw geometry**; the connectivity cleanup that follows (merge +collinear wires, drop/split junctions) was never broadcast. The peer reconstructed the raw +edit and ran ITS OWN cleanup over a different "dirty" scope → the two peers cleaned up +differently and the peer lost segments. + +**Final fix (`wasm/bindings/eeschema_embind.cpp`): emit a post-settle snapshot diff.** The +native listener is now just a "something changed" trigger; the actual change set is a DIFF of +the full model taken after the edit *settles* — a `CallAfter` flush, which runs once Push +(cleanup included) returns — so it captures tab A's FINAL, already-clean geometry. The peer +applies that and re-cleaning already-clean geometry is idempotent, so the two converge. +(Mirrors pl_editor's snapshot-differ.) `g_baseline` holds the last-broadcast state; +`doApply` and `kicadCollabSnapshot` rebaseline so applied/seed items aren't re-broadcast +(echo). No kicad-fork change. **Verified two-tab, rigorously** (real edit: `tabA` state +changed AND `tabA===tabB` byte-for-byte): a wire reroute, plus U1A/U1B/C2 symbol drags, all +converge exactly; eeschema-collab + eeschema-ui suites green. Embind-only build. ## Files touched (all root repo) diff --git a/kicad b/kicad index 4132395..91948d1 160000 --- a/kicad +++ b/kicad @@ -1 +1 @@ -Subproject commit 4132395c823d54105b47049b32b40cbae85eff8b +Subproject commit 91948d1c78df6389f19438c44189933db64ce2b3 diff --git a/wasm/bindings/eeschema_embind.cpp b/wasm/bindings/eeschema_embind.cpp index 7593aac..80d8369 100644 --- a/wasm/bindings/eeschema_embind.cpp +++ b/wasm/bindings/eeschema_embind.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -284,93 +285,124 @@ void emit( const json& aDelta ) }, s.c_str() ); } -// ChangeSource: native SCHEMATIC_LISTENER. SCH_COMMIT::Push fires these in bulk for -// every local edit (move, add, remove, …) — that's our emit trigger. +// ── Emit via post-settle snapshot diff ─────────────────────────────────────────────────── // -// CRUCIAL: one local edit (a single SCH_COMMIT::Push) calls OnItemsAdded, OnItemsRemoved -// and OnItemsChanged *separately and synchronously*, then runs RecalculateConnections once -// (sch_commit.cpp). Emitting each category as its own delta makes the peer apply them as -// THREE separate commits, each followed by its own connectivity recompute — so an item that -// only makes sense in the final state is mis-handled mid-sequence. Concretely, a connected -// drag adds a junction at the wires' new crossing AND moves those wires; if the junction -// (added) is applied before the wires (changed), the peer sees a dangling junction and -// connectivity cleanup deletes it → the peer permanently loses it (the "lost segments on a -// big drag" divergence). Fix: buffer all three categories and emit ONE combined delta after -// Push returns (coalesced via CallAfter), so the peer applies it atomically in a single -// SCH_COMMIT — doApply orders it removed→changed→added, with one recompute at the end, so the -// junction is added after its wires are in place and survives. +// A local edit is a single SCH_COMMIT::Push that fires OnItemsAdded/Removed/Changed +// synchronously and THEN runs RecalculateConnections (sch_commit.cpp ~402-430). So the native +// listener only ever sees the *pre-cleanup* (raw) geometry, while the connectivity cleanup +// that follows — merging collinear wires, dropping redundant junctions, splitting at new +// crossings — is never reported. Broadcasting those raw per-category lists made the peer +// reconstruct the edit from the raw state and run ITS OWN cleanup, over a different "dirty" +// scope, so on a big connected drag the two peers cleaned up differently and the peer lost +// segments/junctions. +// +// Instead, treat the listener purely as a "something changed" trigger and broadcast a DIFF of +// the full model taken AFTER the edit settles — a CallAfter, which runs once Push (cleanup +// included) has fully returned. That captures tab A's FINAL, already-clean geometry; the peer +// applies it and re-cleaning already-clean geometry is idempotent, so the two converge. (This +// mirrors pl_editor's snapshot-differ.) g_baseline is the last-broadcast state. + +std::map snapshotByUuid( SCHEMATIC& aSch ) +{ + std::map m; + + for( const SCH_SHEET_PATH& path : aSch.Hierarchy() ) + { + SCH_SCREEN* screen = const_cast( path ).LastScreen(); + + if( !screen ) + continue; + + for( SCH_ITEM* item : screen->Items() ) + { + std::string id = toUtf8( item->m_Uuid.AsString() ); + + if( !m.count( id ) ) + m[id] = itemToJson( item ); + } + } + + return m; +} + +std::map g_baseline; +bool g_flushScheduled = false; + +// Re-seed the diff baseline to the current model — after handing out a seed snapshot, or after +// applying a remote delta (so those items aren't re-broadcast as a spurious local diff/echo). +void rebaseline() +{ + if( SCH_EDIT_FRAME* fr = schFrame() ) + g_baseline = snapshotByUuid( fr->Schematic() ); +} + +// Diff the current (settled, post-cleanup) model against the baseline and broadcast the change. +void flushDiff() +{ + g_flushScheduled = false; + + SCH_EDIT_FRAME* fr = schFrame(); + + if( !fr ) + return; + + std::map cur = snapshotByUuid( fr->Schematic() ); + + json added = json::array(), changed = json::array(), removed = json::array(); + + for( const auto& [id, j] : cur ) + { + auto it = g_baseline.find( id ); + + if( it == g_baseline.end() ) + added.push_back( j ); + else if( it->second != j ) + changed.push_back( j ); + } + + for( const auto& [id, j] : g_baseline ) + { + if( !cur.count( id ) ) + removed.push_back( id ); + } + + g_baseline = std::move( cur ); + + if( !added.empty() || !changed.empty() || !removed.empty() ) + emit( json{ { "added", added }, { "changed", changed }, { "removed", removed } } ); +} + +// Coalesce all the listener callbacks of one commit (and any other edits in the same loop +// turn) into a single post-settle diff. +void scheduleFlush() +{ + if( g_flushScheduled ) + return; + + g_flushScheduled = true; + + if( SCH_EDIT_FRAME* fr = schFrame() ) + fr->CallAfter( []() { flushDiff(); } ); + else + flushDiff(); +} + +// ChangeSource: the native SCHEMATIC_LISTENER is just a trigger — the actual change set comes +// from the post-settle snapshot diff above. Skipped while applying a remote delta (no echo); +// doApply rebaselines instead. class COLLAB_LISTENER : public SCHEMATIC_LISTENER { public: - void OnSchItemsAdded( SCHEMATIC&, std::vector& aItems ) override - { - accumulate( m_added, aItems ); - } - - void OnSchItemsChanged( SCHEMATIC&, std::vector& aItems ) override - { - accumulate( m_changed, aItems ); - } - - void OnSchItemsRemoved( SCHEMATIC&, std::vector& aItems ) override - { - if( s_applyingRemote ) - return; - - for( SCH_ITEM* item : aItems ) - m_removed.push_back( toUtf8( item->m_Uuid.AsString() ) ); - - scheduleFlush(); - } + void OnSchItemsAdded( SCHEMATIC&, std::vector& ) override { trigger(); } + void OnSchItemsChanged( SCHEMATIC&, std::vector& ) override { trigger(); } + void OnSchItemsRemoved( SCHEMATIC&, std::vector& ) override { trigger(); } private: - // Serialize the items NOW (they're valid during the synchronous callback); the buffered - // json is emitted later by flush(). - void accumulate( json& aBucket, std::vector& aItems ) + void trigger() { - if( s_applyingRemote ) - return; - - for( SCH_ITEM* item : aItems ) - aBucket.push_back( itemToJson( item ) ); - - scheduleFlush(); + if( !s_applyingRemote ) + scheduleFlush(); } - - // All three category callbacks for one Push fire synchronously before control returns to - // the main loop, so a single CallAfter run after Push drains them into one delta. (Several - // commits in the same loop turn coalesce into one delta — harmless for the CRDT.) - void scheduleFlush() - { - if( m_flushScheduled ) - return; - - m_flushScheduled = true; - - if( SCH_EDIT_FRAME* fr = schFrame() ) - fr->CallAfter( [this]() { flush(); } ); - else - flush(); - } - - void flush() - { - m_flushScheduled = false; - - if( m_added.empty() && m_changed.empty() && m_removed.empty() ) - return; - - emit( json{ { "added", m_added }, { "changed", m_changed }, { "removed", m_removed } } ); - - m_added = json::array(); - m_changed = json::array(); - m_removed = json::array(); - } - - json m_added = json::array(); - json m_changed = json::array(); - json m_removed = json::array(); - bool m_flushScheduled = false; }; COLLAB_LISTENER* g_listener = nullptr; @@ -524,6 +556,10 @@ void doApply( SCH_EDIT_FRAME* aFrame, const json& aDelta ) if( staged ) commit.Push( wxT( "Collaborative edit" ) ); + // The applied remote changes (and any connectivity cleanup they triggered) are now the + // shared state — fold them into the baseline so the post-apply listener flush doesn't + // re-broadcast them as a local diff (echo). + rebaseline(); s_applyingRemote = false; } @@ -587,6 +623,10 @@ std::string kicadCollabSnapshot() SCHEMATIC* sch = ensureBridge(); json added = sch ? snapshotItems( *sch ) : json::array(); + // Seed the diff baseline to exactly the model we're handing out, so the first local edit + // diffs against this snapshot (and we don't re-broadcast the whole model). + rebaseline(); + return json{ { "added", added }, { "changed", json::array() }, { "removed", json::array() } }.dump(); }