From 96abe5d4a5c3a38cde7aebd3951b5ce363658409 Mon Sep 17 00:00:00 2001 From: Istvan Matejcsok <119620946+matejcsok-ee@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:01:32 +0200 Subject: [PATCH] fix(3d): return the substituted .step path from the model memo, not the ref's .wrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensureModelInMemfs memoized materialized refs in a Set and, on a cache hit, returned `${MODELS_3D_ROOT}/${ref}` — the ref's OWN path. But the wrl→step fallback writes the body under the .step filename and returns that. So the first ensure (prescan) wrote M.step and returned it, while the second ensure (the C++ viewer's PCBJAM_3D::EnsureModelFile lazy fallback) hit the early-return and got back M.wrl — a file never written. KiCad then stat'd the missing .wrl (S3D_CACHE::checkCache → GetModificationTime), so every component model on a KiCad-6-vintage (all-.wrl) board failed with "Failed to retrieve file times for '…​.wrl' (error 44)" and nothing rendered. Fix: memoize ref → the ABSOLUTE path actually written (Set → Map) and return that; doEnsure reuses an on-disk body via analyzePath and records the real target. Regression test: a second ensure of a .wrl ref returns the .step path, not the .wrl. JS-only, no pcbnew rebuild. Only bit boards that reference .wrl (the CDN is STEP-only from 10.x) AND double-ensure via prescan + lazy fallback — which is why .step-ref boards worked. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/wasm/libs/models-bridge.test.ts | 14 +++++++ web/standalone/src/wasm/libs/models-bridge.ts | 38 +++++++++++++------ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/web/standalone/src/wasm/libs/models-bridge.test.ts b/web/standalone/src/wasm/libs/models-bridge.test.ts index 03eca35..fdd2251 100644 --- a/web/standalone/src/wasm/libs/models-bridge.test.ts +++ b/web/standalone/src/wasm/libs/models-bridge.test.ts @@ -68,6 +68,20 @@ describe("ensureModelInMemfs format fallback", () => { expect(files.has("/pcbjam/3dmodels/FallbackLibA.3dshapes/M1.wrl")).toBe(false); }); + it("returns the substituted .step path on the memoized second ensure", async () => { + // Regression: the first ensure (e.g. the prescan) writes the .step body and + // memoizes it; a second ensure for the SAME .wrl ref (the C++ viewer's lazy + // fallback) must hand back the .step path that exists on disk — NOT the + // ref's own .wrl path, which was never written. Returning the .wrl path + // pointed KiCad at a missing file → "Failed to retrieve file times '…​.wrl'". + const files = installFakes((r) => r.endsWith(".step")); + const first = await ensureModelInMemfs("FallbackLibD.3dshapes/M4.wrl"); + const second = await ensureModelInMemfs("FallbackLibD.3dshapes/M4.wrl"); + expect(first).toBe("/pcbjam/3dmodels/FallbackLibD.3dshapes/M4.step"); + expect(second).toBe(first); + expect(files.has("/pcbjam/3dmodels/FallbackLibD.3dshapes/M4.wrl")).toBe(false); + }); + it("prefers the exact ref when it exists", async () => { const files = installFakes(() => true); const dest = await ensureModelInMemfs("FallbackLibB.3dshapes/M2.wrl"); diff --git a/web/standalone/src/wasm/libs/models-bridge.ts b/web/standalone/src/wasm/libs/models-bridge.ts index 7230a2b..81deb7e 100644 --- a/web/standalone/src/wasm/libs/models-bridge.ts +++ b/web/standalone/src/wasm/libs/models-bridge.ts @@ -75,8 +75,16 @@ function toolFS(): ModelFS | null { let installedSource: Model3dSource | null = null; let installedLog: (msg: string) => void = () => {}; -/** Refs already materialized in MEMFS this session (bodies are immutable). */ -const written = new Set(); +/** + * Refs materialized in MEMFS this session → the ABSOLUTE path actually written. + * The written path can differ in extension from the ref: a `.wrl` ref with no + * `.wrl` body is served by the `.step` fallback and written under `.step` (see + * refCandidates). We must memoize — and return — the REAL path. Returning the + * ref's own `.wrl` path (a value the ref implies but was never written) points + * KiCad at a missing file, surfacing as "Failed to retrieve file times for + * '…​.wrl'". Bodies are immutable, so the mapping never goes stale. + */ +const materialized = new Map(); /** In-flight ensures, coalesced per ref (prescan and the C++ fallback race). */ const ensuring = new Map>(); @@ -92,11 +100,11 @@ export function installModel3dHandler( /** Fetch one model body and write it under MODELS_3D_ROOT. Resolves to the * absolute MEMFS path when present, null when the source can't serve it. */ export async function ensureModelInMemfs(ref: string): Promise { - const dest = `${MODELS_3D_ROOT}/${ref}`; - if (written.has(ref)) return dest; + const cached = materialized.get(ref); + if (cached !== undefined) return cached; let p = ensuring.get(ref); if (!p) { - p = doEnsure(ref, dest).finally(() => ensuring.delete(ref)); + p = doEnsure(ref).finally(() => ensuring.delete(ref)); ensuring.set(ref, p); } return p; @@ -125,21 +133,27 @@ function refCandidates(ref: string): string[] { return [ref, ...(FALLBACK_EXTS[ext] ?? []).map((e) => `${stem}${e}`)]; } -async function doEnsure(ref: string, dest: string): Promise { +async function doEnsure(ref: string): Promise { const source = installedSource; const fs = toolFS(); if (!source || !fs) return null; - if (fs.analyzePath(dest).exists) { - written.add(ref); - return dest; - } + + // Try the exact ref, then the format-fallback candidates. The ACTUAL written + // path (which may differ in extension from the ref — a `.wrl` served by a + // `.step` body) is memoized and returned, so a later ensure for the same ref + // hands KiCad the file that exists, not the ref's own (never-written) path. for (const candidate of refCandidates(ref)) { + const target = `${MODELS_3D_ROOT}/${candidate}`; + // Already on disk (a prior ensure — this ref or another — wrote this body). + if (fs.analyzePath(target).exists) { + materialized.set(ref, target); + return target; + } const body = await source.getModelBody(candidate); if (!body) continue; - const target = `${MODELS_3D_ROOT}/${candidate}`; fs.mkdirTree(target.slice(0, target.lastIndexOf("/"))); fs.writeFile(target, body); - written.add(ref); + materialized.set(ref, target); installedLog( `[3d] materialized ${candidate}${candidate === ref ? "" : ` (for ${ref})`} (${body.length} bytes)`, );