AI_Assistant/pcbjam
Watch
1
0
Fork
You've already forked pcbjam
 0
Code Issues Pull requests Projects Releases 4 Packages Wiki Activity Actions 2

fix(3d): blank render + lost position after 3D viewer close/reopen (gl1 context guard)

Browse source 
Closing the viewer destroys its wxGLCanvas's WebGL context; reopening mints a
new one. The gl1 shim cached GL names (FFP program, stream/scratch VBOs) in
never-reset statics behind `if (!handle)` guards — in the new context every
draw died with INVALID_OPERATION and the viewer showed only the clear color
("Reload time 0.031 s" is benign: warm model caches make the rebuild fast).

contextSync() (gl1_state.cpp) now detects the context change in programSync()
— the one choke point every shim draw crosses, and a path the 2D GAL never
reaches (a first attempt checking in the glBindTexture wrap saw the GAL's
context and thrash-rebuilt the program 23x per run) — and drops the cached
names for lazy rebuild in the new context. Context identity is a monotonic id
stamped on Emscripten's per-context record: the numeric
EMSCRIPTEN_WEBGL_CONTEXT_HANDLE is recycled, so a destroy-then-create can
return the same number and a handle comparison detects nothing.

The lost-position half is a wxwidgets wasm fix (pointer bump: GetFromWindow
reports display 0; saved geometry used to carry display=(unsigned)-1, which
LoadWindowState treats as "display not found" and re-centres the frame).

TDD (red observed before each fix, green after):
- tests/kicad/3d-viewer-reopen.spec.ts (new, own worker like the deadlock
  spec): load board, open viewer, render-gate, drag by the titlebar, close
  via the x, reopen; asserts the board re-renders (was: 1 distinct colour for
  90 s) and the window position is restored (was: re-centred to 0,0 after
  closing at 40,90). Green run logs exactly one [gl1] context-change line.
- 3d-regression harness: recreateContext() destroys the context AND swaps in
  a fresh canvas element (a browser canvas keeps its context for life, so
  same-element recreation hands back the live old context and hides the bug);
  the new 3d-webgl spec test renders redraw-mini-board-navigator before and
  after recreation and requires pixel-identical output. Parity: 47/47, zero
  drift.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Istvan Matejcsok 2026-08-24 15:55:31 +02:00
commit 3722891d48
13 changed files with 504 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

202
tests/kicad/3d-viewer-reopen.spec.ts Normal file
View file

