From 9dbfddc5270be8fab09d07bf89731de4a6bbd551 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Mon, 8 Jun 2026 11:39:29 +0200 Subject: [PATCH] feat(pcbnew): collab adds footprints (s-expr blob) + vias/zones (native) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Footprint add reconstructs from the bare `(footprint …)` s-expr clipboard blob via CLIPBOARD_IO (SetWriter/SetReader redirect it to a string for headless/wasm). The non-footprint `(kicad_pcb …)` envelope parse (parseBOARD) is asyncify-fragile in wasm — commit.Add of an envelope-parsed item silently stages nothing and subsequent virtual dispatch traps ("index out of bounds"), the same wall that deferred the eeschema symbol blob — so vias and zones reconstruct NATIVELY instead: itemToJson emits drill/layer-pair for a PCB_VIA and the outline polygon for a ZONE, and makeItem builds a fresh PCB_VIA / ZONE. flushDiff attaches the blob only to types that need it (footprints/board graphics); tracks/vias/zones skip it. Includes the dangling-parent fix for the envelope path (SetParent before delete) which footprints don't hit. Build: kicad_clipboard.h pulls in the generated pcb_lexer.h (emitted into the common build subdir by make_lexer) — added -I${KICAD_BUILD}/common to the embind include path. Tests: sample board gains a via + zone; snapshot asserts their native fields are emitted, and three round-trip add tests (footprint via blob, via/zone native) delete then re-add each by uuid and confirm it returns at the same position. 6 passed, 1 skipped, 0 aborts. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/kicad/build-kicad-target.sh | 3 + tests/kicad/pcbnew-collab.spec.ts | 74 ++++++++++ wasm/bindings/pcbnew_embind.cpp | 204 ++++++++++++++++++++++++++-- 3 files changed, 272 insertions(+), 9 deletions(-) diff --git a/scripts/kicad/build-kicad-target.sh b/scripts/kicad/build-kicad-target.sh index 6ace02b..c322f0d 100755 --- a/scripts/kicad/build-kicad-target.sh +++ b/scripts/kicad/build-kicad-target.sh @@ -395,6 +395,9 @@ if [ -f "${EMBIND_SRC}" ]; then log_info "Compiling Embind bindings (${APP_NAME})..." # Use the same includes and flags that KiCad uses KICAD_INCLUDES="-I${KICAD_BUILD} -I${KICAD_DIR}/include -I${KICAD_DIR}/${KICAD_SUBDIR} -I${KICAD_DIR}/common" + # Generated DSN-lexer headers (e.g. pcb_lexer.h, used transitively via kicad_clipboard.h → + # pcb_io_kicad_sexpr_parser.h) are emitted into the common build subdir by make_lexer. + KICAD_INCLUDES+=" -I${KICAD_BUILD}/common" KICAD_INCLUDES+=" -I${KICAD_DIR}/libs/core/include -I${KICAD_DIR}/libs/kimath/include -I${KICAD_DIR}/libs/kiplatform/include" KICAD_INCLUDES+=" -I${KICAD_DIR}/thirdparty/clipper2/Clipper2Lib/include" KICAD_INCLUDES+=" -I${KICAD_DIR}/thirdparty/nlohmann_json" diff --git a/tests/kicad/pcbnew-collab.spec.ts b/tests/kicad/pcbnew-collab.spec.ts index 49e2a70..b04a2cf 100644 --- a/tests/kicad/pcbnew-collab.spec.ts +++ b/tests/kicad/pcbnew-collab.spec.ts @@ -26,6 +26,8 @@ const SEG2 = "44444444-0000-0000-0000-000000000002"; const FP1 = "66666666-0000-0000-0000-000000000001"; const FP1_REF = "66666666-0000-0000-0000-0000000000aa"; // Reference field (PCB_FIELD, F.SilkS) const FP1_TXT = "66666666-0000-0000-0000-0000000000cc"; // user fp_text (PCB_TEXT, F.SilkS) +const VIA1 = "77777777-0000-0000-0000-000000000001"; // a through via (PCB_VIA) +const ZONE1 = "77777777-0000-0000-0000-000000000002"; // a copper zone (ZONE) const SAMPLE_PCB = `(kicad_pcb \t(version 20241229) \t(generator "pcbnew") @@ -66,6 +68,17 @@ const SAMPLE_PCB = `(kicad_pcb \t\t\t(effects (font (size 1 1) (thickness 0.15))) \t\t) \t) +\t(via (at 80 80) (size 1.4) (drill 0.6) (layers "F.Cu" "B.Cu") (net 0) (uuid "${VIA1}")) +\t(zone +\t\t(net 0) +\t\t(net_name "") +\t\t(layer "F.Cu") +\t\t(uuid "${ZONE1}") +\t\t(hatch edge 0.5) +\t\t(connect_pads (clearance 0)) +\t\t(min_thickness 0.25) +\t\t(polygon (pts (xy 60 110) (xy 75 110) (xy 75 125) (xy 60 125))) +\t) \t(segment (start 50.8 50.8) (end 101.6 50.8) (width 0.2) (layer "F.Cu") (net 0) (uuid "${SEG1}")) \t(segment (start 50.8 76.2) (end 101.6 76.2) (width 0.2) (layer "F.Cu") (net 0) (uuid "${SEG2}")) ) @@ -78,6 +91,7 @@ type Mod = { kicadCollabApply(j: string): unknown; kicadCollabTestMoveFirst(dx: number, dy: number): string; kicadCollabGetPos(id: string): string; + kicadCollabTestItemBlob(id: string): string; }; function hasAbort(l: { consoleLogs: string[]; errors: string[] }): boolean { @@ -155,9 +169,69 @@ test.describe("pcbnew collab bridge — single page", () => { expect(byId.get(FP1_REF)!.type).toBe("PCB_FIELD"); expect(byId.has(FP1_TXT), "footprint user fp_text present").toBe(true); expect(byId.get(FP1_TXT)!.type).toBe("PCB_TEXT"); + // Via/zone carry the native geometry their `added` reconstruction needs (no blob path). + expect(byId.has(VIA1), "via present").toBe(true); + expect(byId.get(VIA1)!.type).toBe("PCB_VIA"); + expect((byId.get(VIA1) as { drill?: number }).drill, "via drill emitted").toBeGreaterThan(0); + expect(byId.has(ZONE1), "zone present").toBe(true); + expect(byId.get(ZONE1)!.type).toBe("ZONE"); + expect( + (byId.get(ZONE1) as { poly?: number[][] }).poly?.length, + "zone outline emitted", + ).toBeGreaterThanOrEqual(3); expect(hasAbort(testLogger), "no WASM abort").toBe(false); }); + // `added` reconstruction of a footprint, via and zone. The emit side attaches BOTH the full + // itemToJson fields AND an s-expr clipboard blob; makeItem then reconstructs a footprint from + // the bare `(footprint …)` blob, and a via/zone NATIVELY from the geometry fields (the + // `(kicad_pcb …)` envelope parse is asyncify-fragile in wasm for those). Round-trip each: read + // its full snapshot item + blob, delete it, re-add, confirm it returns at the same position. + for (const [label, id, type] of [ + ["footprint", FP1, "FOOTPRINT"], + ["via", VIA1, "PCB_VIA"], + ["zone", ZONE1, "ZONE"], + ] as const) { + test(`apply adds a ${label} (footprint via blob, via/zone native)`, async ({ page, testLogger }) => { + await bootAndOpen(page, `add-${label}`); + + // Full emit-equivalent payload: snapshot item (native geometry fields) + the clipboard blob. + const payload = await page.evaluate((i) => { + const snap = JSON.parse(window.Module.kicadCollabSnapshot()); + const item = snap.added.find((it: { id: string }) => it.id === i); + return { ...item, sexpr: window.Module.kicadCollabTestItemBlob(i) }; + }, id); + expect(payload.id, `${label} in snapshot`).toBe(id); + const posBefore = await page.evaluate((i) => window.Module.kicadCollabGetPos(i), id); + expect(posBefore, `${label} resolvable before`).not.toBe(""); + + // delete it + await page.evaluate( + (i) => window.Module.kicadCollabApply(JSON.stringify({ added: [], changed: [], removed: [i] })), + id, + ); + await expect + .poll(() => page.evaluate((i) => window.Module.kicadCollabGetPos(i), id), { + timeout: 10000, + intervals: [200], + }) + .toBe(""); + + // re-add it + await page.evaluate( + (p) => window.Module.kicadCollabApply(JSON.stringify({ added: [p], changed: [], removed: [] })), + payload, + ); + await expect + .poll(() => page.evaluate((i) => window.Module.kicadCollabGetPos(i), id), { + timeout: 10000, + intervals: [200], + }) + .toBe(posBefore); + expect(hasAbort(testLogger), "no WASM abort").toBe(false); + }); + } + // Apply mutates the model headless: kicadOpenFile returns false (the incomplete-project load // skips some late steps) but the board IS built, so BOARD_COMMIT::Push takes effect. Rendering // still needs the real app. (Same headless reality as the eeschema apply test.) diff --git a/wasm/bindings/pcbnew_embind.cpp b/wasm/bindings/pcbnew_embind.cpp index 6361247..e67f1be 100644 --- a/wasm/bindings/pcbnew_embind.cpp +++ b/wasm/bindings/pcbnew_embind.cpp @@ -24,6 +24,10 @@ #include #include #include +#include +#include +#include +#include #include #include #include @@ -163,6 +167,36 @@ json itemToJson( BOARD_ITEM* aItem ) j["width"] = tr->GetWidth(); } + // Vias and zones reconstruct NATIVELY on `added` (the s-expr clipboard blob's `(kicad_pcb …)` + // envelope parse — used for footprints — is asyncify-fragile in wasm for these, the same wall + // that deferred the eeschema symbol blob). So emit the geometry their makeItem needs. + if( aItem->Type() == PCB_VIA_T ) + { + auto* via = static_cast( aItem ); + j["drill"] = via->GetDrillValue(); + j["ltop"] = (int) via->TopLayer(); + j["lbot"] = (int) via->BottomLayer(); + } + else if( aItem->Type() == PCB_ZONE_T ) + { + auto* zone = static_cast( aItem ); + const SHAPE_POLY_SET* poly = zone->Outline(); + json pts = json::array(); + + if( poly && poly->OutlineCount() > 0 ) + { + const SHAPE_LINE_CHAIN& chain = poly->COutline( 0 ); + + for( int i = 0; i < chain.PointCount(); ++i ) + { + const VECTOR2I& p = chain.CPoint( i ); + pts.push_back( { p.x, p.y } ); + } + } + + j["poly"] = pts; + } + // Text items (incl. footprint fields / graphic text): carry the string so a move diff is // legible and a future text `added` can reconstruct. Position-only sync uses x/y above. if( EDA_TEXT* txt = dynamic_cast( aItem ) ) @@ -171,16 +205,89 @@ json itemToJson( BOARD_ITEM* aItem ) return j; } +// ── s-expr clipboard blob (the generic `added` mechanism) ──────────────────────────────────── +// +// For added items beyond the natively-reconstructed PCB_TRACK (footprints, vias, zones, graphic +// shapes/text…), reuse KiCad's own copy/paste serializer, CLIPBOARD_IO. It Format()s a one-item +// selection exactly as Ctrl-C does — a bare `(footprint …)` for a footprint, or a fake +// `(kicad_pcb … )` envelope for everything else (the bare item tokens like +// `(segment`/`(via`/`(zone` are NOT accepted by the parser top-level, so the envelope is +// required). CLIPBOARD_IO normally talks to the system clipboard; SetWriter/SetReader redirect +// it to a string so it works headless / in wasm. + +// Serialize one live board item to a clipboard blob (used only for `added` payloads — NOT the +// diff unit, so `changed`/`removed` stay light and the blob never drives change detection). +std::string blobForItem( BOARD* aBoard, BOARD_ITEM* aItem ) +{ + PCB_SELECTION sel; + sel.Add( aItem ); // pointer-only insert; no mutation of the live item + + CLIPBOARD_IO io; + io.SetBoard( aBoard ); + + std::string out; + io.SetWriter( [&out]( const wxString& s ) { out = std::string( s.utf8_str() ); } ); + io.SaveSelection( sel, /*isFootprintEditor*/ false ); + return out; +} + +// Reconstruct a board item from a clipboard blob. Parse() returns a bare FOOTPRINT*, or a BOARD* +// (the `(kicad_pcb …)` envelope) holding the single item — in which case detach that item from +// the throw-away board and hand back ownership. Returns nullptr on a parse failure (Parse catches +// internally) or if no item is found. Runs inside the apply COROUTINE. +BOARD_ITEM* makeFromBlob( BOARD& aBoard, const std::string& aBlob ) +{ + if( aBlob.empty() ) + return nullptr; + + CLIPBOARD_IO io; + io.SetBoard( &aBoard ); + io.SetReader( [&aBlob]() -> wxString { return wxString::FromUTF8( aBlob.c_str() ); } ); + + BOARD_ITEM* parsed = io.Parse(); // FOOTPRINT* (bare) | BOARD* (envelope) | nullptr + + if( !parsed ) + return nullptr; + + if( parsed->Type() != PCB_T ) + return parsed; // bare footprint — ready to commit.Add + + // Envelope board: remap its net codes onto ours, then lift out the single item it carries. + BOARD* clip = static_cast( parsed ); + clip->MapNets( &aBoard ); + + BOARD_ITEM* found = nullptr; + + if( !clip->Tracks().empty() ) found = clip->Tracks().front(); // track / via / arc + else if( !clip->Zones().empty() ) found = clip->Zones().front(); + else if( !clip->Drawings().empty() ) found = clip->Drawings().front(); // shape / text / … + else if( !clip->Footprints().empty() ) found = clip->Footprints().front(); + else if( !clip->Groups().empty() ) found = clip->Groups().front(); + + if( found ) + { + clip->Remove( found ); // detach so clip's dtor doesn't delete it + // Reparent onto the REAL board before clip is freed: the item's m_parent still points at + // clip, and commit.Push/saveCopyInUndoList dereferences GetParent() — a dangling pointer + // here is what trapped via add ("index out of bounds") and tripped the zone undo assert. + found->SetParent( &aBoard ); + found->SetParentGroup( nullptr ); + } + + delete clip; + return found; +} + // Construct a new BOARD_ITEM from a delta item (for `added`), with the delta's uuid (m_Uuid is -// const → const_cast, exactly as the s-expr parser does). Returns nullptr for types without a -// converter yet (footprints/vias/zones — deferred, see header). PCB_TRACK segments reconstruct -// natively (no clipboard Parse), so the add path is trap-free for the common collab case. +// const → const_cast, exactly as the s-expr parser does). PCB_TRACK segments reconstruct natively +// from their fields (cheap, trap-free); every other type goes through the s-expr clipboard blob +// (`sexpr`, attached to added payloads by the emit side). Returns nullptr if neither applies. BOARD_ITEM* makeItem( BOARD& aBoard, const json& j ) { std::string type = j.value( "type", "" ); BOARD_ITEM* item = nullptr; - if( type == "PCB_TRACK" ) + if( type == "PCB_TRACK" && j.contains( "sx" ) ) { auto* tr = new PCB_TRACK( &aBoard ); tr->SetStart( VECTOR2I( j.value( "sx", 0 ), j.value( "sy", 0 ) ) ); @@ -189,10 +296,44 @@ BOARD_ITEM* makeItem( BOARD& aBoard, const json& j ) tr->SetLayer( (PCB_LAYER_ID) j.value( "layer", (int) F_Cu ) ); item = tr; } - // PCB_VIA / PCB_ARC / FOOTPRINT / ZONE `added` deferred (need layer-pair/drill, arc center, - // or a library / s-expr clipboard blob — same deferred class as SCH_SYMBOL). Their move/ - // delete already sync via the generic changed/removed paths. + // Via / zone: reconstruct natively from emitted geometry (the envelope-blob parse is + // asyncify-fragile for these — see itemToJson). The blob is still emitted as a fallback. + else if( type == "PCB_VIA" && j.contains( "drill" ) ) + { + auto* via = new PCB_VIA( &aBoard ); + VECTOR2I c( j.value( "x", 0 ), j.value( "y", 0 ) ); + via->SetPosition( c ); + via->SetWidth( j.value( "width", 0 ) ); + via->SetDrill( j.value( "drill", 0 ) ); + via->SetLayerPair( (PCB_LAYER_ID) j.value( "ltop", (int) F_Cu ), + (PCB_LAYER_ID) j.value( "lbot", (int) B_Cu ) ); + item = via; + } + else if( type == "ZONE" && j.contains( "poly" ) ) + { + auto* zone = new ZONE( &aBoard ); + std::vector outline; + for( const json& p : j["poly"] ) + { + if( p.is_array() && p.size() == 2 ) + outline.emplace_back( p[0].get(), p[1].get() ); + } + + zone->SetLayer( (PCB_LAYER_ID) j.value( "layer", (int) F_Cu ) ); + + if( outline.size() >= 3 ) + zone->AddPolygon( outline ); + + item = zone; + } + else if( j.contains( "sexpr" ) ) + { + item = makeFromBlob( aBoard, j.value( "sexpr", "" ) ); + } + + // Force the delta's uuid (the blob already carries the sender's uuid for the item and any + // children, but set the top-level one explicitly to be certain peers agree on identity). if( item ) const_cast( item->m_Uuid ) = KIID( wxString::FromUTF8( j.value( "id", "" ).c_str() ) ); @@ -275,7 +416,8 @@ void flushDiff() if( !fr ) return; - std::map cur = snapshotByUuid( *fr->GetBoard() ); + BOARD* board = fr->GetBoard(); + std::map cur = snapshotByUuid( *board ); json added = json::array(), changed = json::array(), removed = json::array(); @@ -284,9 +426,31 @@ void flushDiff() auto it = g_baseline.find( id ); if( it == g_baseline.end() ) - added.push_back( j ); + { + // Skip a newly-added footprint's text CHILDREN: the footprint's own add carries them, + // and emitting a lone child would (for a field) wrap it in a spurious footprint. (A + // child-only add onto an existing footprint is therefore not synced yet — rare.) + BOARD_ITEM* live = + board->ResolveItem( KIID( wxString::FromUTF8( id.c_str() ) ), /*allowNull*/ true ); + + if( live && live->GetParentFootprint() ) + continue; + + json withBlob = j; + + // Attach an s-expr clipboard blob ONLY for types makeItem reconstructs from it + // (footprints, board shapes/text, …). Tracks/vias/zones rebuild NATIVELY from the + // fields itemToJson already emitted, so they need no blob — and skipping it avoids a + // wasted SaveSelection plus the asyncify-fragile envelope parse for those. + if( live && !isTrackType( live->Type() ) && live->Type() != PCB_ZONE_T ) + withBlob["sexpr"] = blobForItem( board, live ); + + added.push_back( withBlob ); + } else if( it->second != j ) + { changed.push_back( j ); + } } for( const auto& [id, j] : g_baseline ) @@ -559,6 +723,27 @@ std::string kicadCollabGetPos( std::string aId ) return ""; } + +// Test helper: the s-expr clipboard blob for an item by uuid (what the emit side attaches to an +// `added` payload). Lets the e2e round-trip the blob add path without a real draw. +std::string kicadCollabTestItemBlob( std::string aId ) +{ + PCB_EDIT_FRAME* fr = pcbFrame(); + + if( !fr ) + return ""; + + BOARD* board = fr->GetBoard(); + + if( BOARD_ITEM* item = board->ResolveItem( KIID( wxString::FromUTF8( aId.c_str() ) ), + /*allowNullptr*/ true ) ) + { + return blobForItem( board, item ); + } + + return ""; +} + // Wrapper to return footprints as vector for JS iteration std::vector Board_GetFootprints(BOARD* board) { if (!board) return {}; @@ -632,5 +817,6 @@ EMSCRIPTEN_BINDINGS(pcbnew) { function("kicadCollabSnapshot", &kicadCollabSnapshot); function("kicadCollabTestMoveFirst", &kicadCollabTestMoveFirst); function("kicadCollabGetPos", &kicadCollabGetPos); + function("kicadCollabTestItemBlob", &kicadCollabTestItemBlob); } #endif