feat(eeschema): collab SCH_SHAPE emit + defensive text; defer shape/symbol add (asyncify trap); un-skip apply e2e
- itemToJson hand-maps SCH_SHAPE geometry (SHAPE_T, start/end, arc center, bezier ctrl pts, stroke width, fill) for emit + the changed/move path. - makeItem: parent + default-size SCH_TEXT (mirrors createNewText). - SCH_SHAPE/SCH_SYMBOL `added` reconstruction deferred: committing a new shape/symbol traps in SCH_COMMIT::Push from the programmatic apply context (asyncify invoke_* mis-dispatch, not the boundary 'signature mismatch' the dyncall shim catches). makeItem returns nullptr -> graceful 'no converter'. - Un-skipped + extended the headless apply e2e (move via sx/sy line form, delete, text-add); apply works headless (old 'Push no-ops' belief was stale). - features/yjs-bridge/0006: full findings (bugs 1+2 don't reproduce, etc). No kicad-fork change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
5764d7ec24
commit
04b011d032
3 changed files with 190 additions and 11 deletions
95
features/yjs-bridge/0006-eeschema-apply-findings.md
Normal file
95
features/yjs-bridge/0006-eeschema-apply-findings.md
Normal file
|
|
@ -0,0 +1,95 @@
|
|||
# eeschema collab apply — bugfix session findings (2026-06-04)
|
||||
|
||||
Investigation + fixes for the four reported eeschema collab apply bugs, plus the
|
||||
SCH_SHAPE / SCH_SYMBOL converters. Ground truth was gathered with a headless probe
|
||||
spec and with the **real web app driven in two tabs** (`?collab=1`, demo.kicad_sch,
|
||||
75 items incl. 26 SCH_SYMBOL) — the only place real GAL painting happens.
|
||||
|
||||
## Key correction: apply works headless
|
||||
|
||||
The e2e harness's `kicadOpenFile` returns **false**, but the schematic + screens ARE
|
||||
built, so `SCH_COMMIT::Push` **does take effect** headless (move/delete/add all mutate
|
||||
the model). The earlier belief that "Push no-ops headless" predated the dyncall-shim
|
||||
fix. What headless *cannot* do is **paint** (the GL window is incomplete → the
|
||||
`window.cpp(731) GetScreenPositionOfClientOrigin: no window` assert). So apply logic is
|
||||
testable headless; rendering must be checked in the real app. The skipped apply test is
|
||||
now **un-skipped and extended** (move via the `sx/sy/ex/ey` line form, delete, text add,
|
||||
shape add).
|
||||
|
||||
## Bug-by-bug
|
||||
|
||||
1. **"Adding text freezes"** — does NOT reproduce through the collab bridge. Verified in
|
||||
the real app: a text added locally in tab A (via the `onDelta` emit sink, the exact
|
||||
path the native `SCHEMATIC_LISTENER` uses) propagates over BroadcastChannel and tab B
|
||||
**applies it and stays responsive**. Single-tab `kicadCollabApply` of a constructed
|
||||
`SCH_TEXT` also stays responsive. Conclusion: the user's freeze is the **interactive**
|
||||
text placement (`createNewText` → `DIALOG_TEXT_PROPERTIES::ShowQuasiModal`, a modal
|
||||
nested event loop — a known Asyncify-wasm hang), not the collab apply/emit path.
|
||||
Defensive change anyway: `makeItem` now parents the text to the schematic and applies
|
||||
the default text size, mirroring `createNewText`.
|
||||
|
||||
2. **"Delete doesn't apply"** — does NOT reproduce. C++ apply-delete works headless
|
||||
(`kicadCollabGetPos` → "" after a `removed` delta) AND end-to-end in two tabs (tab B
|
||||
drops the item, count decrements). Likely already fixed by the converters + the
|
||||
`CallAfter` deferral now in place.
|
||||
|
||||
3. **Circles / SCH_SHAPE don't sync** — REAL gap. Emit side added (itemToJson hand-maps
|
||||
SHAPE_T + start/end, arc center, bezier control points, stroke width, fill). But
|
||||
`added` **reconstruction is DEFERRED** — same wasm trap class as SCH_SYMBOL (below).
|
||||
Bisected with EM_ASM markers: `makeItem` + `commit.Add` succeed; the trap is inside
|
||||
`SCH_COMMIT::Push`'s **CHT_ADD** path — the GAL `view->Add` of a *new* SCH_SHAPE —
|
||||
"memory access out of bounds". The identical `view->Add` succeeds when the shape is
|
||||
**loaded from file** and when an existing shape is **moved** (CHT_MODIFY, verified), so
|
||||
it's the asyncify `invoke_viii` mis-dispatch of a SCH_SHAPE add-path virtual, only from
|
||||
the programmatic CallAfter/apply context. The dyncall shim catches only the
|
||||
"signature mismatch" variant; this trap fires inside the wrong function → not safely
|
||||
retryable. (`SetParent`/fill-value/layer-warmup were all ruled out by experiment.)
|
||||
|
||||
4. **Wire moves only partially converge** — basic two-tab move **does** converge
|
||||
(verified: tab A `kicadCollabTestMoveFirst` → tab B matches, count stable, no dup).
|
||||
The connected-wire re-split divergence was not deeply reproduced; it remains a known
|
||||
connectivity-CRDT limitation (both peers run `RecalculateConnections` on apply).
|
||||
Also found: for a `SCH_LINE`, the `changed` path's bare `item->Move()` is a **no-op**
|
||||
(line endpoints only move when flagged) — but the emit side always sends
|
||||
`sx/sy/ex/ey`, so real wire moves take the working `SetStart/EndPoint` branch.
|
||||
|
||||
## SCH_SYMBOL — deferred (wasm parser trap)
|
||||
|
||||
The robust approach is the s-expr clipboard blob (0001 §3 mechanism #2): serialize a
|
||||
single item with `SCH_IO_KICAD_SEXPR::Format(SCH_SELECTION*, …, /*clipboard*/ true)` and
|
||||
reconstruct with `LoadContent`. Emit works (`itemToSexpr` produced a valid
|
||||
`(symbol(lib_id …))` blob). **Reconstruction traps**: `LoadContent` → the s-expr parser
|
||||
hits `RuntimeError: table index is out of bounds` via `invoke_iii` (the exception
|
||||
trampoline) when called from the apply/`CallAfter` context — even though the *same
|
||||
parser* works during file load. This is the same Asyncify + indirect-call family as the
|
||||
original `SCH_ITEM::Move` trap; the dyncall shim only catches `"signature mismatch"`, not
|
||||
`"table index out of bounds"`, and a C++ `catch(...)` cannot catch a wasm trap (it
|
||||
escapes and leaves `s_applyingRemote` stuck true). So the blob path was removed. Symbol
|
||||
**position/move already syncs** via the generic `changed` path; only live symbol-**add**
|
||||
is unsupported. Next step for symbols: either (a) extend the dyncall/invoke shim to fall
|
||||
back to `getWasmTableEntry` on `"table index out of bounds"` too, or (b) route the parse
|
||||
through a tool coroutine, then re-enable the blob converter for SCH_SYMBOL (+ bus
|
||||
entries, tables, sheets).
|
||||
|
||||
## Shared blocker — the asyncify `invoke_*` mis-dispatch from the apply context
|
||||
|
||||
Three deferred items (SCH_SHAPE add, SCH_SYMBOL add, and historically `SCH_ITEM::Move`)
|
||||
all hit the same root cause: a virtual call dispatched through an asyncify-instrumented
|
||||
`invoke_*` / `dynCall_*` trampoline **mis-dispatches** when invoked from the programmatic
|
||||
`CallAfter`/apply context (it works from the file-load / main-loop-boot context). The
|
||||
dyncall shim (`scripts/common/shims/dyncall-binding.js.tmpl`) fixed the **`Move`** case
|
||||
because that one trapped with `"indirect call signature mismatch"` *at the call boundary*
|
||||
(safe to fall back to `getWasmTableEntry`). The shape (`invoke_viii` → "memory access out
|
||||
of bounds") and symbol (`invoke_iii` → "table index is out of bounds") cases trap *inside*
|
||||
the mis-dispatched function, so a blind retry is unsafe. **Next step for both:** either
|
||||
extend the shim to catch these at the boundary, or route apply through the TOOL_MANAGER so
|
||||
it runs inside a tool coroutine (the asyncify root real edits use) — then re-enable the
|
||||
SCH_SHAPE hand converter and the SCH_SYMBOL s-expr-blob converter.
|
||||
|
||||
## Files touched
|
||||
|
||||
- `wasm/bindings/eeschema_embind.cpp` — SCH_SHAPE emit fields (itemToJson), defensive
|
||||
SCH_TEXT construction (SetParent + default size). SCH_SHAPE/SCH_SYMBOL `added`
|
||||
reconstruction documented + deferred. (No kicad-fork change.)
|
||||
- `tests/kicad/eeschema-collab.spec.ts` — un-skipped + extended the single-page apply
|
||||
test (move / delete / text-add).
|
||||
|
|
@ -107,12 +107,13 @@ test.describe("eeschema collab bridge — single page", () => {
|
|||
expect(hasAbort(testLogger), "no WASM abort").toBe(false);
|
||||
});
|
||||
|
||||
// SKIP headless: verified working in the real web app (wire move/add/remove syncs A↔B),
|
||||
// but the e2e harness's kicadOpenFile returns false (OpenProjectFiles bails before the
|
||||
// connectivity graph is built — files-io.cpp), so SCH_COMMIT::Push no-ops here. Re-enable
|
||||
// once the harness loads a full project. (Crash-free + no-echo still hold; the move just
|
||||
// doesn't persist in this incomplete-load editor.)
|
||||
test.skip("apply moves/removes by uuid, no echo", async ({ page, testLogger }) => {
|
||||
// Apply mutates the model headless: kicadOpenFile returns false (the incomplete-project
|
||||
// load skips some late steps) but the schematic + screens ARE built, so SCH_COMMIT::Push
|
||||
// takes effect. (Earlier this was skipped on the belief Push no-ops headless; that predated
|
||||
// the dyncall-shim fix — apply now works here. Rendering still needs the real app.)
|
||||
const TEXT_ID = "33333333-0000-0000-0000-000000000001";
|
||||
|
||||
test("apply moves/removes/adds by uuid, no echo", async ({ page, testLogger }) => {
|
||||
await bootAndOpen(page, "apply");
|
||||
|
||||
const before = await page.evaluate((id) => window.Module.kicadCollabGetPos(id), WIRE1);
|
||||
|
|
@ -126,11 +127,17 @@ test.describe("eeschema collab bridge — single page", () => {
|
|||
};
|
||||
});
|
||||
|
||||
// Move WIRE1 (deferred via CallAfter+coroutine → poll).
|
||||
// changed: move WIRE1. A wire reshapes via its endpoints (SetStart/EndPoint) — the same
|
||||
// sx/sy/ex/ey form the emit side always produces for a SCH_LINE (a bare x/y Move is a no-op
|
||||
// for a line, whose endpoints only move when flagged). Deferred via CallAfter → poll.
|
||||
await page.evaluate(
|
||||
({ id, nx, by }) =>
|
||||
window.Module.kicadCollabApply(
|
||||
JSON.stringify({ changed: [{ id, type: "SCH_LINE", x: nx, y: by }], added: [], removed: [] }),
|
||||
JSON.stringify({
|
||||
changed: [{ id, type: "SCH_LINE", sx: nx, sy: by, ex: nx + 508000, ey: by }],
|
||||
added: [],
|
||||
removed: [],
|
||||
}),
|
||||
),
|
||||
{ id: WIRE1, nx, by },
|
||||
);
|
||||
|
|
@ -141,7 +148,7 @@ test.describe("eeschema collab bridge — single page", () => {
|
|||
})
|
||||
.toBe(`${nx},${by}`);
|
||||
|
||||
// Remove WIRE2.
|
||||
// removed: delete WIRE2.
|
||||
await page.evaluate(
|
||||
(wire) =>
|
||||
window.Module.kicadCollabApply(JSON.stringify({ changed: [], added: [], removed: [wire] })),
|
||||
|
|
@ -154,6 +161,27 @@ test.describe("eeschema collab bridge — single page", () => {
|
|||
})
|
||||
.toBe("");
|
||||
|
||||
// added: a graphic text reconstructs by uuid. (SCH_SHAPE / SCH_SYMBOL `added` are deferred —
|
||||
// committing a newly-constructed shape/symbol traps via the asyncify invoke_* mis-dispatch
|
||||
// in SCH_COMMIT::Push from the programmatic apply context; see features/yjs-bridge/0006.)
|
||||
await page.evaluate(
|
||||
(textId) =>
|
||||
window.Module.kicadCollabApply(
|
||||
JSON.stringify({
|
||||
changed: [],
|
||||
removed: [],
|
||||
added: [{ id: textId, type: "SCH_TEXT", x: 600000, y: 600000, text: "hello" }],
|
||||
}),
|
||||
),
|
||||
TEXT_ID,
|
||||
);
|
||||
await expect
|
||||
.poll(
|
||||
async () => (await page.evaluate(() => window.Module.kicadCollabSnapshot())).includes(TEXT_ID),
|
||||
{ timeout: 10000, intervals: [250] },
|
||||
)
|
||||
.toBe(true);
|
||||
|
||||
const echoes = await page.evaluate(() => (window as unknown as { __echo: string[] }).__echo);
|
||||
expect(echoes, "apply() must not echo a local onDelta").toHaveLength(0);
|
||||
expect(hasAbort(testLogger), "no WASM abort").toBe(false);
|
||||
|
|
|
|||
|
|
@ -29,8 +29,12 @@
|
|||
#include <sch_no_connect.h>
|
||||
#include <sch_text.h>
|
||||
#include <sch_label.h>
|
||||
#include <sch_shape.h>
|
||||
#include <eda_shape.h>
|
||||
#include <stroke_params.h>
|
||||
#include <sch_screen.h>
|
||||
#include <sch_sheet_path.h>
|
||||
#include <schematic_settings.h>
|
||||
|
||||
using namespace emscripten;
|
||||
using json = nlohmann::json;
|
||||
|
|
@ -110,6 +114,35 @@ json itemToJson( SCH_ITEM* aItem )
|
|||
if( SCH_LABEL_BASE* lbl = dynamic_cast<SCH_LABEL_BASE*>( aItem ) )
|
||||
j["shape"] = (int) lbl->GetShape();
|
||||
|
||||
// Graphic shapes (circle / rectangle / arc / line / bezier): geometry is start/end plus,
|
||||
// for an arc, the center, and for a bezier the two control points. Stroke + fill complete it.
|
||||
if( aItem->Type() == SCH_SHAPE_T )
|
||||
{
|
||||
auto* shp = static_cast<SCH_SHAPE*>( aItem );
|
||||
j["stype"] = (int) shp->GetShape(); // SHAPE_T
|
||||
j["sx"] = shp->GetStart().x;
|
||||
j["sy"] = shp->GetStart().y;
|
||||
j["ex"] = shp->GetEnd().x;
|
||||
j["ey"] = shp->GetEnd().y;
|
||||
j["layer"] = (int) shp->GetLayer();
|
||||
j["width"] = shp->GetStroke().GetWidth();
|
||||
j["fill"] = (int) shp->GetFillMode();
|
||||
|
||||
if( shp->GetShape() == SHAPE_T::ARC )
|
||||
{
|
||||
VECTOR2I c = shp->GetCenter();
|
||||
j["cx"] = c.x;
|
||||
j["cy"] = c.y;
|
||||
}
|
||||
else if( shp->GetShape() == SHAPE_T::BEZIER )
|
||||
{
|
||||
j["c1x"] = shp->GetBezierC1().x;
|
||||
j["c1y"] = shp->GetBezierC1().y;
|
||||
j["c2x"] = shp->GetBezierC2().x;
|
||||
j["c2y"] = shp->GetBezierC2().y;
|
||||
}
|
||||
}
|
||||
|
||||
return j;
|
||||
}
|
||||
|
||||
|
|
@ -138,8 +171,20 @@ SCH_ITEM* makeItem( const json& j )
|
|||
}
|
||||
else if( type == "SCH_TEXT" )
|
||||
{
|
||||
item = new SCH_TEXT( VECTOR2I( j.value( "x", 0 ), j.value( "y", 0 ) ),
|
||||
wxString::FromUTF8( j.value( "text", "" ).c_str() ) );
|
||||
auto* txt = new SCH_TEXT( VECTOR2I( j.value( "x", 0 ), j.value( "y", 0 ) ),
|
||||
wxString::FromUTF8( j.value( "text", "" ).c_str() ) );
|
||||
|
||||
// Mirror the interactive text tool (sch_drawing_tools.cpp createNewText): parent the
|
||||
// item to the schematic and apply the project's default text size, so a remotely-added
|
||||
// text resolves variables / renders identically to a locally-placed one.
|
||||
if( SCH_EDIT_FRAME* fr = schFrame() )
|
||||
{
|
||||
txt->SetParent( &fr->Schematic() );
|
||||
int sz = fr->Schematic().Settings().m_DefaultTextSize;
|
||||
txt->SetTextSize( VECTOR2I( sz, sz ) );
|
||||
}
|
||||
|
||||
item = txt;
|
||||
}
|
||||
else if( type == "SCH_LABEL" || type == "SCH_GLOBALLABEL" || type == "SCH_HIERLABEL" )
|
||||
{
|
||||
|
|
@ -155,6 +200,17 @@ SCH_ITEM* makeItem( const json& j )
|
|||
|
||||
item = lbl;
|
||||
}
|
||||
// SCH_SHAPE `added` reconstruction is DEFERRED (returns nullptr → logged "no converter").
|
||||
// A constructed shape builds fine, but committing a *new* one traps in SCH_COMMIT::Push's
|
||||
// CHT_ADD path (the GAL view->Add of a new SCH_SHAPE) with a wasm "memory access out of
|
||||
// bounds". The identical view->Add succeeds when the shape is loaded from file and when an
|
||||
// existing shape is MOVED (CHT_MODIFY) — so this is the asyncify indirect-call (invoke_viii)
|
||||
// mis-dispatch class: a SCH_SHAPE add-path virtual that mis-dispatches only from the
|
||||
// programmatic CallAfter/apply context (same family as the original SCH_ITEM::Move trap,
|
||||
// 0003). The dyncall shim only catches the "signature mismatch" variant, and this trap
|
||||
// fires inside the wrong function so it can't be safely retried. itemToJson still emits the
|
||||
// shape geometry (forward-compatible + drives `changed`/move); only `added` is skipped.
|
||||
// Same blocker as SCH_SYMBOL add — see features/yjs-bridge/0006.
|
||||
|
||||
if( item )
|
||||
const_cast<KIID&>( item->m_Uuid ) = KIID( wxString::FromUTF8( j.value( "id", "" ).c_str() ) );
|
||||
|
|
|
|||
Loading…
Reference in a new issue