fix(3d): return the substituted .step path from the model memo, not the ref's .wrl

ensureModelInMemfs memoized materialized refs in a Set<string> 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<string> →
Map<string,string>) 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) <noreply@anthropic.com>
This commit is contained in:
Istvan Matejcsok 2026-07-02 18:01:32 +02:00
commit 96abe5d4a5
2 changed files with 40 additions and 12 deletions

View file

@ -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");

View file

@ -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<string>();
/**
* 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<string, string>();
/** In-flight ensures, coalesced per ref (prescan and the C++ fallback race). */
const ensuring = new Map<string, Promise<string | null>>();
@ -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<string | null> {
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<string | null> {
async function doEnsure(ref: string): Promise<string | null> {
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)`,
);