fix(FragmentsModels): don't crash when a shell needs more render buffers than predicted

[iamahsanmehmood]

Closes #307

Summary

A shell (one densely-triangulated mesh) whose vertex/index data exceeds limitOf2Bytes (65,536, a uint16-range unit) gets split across multiple internal TileData buffers. Two independent passes decide how many buffers a shell needs:

• ShellTemplateConstructor.manageMemory - a sizing pass, run ahead of time to pre-allocate the TileData[] array
• ShellConstructor.manageMemory - the real construction pass, run later against the actual geometry

Nothing enforces that these two formulas always agree. If the construction pass ever needs one more buffer than the sizing pass predicted, setTileData read past the end of the pre-built array (bufferGeometries[this._indices] on an array too short), producing undefined, and the very next call (initializeIndices's this._tileData.indexCount!) threw:

TypeError: Cannot read properties of undefined (reading 'indexCount')
    at ShellConstructor.initializeIndices
    at ShellConstructor.nextBuffer
    at ShellConstructor.manageMemory
    at ShellConstructor.constructShell
    at ShellConstructor.construct

on every render frame - a model that would never finish loading, with the .frag file itself perfectly valid (metadata-only reads succeed fine; this is purely a render-geometry-construction bug). Originally surfaced by a production model (via a downstream exporter change that finally produced a shell large enough to exercise this code path for the first time).

Fix

ShellConstructor now creates a buffer on demand (createOverflowTileData, sized at the same limitOf2Bytes cap the whole scheme targets) instead of reading past the array, and trims it down to the real final index/position/normal counts once its construction completes (finalizeCurrentIfDynamic). This matters beyond just avoiding the crash: downstream code (VirtualTilesController.setupTileSampleAttributes) treats indexCount/positionCount/normalCount as authoritative exact counts when copying a shell's data into the merged render tile, so leaving the safe-upper-bound allocation as the reported size would silently corrupt that copy.

Honesty note on root cause: extensive empirical testing (large synthetic grid meshes, both ShellType.NONE and ShellType.BIG) found no real-world case where the two passes' formulas genuinely disagree at production scale, so the exact organic trigger for the originally-reported crash remains unconfirmed. This fix is a direct, always-safe guard against the failure mode regardless of what causes it, not a fix to a pinned-down formula bug. Tests verify the defensive mechanism itself by artificially truncating a correctly-predicted buffer array before construction - the same failure shape a genuine divergence would produce - including a multi-buffer-drop case that caught a real off-by-one in an earlier version of this fix (this._indices's meaning shifts by one across call sites once nextBuffer's own trailing incrementer has run; fixed by tracking _currentTileIndex explicitly instead).

Test plan

• New shell-constructor.test.ts: 3/3 passing - crash reproduction via simulated under-prediction (single and multi-buffer), plus a no-regression check that untouched (correctly Pass-1-sized) buffers are never modified
• npx tsc --noEmit: clean
• npx eslint: clean
• Rebased onto current main; confirmed no conflicts and tests still pass

[agviegas]

Thanks for the report and the test. The crash is real and your test does fail on current main with exactly the reported error, so the repro is solid.

Before we take it, though: this guards the symptom rather than closing the gap. The two sizing passes still disagree, and when the second pass needs an extra buffer it means the first one under-filled one of its own and left it with a too-large indexCount, which the tile controller then copies verbatim. That turns a loud crash into geometry that is quietly wrong, which for a viewer is the worse failure. Could you dig until the divergence actually reproduces, so the fix can make both passes agree?

One practical note too: the 200x200 fixture takes around 12 seconds on its own and times out under parallel load, and it starves an unrelated tiles-controller test into a timeout as well. It needs to shrink or move out of the default run.

[iamahsanmehmood]

Pushed 8eca082 addressing both points.

Test timeout

Both slow tests needed the same 200x200 fixture (one needs ≥2 predicted buffers, the other ≥4) but each built it independently - that duplication was the actual cost, not the buffer-count requirement itself. Reaching ≥4 buffers needs roughly 32,768+ triangles no matter how they're arranged (buffer limit is 65,536 points, ~3 points/triangle via ShellFace3's fast path), so 200x200 is already close to the minimum for that test's own guarantee - shrinking the grid further isn't really available without weakening what's being tested.

