From b672c5418945d0be21380d8afa5bfcfa24e6553f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Tue, 9 Jun 2026 06:03:17 +0200 Subject: [PATCH] =?UTF-8?q?fix(pcbnew):=20collab=20syncs=20real=20item=20l?= =?UTF-8?q?ayer=20=E2=80=94=20devirtualize=20Get/SetLayer;=20collab=20on?= =?UTF-8?q?=20by=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/kicad/pcbnew-collab.spec.ts | 5 ++++ wasm/bindings/pcbnew_embind.cpp | 27 ++++++++++++++++--- web/apps/frontend/src/components/WasmTool.tsx | 20 +++++++------- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/tests/kicad/pcbnew-collab.spec.ts b/tests/kicad/pcbnew-collab.spec.ts index c27fbdc..9e32b61 100644 --- a/tests/kicad/pcbnew-collab.spec.ts +++ b/tests/kicad/pcbnew-collab.spec.ts @@ -191,6 +191,11 @@ test.describe("pcbnew collab bridge — single page", () => { expect((byId.get(TEXT1) as { text?: string }).text, "board text string emitted").toBe( "BOARDTEXT", ); + // The text is on F.SilkS (=5). Asserting the ACTUAL layer (not just "round-trips") catches the + // GetLayer() emit bug, where every item reported layer 0 (F.Cu) and added items landed on the + // wrong copper layer on the peer. + expect((byId.get(TEXT1) as { layer?: number }).layer, "board text on F.SilkS (not stuck at 0)").toBe(5); + expect((byId.get(SEG1) as { layer?: number }).layer, "segment on F.Cu").toBe(0); expect(hasAbort(testLogger), "no WASM abort").toBe(false); }); diff --git a/wasm/bindings/pcbnew_embind.cpp b/wasm/bindings/pcbnew_embind.cpp index d320974..e202c6c 100644 --- a/wasm/bindings/pcbnew_embind.cpp +++ b/wasm/bindings/pcbnew_embind.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -107,6 +108,20 @@ bool isTrackType( KICAD_T t ) return t == PCB_TRACE_T || t == PCB_ARC_T || t == PCB_VIA_T; } +// Read an item's layer WITHOUT the virtual GetLayer(). That virtual mis-dispatches in the +// non-coroutine emit/snapshot context — it returns 0 (F_Cu) for EVERY item (the same asyncify +// call_indirect class as eeschema's Move()), which silently put every collab-added item/track/ +// text on the top copper layer on the peer. A class-qualified `BOARD_ITEM::GetLayer()` is a +// statically-bound (direct) call that just reads m_layer, bypassing call_indirect. Zones keep +// their layer in m_layerSet (not m_layer), so use their non-virtual GetFirstLayer(). +int itemLayer( BOARD_ITEM* aItem ) +{ + if( aItem->Type() == PCB_ZONE_T ) + return (int) static_cast( aItem )->GetFirstLayer(); + + return (int) aItem->BOARD_ITEM::GetLayer(); +} + // Iterate every board item the bridge syncs: the top-level items (tracks, footprints, drawings, // zones, groups) PLUS each footprint's TEXT children (fields = reference/value/user, and graphic // PCB_TEXT). The text children are visited by their OWN uuid because a silkscreen reference/value @@ -155,7 +170,7 @@ json itemToJson( BOARD_ITEM* aItem ) { "type", toUtf8( aItem->GetClass() ) }, { "x", p.x }, // internal units (nm); integral, no quantization needed { "y", p.y }, - { "layer", (int) aItem->GetLayer() }, + { "layer", itemLayer( aItem ) }, // devirtualized — aItem->GetLayer() mis-dispatches here }; if( isTrackType( aItem->Type() ) ) @@ -313,7 +328,9 @@ BOARD_ITEM* makeItem( BOARD& aBoard, const json& j ) tr->SetStart( VECTOR2I( j.value( "sx", 0 ), j.value( "sy", 0 ) ) ); tr->SetEnd( VECTOR2I( j.value( "ex", 0 ), j.value( "ey", 0 ) ) ); tr->SetWidth( j.value( "width", 0 ) ); - tr->SetLayer( (PCB_LAYER_ID) j.value( "layer", (int) F_Cu ) ); + // SetLayer is virtual and (like GetLayer) mis-dispatches in this apply context → it no-ops, + // leaving the item on the default layer. Class-qualify to a direct m_layer write. + tr->BOARD_ITEM::SetLayer( (PCB_LAYER_ID) j.value( "layer", (int) F_Cu ) ); item = tr; } // Via / zone: reconstruct natively from emitted geometry (the envelope-blob parse is @@ -340,7 +357,9 @@ BOARD_ITEM* makeItem( BOARD& aBoard, const json& j ) outline.emplace_back( p[0].get(), p[1].get() ); } - zone->SetLayer( (PCB_LAYER_ID) j.value( "layer", (int) F_Cu ) ); + // Zones keep their layer in m_layerSet via SetLayer→SetLayerSet (both virtual, both no-op + // here). Class-qualify ZONE::SetLayerSet to set it directly (else GetFirstLayer == -1). + zone->ZONE::SetLayerSet( LSET( { (PCB_LAYER_ID) j.value( "layer", (int) F_Cu ) } ) ); if( outline.size() >= 3 ) zone->AddPolygon( outline ); @@ -352,7 +371,7 @@ BOARD_ITEM* makeItem( BOARD& aBoard, const json& j ) auto* txt = new PCB_TEXT( &aBoard ); txt->SetText( wxString::FromUTF8( j.value( "text", "" ).c_str() ) ); txt->SetPosition( VECTOR2I( j.value( "x", 0 ), j.value( "y", 0 ) ) ); - txt->SetLayer( (PCB_LAYER_ID) j.value( "layer", (int) F_SilkS ) ); + txt->BOARD_ITEM::SetLayer( (PCB_LAYER_ID) j.value( "layer", (int) F_SilkS ) ); // devirt if( j.contains( "tw" ) ) txt->SetTextSize( VECTOR2I( j.value( "tw", 0 ), j.value( "th", 0 ) ) ); diff --git a/web/apps/frontend/src/components/WasmTool.tsx b/web/apps/frontend/src/components/WasmTool.tsx index 4b49d67..2b7affe 100644 --- a/web/apps/frontend/src/components/WasmTool.tsx +++ b/web/apps/frontend/src/components/WasmTool.tsx @@ -12,11 +12,12 @@ import { clog, cwarn } from "@/wasm/collab/debug"; const COLLAB_TOOLS = new Set(["pl_editor", "eeschema", "pcbnew"]); /** - * Opt-in collaborative editing (features/yjs-bridge). Enabled when the URL carries - * `?collab=1` and the tool has the collab bridge. Open the same project URL in two tabs - * with `?collab=1` to edit together: the channel is keyed to project+file, so both tabs - * share one Y.Doc over BroadcastChannel. Editor edits (add/move items) fire the tool's - * change hook → the bridge → the peer tab. + * Collaborative editing (features/yjs-bridge), ON BY DEFAULT for any tool that has the + * collab bridge. Open the same project URL in two tabs to edit together: the channel is + * keyed to project+file, so both tabs share one Y.Doc over BroadcastChannel. Editor edits + * (add/move items) fire the tool's change hook → the bridge → the peer tab. + * + * Opt OUT with `?collab=0` (or `collab=false`). Tools without a bridge are skipped anyway. */ async function maybeStartCollab( win: ToolWindow, @@ -28,10 +29,10 @@ async function maybeStartCollab( onStatus: (t: string) => void; }, ): Promise { - const enabled = new URLSearchParams(win.location.search).get("collab"); + const collabParam = new URLSearchParams(win.location.search).get("collab"); const mod = win.Module; clog("maybeStartCollab gate:", { - collabParam: enabled, + collabParam, tool: opts.tool, hasModule: !!mod, hasSnapshot: typeof mod?.kicadCollabSnapshot, @@ -39,8 +40,9 @@ async function maybeStartCollab( url: win.location.href, }); - if (!enabled || enabled === "0" || enabled === "false") { - clog("disabled (no ?collab=1) — skipping"); + // On by default; only an explicit opt-out disables it. + if (collabParam === "0" || collabParam === "false") { + clog("disabled (?collab=0) — skipping"); return; } if (!COLLAB_TOOLS.has(opts.tool)) {