Skip to content

Commit 4a606d6

Browse files
authored
fix(export,drawing-2d): stop writing NaN/Infinity into GLB, COLLADA, KMZ and SVG (#3329)
* fix(export,drawing-2d): stop writing NaN/Infinity into GLB, COLLADA, KMZ and SVG Neither format has a lexical form for a non-finite number, and four writers wrote one anyway. Observed on main, per value, by running each writer: export_glb_from_meshes +/-Inf position -> "min":[null,-0.5,0.0] and "translation":[null,0.5,0.0] (serde_json renders a non-finite float as null); NaN position reached the BIN chunk while min/max stayed finite, because NaN < min and NaN > max are both false. export_collada_from_meshes a triangle whose first X was -Infinity came out as `NaN 0 0 inf 0 0 inf 0 1` -- the AABB re-centring spread one bad vertex over every good one, and `inf` is not even an xs:float lexical form. export_kmz_collada_from_meshes same, through COLLADA. exportToSVG x1="NaN" / x1="-Infinity" at thirteen unguarded .toFixed(3) interpolations, and one non-finite corner of the bounds moved every finite line in the drawing via boundsCenter -> offsetX. The three exporters that take the viewer's flattened MeshData now pass all four float arrays (positions, normals, colours, per-mesh origins) through one gate, mesh_input::scrub_nonfinite, before either per-mesh loop reads any of them -- one emitter rather than a guard at each point a value becomes bytes. Non-finite becomes 0.0, matching usd::fmt::fmt_f32; alpha becomes 1.0, since scrubbing it to 0 would turn a colour defect into an invisible mesh. An all-finite input is passed through borrowed: no copy, byte-identical output. The SVG exporter gains the same shape: one svgNum(), mirroring the DXF writer's fmt() in the same package, which had guarded these values all along -- two writers of the same drawing disagreeing about the same input was the defect. computeTransform sanitises the bounds before deriving anything from them, so a degenerate corner no longer relocates the rest of the drawing. collada.rs's float formatting moves to collada_fmt.rs to stay under its module-size budget; the emitted text is unchanged. * refactor(drawing-2d): split the paper transform out of svg-exporter.ts The `svgNum()` guard and the `computeTransform` bounds sanitising added by this PR took `svg-exporter.ts` to 652 lines against a budget of 613, failing the module-size ratchet. Split rather than raise. `computeTransform`, `scaleLabel` and `transformPoint` move to a new `svg-transform.ts` alongside `Transform2D` and `finiteOr0`. Fitting a drawing's world bounds onto a sheet — and reporting the scale actually used — is one cohesive job, separable from turning the fitted geometry into markup. None of the three ever read `this`, so they become plain functions and the call sites drop the `this.` prefix. `createTitleBlock`'s `scaleLabel` parameter is renamed `scaleText`: that name now belongs to the imported function, and shadowing it there would read as a call site. A pure move, proven rather than asserted: the moved bodies `diff` clean against the originals modulo the dedent and the `private` -> `export function` keyword swap, and the `Transform2D` body is byte-identical. Full `packages/drawing-2d` suite green (518 tests, 39 files), as is `packages/export`. The `svg-exporter.ts` row ratchets DOWN 613 -> 537 to the new measured count, with `packages/drawing-2d`'s ALLOWLIST_DIGESTS entry re-pinned in the same commit. Edited by hand rather than with `--update`, which would also have ratcheted the unrelated headroom rows the gate notes for `packages/export/src/schema-converter.ts` and `packages/server-client/src/parquet-tables.ts` — those belong to other PRs. No row moved up.
1 parent e8c0d71 commit 4a606d6

12 files changed

Lines changed: 849 additions & 166 deletions
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
"@ifc-lite/wasm": patch
3+
"@ifc-lite/drawing-2d": patch
4+
---
5+
6+
Stop writing `NaN` / `Infinity` / `-Infinity` into exported GLB, COLLADA, KMZ and SVG files.
7+
8+
Neither format can carry a non-finite number, and every one of these paths wrote one anyway.
9+
10+
**GLB / COLLADA / KMZ** (`export_glb_from_meshes`, `export_collada_from_meshes`, `export_kmz_collada_from_meshes` — the viewer's "export" buttons, via `GeometryProcessor.exportGlbFromMeshes` / `exportKmzFromMeshes`). Nothing between a mesh buffer and the bytes established that a coordinate was finite, and the three values did not fail alike:
11+
12+
- An infinite position made `serde_json` write `null` where glTF requires a number — `"min":[null,-0.5,0.0]`, `"translation":[null,0.5,0.0]` — which is schema-invalid, so the whole GLB is rejected rather than merely wrong.
13+
- A `NaN` position reached the BIN chunk while `min`/`max` stayed finite, because `NaN < min` and `NaN > max` are both false. The accessor's bounding box described a buffer it did not contain.
14+
- COLLADA re-centres on the mesh AABB, so **one** non-finite vertex turned **every other vertex in the document** into `inf`/`NaN`. Observed: a triangle whose first X was `-Infinity` came out as `NaN 0 0 inf 0 0 inf 0 1` — one bad vertex, no surviving geometry. `<float_array>` is `xs:float`, whose non-finite lexical forms are `INF`/`-INF`/`NaN`; Rust's `Display` writes `inf`/`-inf`, which are not even those.
15+
- A non-finite colour component became `"baseColorFactor":[null,0.5,0.5,null]`.
16+
17+
All four float arrays (positions, normals, colours, per-mesh origins) now pass through one gate, `mesh_input::scrub_nonfinite`, before either exporter's per-mesh loop reads any of them — rather than at each of the several points where a value becomes bytes, where a guard reaches three call sites out of four. A non-finite component is replaced with `0.0`, matching what the USD writer already did; alpha is the exception and becomes `1.0`, since scrubbing it to `0` would turn a colour defect into an invisible mesh. An all-finite input — the only case a well-formed model produces — is passed through borrowed, with no copy and byte-identical output.
18+
19+
**SVG** (`exportToSVG`). SVG's `<number>` grammar admits a sign, digits, a point and an exponent and nothing else, so `x1="NaN"` is an error a conforming renderer must not draw. Thirteen coordinate, size and rotation interpolations went through a bare `.toFixed(3)`, which stringifies all three values verbatim; the DXF writer beside it in the same package has guarded exactly these at its single `fmt()` since it was written, so the two writers of the same drawing disagreed about the same input. Every SVG number now goes through one `svgNum()`. Separately, `computeTransform` derived the paper offsets from `boundsCenter`/`boundsSize`, which are plain min/max arithmetic: one non-finite corner of the bounding box moved every finite line in the drawing (a line that belonged at `x1="190.000"` was written as `x1="NaN"`). The bounds are sanitised before anything is derived from them, so a degenerate corner no longer relocates the rest of the drawing.

packages/drawing-2d/src/svg-exporter.test.ts

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,3 +365,91 @@ describe('SVGExporter impossible padding', () => {
365365
}
366366
});
367367
});
368+
369+
// ═══════════════════════════════════════════════════════════════════════════
370+
// Non-finite coordinates must never reach the SVG text.
371+
//
372+
// `NaN`, `Infinity` and `-Infinity` are not SVG `<number>` tokens — the SVG
373+
// grammar admits only an optional sign, digits, a decimal point and an
374+
// exponent — so an attribute reading `x1="NaN"` is in error and a conforming
375+
// renderer must not draw the element. Every coordinate here went through a
376+
// bare `.toFixed(3)`, which stringifies all three verbatim, while the DXF
377+
// writer sitting beside it in this package has guarded the same values at its
378+
// single `fmt()` since it was written. Two writers of the same drawing that
379+
// disagree about the same input is the defect; the fix is one shared emitter,
380+
// not a guard sprinkled over thirteen interpolations.
381+
//
382+
// The three values are asserted separately: `Infinity` reaches the text as
383+
// `"Infinity"` while `NaN` reaches it as `"NaN"`, and a guard keyed on only
384+
// one of them (`isNaN`, say) would let the other two through.
385+
// ═══════════════════════════════════════════════════════════════════════════
386+
387+
describe('SVG exporter: non-finite coordinates', () => {
388+
const NONFINITE: Array<[string, number]> = [
389+
['NaN', NaN],
390+
['Infinity', Infinity],
391+
['-Infinity', -Infinity],
392+
];
393+
394+
it.each(NONFINITE)('writes no %s token anywhere in the document', (label, poison) => {
395+
const drawing = drawingWithLine(
396+
{ min: { x: 0, y: 0 }, max: { x: 4, y: 6 } },
397+
{ x: poison, y: 0 },
398+
{ x: 2, y: 3 }
399+
);
400+
const svg = exportToSVG(drawing, {
401+
paperSize: PAPER_SIZES.A3_LANDSCAPE,
402+
scale: scaleByFactor(100),
403+
padding: 20,
404+
});
405+
// Anti-vacuity: the drawing line really is in the document.
406+
expect(svg).toContain('data-entity-id="1"');
407+
expect(svg, `[${label}] emitted NaN`).not.toContain('NaN');
408+
expect(svg, `[${label}] emitted Infinity`).not.toContain('Infinity');
409+
});
410+
411+
it.each(NONFINITE)(
412+
'a non-finite bound (%s) does not poison the other coordinates',
413+
(label, poison) => {
414+
const drawing = drawingWithLine(
415+
{ min: { x: poison, y: 0 }, max: { x: 4, y: 6 } },
416+
{ x: 0, y: 0 },
417+
{ x: 2, y: 3 }
418+
);
419+
const svg = exportToSVG(drawing, {
420+
paperSize: PAPER_SIZES.A3_LANDSCAPE,
421+
scale: scaleByFactor(100),
422+
padding: 20,
423+
});
424+
// Not merely finite: the finite line must land EXACTLY where it does when
425+
// the bounds are clean (the 190/178.5/210/148.5 of the characterisation
426+
// test above). A guard that emitted "0" for everything would pass a
427+
// finiteness-only assertion while still having lost the drawing.
428+
const { x1, y1, x2, y2 } = extractLineCoords(svg);
429+
expect(x1, `[${label}] x1`).toBeCloseTo(190, 3);
430+
expect(y1, `[${label}] y1`).toBeCloseTo(178.5, 3);
431+
expect(x2, `[${label}] x2`).toBeCloseTo(210, 3);
432+
expect(y2, `[${label}] y2`).toBeCloseTo(148.5, 3);
433+
}
434+
);
435+
436+
it('still writes exact coordinates for an all-finite drawing', () => {
437+
// Both directions: a guard that emitted "0" for everything would satisfy
438+
// every assertion above. Same fixture as the characterisation test at the
439+
// top of the file, asserted to the same precision.
440+
const drawing = drawingWithLine(
441+
{ min: { x: 0, y: 0 }, max: { x: 4, y: 6 } },
442+
{ x: 0, y: 0 },
443+
{ x: 2, y: 3 }
444+
);
445+
const svg = exportToSVG(drawing, {
446+
paperSize: PAPER_SIZES.A3_LANDSCAPE,
447+
scale: scaleByFactor(100),
448+
padding: 20,
449+
});
450+
expect(svg).toContain('x1="190.000"');
451+
expect(svg).toContain('y1="178.500"');
452+
expect(svg).toContain('x2="210.000"');
453+
expect(svg).toContain('y2="148.500"');
454+
});
455+
});

0 commit comments

Comments
 (0)