Built the shell and ran the sizing pass once in beforeAll, with each test making its own local copy before truncating/constructing (no cross-test mutation). Total file time: ~2.8s now (was ~12s for the slow test alone). Also merged current main in (the vitest 4→5 + node-environment-default bump landed since I last pushed) and re-verified against that - still 3/3 passing, tsc --noEmit and eslint clean on this file.

Digging into the actual divergence

Went through every vertex-accounting code path that contributes to either pass's buffer-overflow check, line by line, rather than relying on empirical testing again:

• ShellFace3/ShellFace4 (the fast path for hole-less triangles/quads, what the existing grid-mesh tests exercise): contributes exactly indexAmount points to _sizes.verticesAmount, matching ShellTemplateConstructor's own profilePoints += indexCount 1:1.
• ShellFaceX/earcut (the path for real N-gons, or any profile with a hole regardless of side count): processBuffers adds exactly profile.indicesLength() points from the outer boundary, setHolesBuffers adds exactly hole.points.length per hole - earcut's own callback only emits index triples referencing already-placed points, it never creates new vertices. Same 1:1 match with the sizing pass's profilePoints/holePoints accumulation.
• Multiple holes on one profile: ShellTemplateConstructor.processShellHoles loops all of a shell's holes per profile, filtering by profileId(). On the construction side, saveInteriorProfile/saveExistingInteriorProfile correctly appends (not overwrites) when a profile already has an entry in interiorProfiles - ruled out a "second hole silently dropped" theory I was chasing.

I couldn't find an arithmetic mismatch in any of these - which, combined with the original PR's own empirical testing across large synthetic grids (ShellType.NONE and ShellType.BIG), narrows things down but doesn't close the loop. What I haven't been able to test here: real production geometry with the specific shape the original crash report came from (the PR description mentions "a downstream exporter change" - openskp's own fragments export is named in this file's test comment as the shape these grid tests intentionally mirror). If you have the actual model or exporter diff that first triggered this, that's probably the fastest way to pin down whatever this is - a static read of the two formulas keeps checking out.

[agviegas]

Thanks for the thorough pass, and for getting the test file from 12 seconds down to under 3. A static read of every accounting path coming back clean is useful in itself: it tells us the divergence is not in the arithmetic for the shapes our importer produces, which points at the input.

Since you opened #307 from a real crash and the geometry mirrors OpenSKP's export, could you send us the .frag that crashed, or the source model plus the exporter version that produced it? Please send it to antonio@thatopen.com. With the real file we can reproduce the divergence and make both passes agree, which is the fix we would like to land rather than the guard alone.

[iamahsanmehmood]

To clarify where this came from: this surfaced through real production use, not a synthetic benchmark. We build FrameSmart on top of OpenSKP, and once we integrated this rendering path, a number of real client files were failing to load with exactly this crash - large, densely-triangulated meshes hitting the divergence between the two buffer-count passes. We root-caused it to the mismatch described in #307, fixed it as in #306, and confirmed in production that those same files - and large files generally - now load and render correctly.

I can't share the actual files - they're private client data - which is why #307 doesn't have one attached. I only had a synthetic, constructed reproduction ready to publish (simulating the under-predicted buffer array directly) rather than a specific minimal mesh that trips it organically, and didn't isolate one before opening the issue.

If it's useful for landing a fix at the root rather than the guard, I'm glad to try building a representative OpenSKP-exported model at a similar scale/topology to what we saw fail, since I can't hand over the real ones. Let me know if you have a sense of what shape is most likely to expose the divergence (single dense shell vs. many smaller ones, a particular triangle/vertex ratio) - that would narrow the search a lot.