@ -0,0 +1,202 @@
import { test, expect } from './fixtures';
import { shotPath } from '../e2e/utils/element-tracker';
import { waitForPcbnew } from './utils/pcbnew-ready';
import { DEMO, loadBoard, countGlCanvases, logThreeDDiag, openThreeDViewer, waitForThreeDRender }
from './utils/threed-viewer';
/**
* Regression: close the 3D viewer, reopen it — it must render again, in the same place.
*
* Closing the viewer Destroy()s the frame; ~wxGLCanvas destroys the WebGL context and its
* DOM canvas (wxwidgets/src/wasm/glcanvas.cpp). Reopening creates a fresh frame + canvas +
* context. Two independent bugs surfaced on that path:
*
* 1. BLANK CANVAS — the wasm/gl1 FFP shim cached GL object names (FFP program, stream and
* scratch VBOs) in file-scope statics guarded by `if (!handle)`, so in the new context it
* kept using names owned by the destroyed context: every draw died with INVALID_OPERATION
* and the reopened viewer showed only the clear color ("Reload time 0.031 s" — the scene
* rebuild itself is fine, warm model caches make it fast). The shim now detects the
* context change and rebuilds its GL objects.
*
* 2. LOST POSITION — wxWindowWasm::GetHandle() returns NULL, so wxDisplay::GetFromWindow()
* was wxNOT_FOUND (-1); EDA_BASE_FRAME::SaveWindowSettings stored display = (unsigned)-1,
* and LoadWindowState's "previous display not found" branch re-centred the frame on every
* reopen instead of restoring the saved position. The wasm display factory now reports
* display 0 for any created window.
*
* ISOLATED in its own spec file (own Playwright worker → own browser process) like the
* deadlock spec: one heavy pcbnew load per process keeps the emscripten Worker pool and the
* shared GPU-process context budget predictable on CI.
*/
// Sample the NEWEST glcanvas backing store: distinct colours on a 16x16 grid. One
// drawImage readback per call (CPU-backed 2D canvas) — same technique as
// waitForThreeDRender, kept SwiftShader-safe (no per-pixel GPU round-trips).
function sampleNewestGlCanvasColors(page: import('@playwright/test').Page): Promise<number> {
return page.evaluate(() => {
const list = document.querySelectorAll('canvas[id^="glcanvas-"]');
const el = list[list.length - 1] as HTMLCanvasElement | undefined;
if (!el || !el.width || !el.height) return 0;
const tmp = document.createElement('canvas');
tmp.width = el.width; tmp.height = el.height;
const ctx = tmp.getContext('2d', { willReadFrequently: true })!;
ctx.drawImage(el, 0, 0);
const img = ctx.getImageData(0, 0, el.width, el.height).data;
const colors = new Set<string>();
for (let i = 0; i < 16; i++) {
for (let j = 0; j < 16; j++) {
const p = (Math.floor(el.height * j / 16) * el.width
+ Math.floor(el.width * i / 16)) * 4;
colors.add(`${img[p]},${img[p + 1]},${img[p + 2]}`);
}
}
return colors.size;
});
}
// The viewer's top-level window div position, from its inline style (the wasm DOM port
// positions windows via style.left/top).
function windowPos(page: import('@playwright/test').Page, winId: string) {
return page.evaluate((id) => {
const el = document.getElementById(id) as HTMLElement | null;
if (!el) return null;
return {
left: parseInt(el.style.left || '0', 10) || 0,
top: parseInt(el.style.top || '0', 10) || 0,
};
}, winId);
}
// Newest window-N div beyond a recorded set (same detection as 3d-viewer.spec.ts).
async function newestWindowId(page: import('@playwright/test').Page,
before: string[]): Promise<string> {
await expect.poll(async () => page.evaluate((prev: string[]) => {
const all = Array.from(document.querySelectorAll('#window-container [id^="window-"]'))
.map((e) => e.id);
return all.find((id) => !prev.includes(id)) ?? null;
}, before), { timeout: 60000, intervals: [300] }).not.toBeNull();
const winId = await page.evaluate((prev: string[]) => {
const all = Array.from(document.querySelectorAll('#window-container [id^="window-"]'))
.map((e) => e.id);
return all.find((id) => !prev.includes(id)) ?? all[all.length - 1] ?? null;
}, before);
expect(winId, 'the 3D viewer should open a new top-level window').toBeTruthy();
return winId as string;
}
test.describe('3D viewer close and reopen', () => {
// Two full viewer opens over one heavy pcbnew load: serial + generous.
test.describe.configure({ mode: 'serial' });
test.setTimeout(480000);
test('re-renders the board and keeps its window position after close + reopen',
async ({ page, testLogger }) => {
await page.goto('/kicad/pcbnew.html');
await waitForPcbnew(page);
await loadBoard(page, testLogger);
// ── First open: must render (precondition, same gate as 3d-viewer.spec.ts). ──
const winsBefore = await page.evaluate(() =>
Array.from(document.querySelectorAll('#window-container [id^="window-"]'))
.map((e) => e.id));
const glBefore = await countGlCanvases(page);
await openThreeDViewer(page, glBefore);
const winId = await newestWindowId(page, winsBefore);
await waitForThreeDRender(page);
await logThreeDDiag(page, 'first open rendered');
// Drag the viewer by its DOM title bar to a distinctive position: with pristine
// settings the frame opens display-sized at (0,0), where "restored" and
// "re-centred" coincide and a position regression is invisible. The drag makes
// the two outcomes distinguishable. (Same machinery as the titlebar spec.)
const posInitial = await windowPos(page, winId);
expect(posInitial, 'the 3D viewer window should have a position').not.toBeNull();
const bar = page.locator(`#${winId} .window-titlebar`);
const barBox = await bar.boundingBox();
expect(barBox, 'the 3D viewer should have a DOM title bar to drag').not.toBeNull();
const cx = barBox!.x + barBox!.width / 2;
const cy = barBox!.y + barBox!.height / 2;
await page.mouse.move(cx, cy);
await page.mouse.down();
await page.mouse.move(cx + 40, cy + 90, { steps: 10 });
await page.mouse.up();
// DOM style updates first; poll it as the observable...
await expect.poll(async () => (await windowPos(page, winId))?.top,
{ timeout: 15000, intervals: [200] }).not.toBe(posInitial!.top);
// ...then let the wx-side op (wx_window_move → wxWindow::Move) fully land before
// close saves the frame position from the wx side (documented interaction dwell —
// the DOM moves before the wx op completes; see 3d-viewer.spec.ts titlebar test).
await page.waitForTimeout(500); // eslint-disable-line -- documented interaction dwell
const posBefore = await windowPos(page, winId);
expect(posBefore, 'the 3D viewer window should have a position').not.toBeNull();
await page.screenshot({ path: shotPath(page, `3d-reopen-01-first-${DEMO.name}.png`),
scale: 'css' });
// ── Close via the titlebar ×; wait for the frame AND its GL canvas to go away
// (the canvas removal is the observable for wxGLCanvas/WebGL-context teardown). ──
await page.locator(`#${winId} .window-titlebar-close`).click();
await expect.poll(async () => page.evaluate((wid) => {
const el = document.getElementById(wid);
return !el || getComputedStyle(el).display === 'none';
}, winId), { timeout: 30000, intervals: [200] }).toBe(true);
await expect.poll(() => countGlCanvases(page), { timeout: 30000, intervals: [200] })
.toBe(glBefore);
await logThreeDDiag(page, 'viewer closed');
// ── Reopen. ──
const winsBeforeReopen = await page.evaluate(() =>
Array.from(document.querySelectorAll('#window-container [id^="window-"]'))
.map((e) => e.id));
await openThreeDViewer(page, glBefore);
const winId2 = await newestWindowId(page, winsBeforeReopen);
// Position must be restored, not re-centred. Read once the frame exists (position is
// applied during frame construction, which precedes the GL canvas the open-wait saw).
// Soft: a position regression must not mask the render assertion below (and vice
// versa) — both defects come from one user action and should report together.
const posAfter = await windowPos(page, winId2);
expect.soft(posAfter, 'the reopened 3D viewer window should have a position')
.not.toBeNull();
if (posBefore && posAfter) {
expect.soft(Math.abs(posAfter.left - posBefore.left),
`reopened 3D viewer should keep its window position (closed at `
+ `${posBefore.left},${posBefore.top}; reopened at ${posAfter.left},${posAfter.top}`
+ ` — re-centring means the saved display index was invalid)`)
.toBeLessThanOrEqual(2);
expect.soft(Math.abs(posAfter.top - posBefore.top),
`reopened 3D viewer should keep its window position (closed at `
+ `${posBefore.left},${posBefore.top}; reopened at ${posAfter.left},${posAfter.top}`
+ ` — re-centring means the saved display index was invalid)`)
.toBeLessThanOrEqual(2);
}
// THE render regression assertion: the reopened viewer must show the board again.
// A blank canvas (stale gl1 GL handles in the new WebGL context) stays at ~1 colour.
await expect.poll(() => sampleNewestGlCanvasColors(page), {
message: 'reopened 3D viewer canvas should render the board again '
+ '(a blank/uniform canvas means the gl1 shim is drawing with GL object '
+ 'names from the destroyed WebGL context)',
timeout: 90000,
intervals: [1000],
}).toBeGreaterThan(8);
await page.screenshot({ path: shotPath(page, `3d-reopen-02-reopened-${DEMO.name}.png`),
scale: 'css' });
// ── Console-clean gates (same signatures as 3d-viewer.spec.ts). ──
const allLines = [...testLogger.consoleLogs, ...testLogger.errors];
const aborts = allLines.filter((l) => l.includes('Aborted('));
expect(aborts, `WASM aborted during close/reopen:\n${aborts.join('\n\n')}`).toEqual([]);
const wasmTrapSignatures = [
'index out of bounds', 'indirect call to null', 'uncaught exception: unwind',
'invalid state', 'is not a function',
];
const wasmTrapErrors = allLines.filter((l) =>
wasmTrapSignatures.some((sig) => l.toLowerCase().includes(sig)));
expect(wasmTrapErrors,
`wasm trap surfaced during close/reopen:\n${wasmTrapErrors.join('\n\n')}`)
.toEqual([]);
});
});