feat(split): config - #268
Merged
Merged
Conversation
ShaMan123
commented
Aug 20, 2026
ShaMan123
left a comment
ShaMan123
left a comment
Contributor
Author
There was a problem hiding this comment.
deferred indexes and splits since they require more work and are less straight forward
Contributor
|
Good news first: we've decided to adopt #252, so feel free to continue the splitter refactor on top of it. About this PR, a few small things before merging: export the default ELEMENT_TYPES and SPATIAL_TYPES so consumers can extend them instead of only replacing them, fix the {@link} that points to a non-exported symbol, guard the undefined spread (it can poison the Required config type), and add a couple of tests for the config merge behavior. Does that work for you? |
Contributor
Author
ShaMan123
added a commit
to ShaMan123/engine_fragment
that referenced
this pull request
Sep 10, 2026
Review follow-up on ThatOpen#268: - export ELEMENT_TYPES, SPATIAL_TYPES and listIdxByType so consumers can extend the defaults instead of only replacing them - {@link listIdxByType} in IfcSplitterConfig now resolves - merge the config field by field, so an explicit `undefined` can no longer overwrite a default and poison Required<IfcSplitterConfig> - cover the merge and each option's effect on the output with tests Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up on ThatOpen#268: - export ELEMENT_TYPES, SPATIAL_TYPES and listIdxByType so consumers can extend the defaults instead of only replacing them - {@link listIdxByType} in IfcSplitterConfig now resolves - merge the config field by field, so an explicit `undefined` can no longer overwrite a default and poison Required<IfcSplitterConfig> - cover the merge and each option's effect on the output with tests Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ShaMan123
force-pushed
the
feat/splitter-config
branch
from
September 10, 2026 12:37
4e056f1 to
fdd0af4
Compare
Contributor
|
Yes, that's the long-term idea: #252 becomes the parsing layer and the splitter's ad-hoc parsing migrates onto it progressively, as you yourself suggested in #180. No need to do it all at once or in this PR; landing the config first and refactoring the internals afterwards works for us. Does that ordering sound good to you? |
Contributor
Author
|
Yes, makes sense. |
iamahsanmehmood
added a commit
to iamahsanmehmood/engine_fragment
that referenced
this pull request
Sep 16, 2026
…ers than predicted (#1) * perf(FragmentsModels): skip view refresh and re-cull when the view is unchanged Every update() dispatched REFRESH_VIEW to every model unconditionally, and worker-side setupView() always reset the cull pass — so each worker re-culled every sample of every model roughly every maxUpdateRate ms forever, even with a fully idle camera: the FINISH emitted by each pass scheduled the next update(), sustaining the loop with no user input. With one model this is barely visible; with N models on N workers it burns several cores at idle. Measured with 13 real IFC-derived models (headless Chromium): ~660 REFRESH_VIEW + ~700 tile batches per 15 s and ~2 cores continuously busy while nothing changed on screen. Changes: - ViewManager fingerprints the outgoing view (frustum planes, camera position in model space, clipping planes, viewport size, graphics quality, model placement) and skips the REFRESH_VIEW RPC when nothing changed since the last dispatch. Forced updates always dispatch because their FINISH acts as the completion fence for forceUpdateFinish. - Worker-side setupView() compares the incoming view against the current one and skips restart() when identical, so a forced update with an unchanged view (e.g. the fence issued after loading another model) no longer re-culls everything. If the previous pass already finished it emits a FINISH directly so fences still settle; an in-flight pass keeps running and its natural FINISH carries the new seq because the stamp is read at emission time. graphicThreshold is deliberately excluded from both comparisons — a budget change must not trigger a full re-cull, and the stored view still adopts the new value. - FragmentsModels.update() reschedules itself with a light timer so view-change detection stays alive without worker traffic (previously the loop was sustained by the FINISH-driven update event, which no longer fires when idle), and force now bypasses the maxUpdateRate throttle instead of being silently dropped. - FragmentsModel._isProcessing is only set when a refresh was actually dispatched, so isBusy cannot get stuck on skipped refreshes. A/B with 13 models, 15 s idle window: worker RPCs 1358 -> 0..31, CPU time of the browser process tree 31.8 s -> 11.0 s. Interaction cost is unchanged (a real view change still triggers the same full pass), and visibility/highlight/edit changes still reach the screen because those paths restart the worker pass themselves. Verified via functional tests: hide/highlight without camera movement, update(true) fence after an unrelated RPC (no hang), camera move and clipping-plane changes picked up. * perf(FragmentsModels): coalesce forced updates inside the rate window instead of bypassing it Letting update(true) skip maxUpdateRate turned every camera-controls "rest" event (the components layer forces an update on it) into a full re-cull of all models plus an unbounded tile drain on the main thread — during a programmatic orbit that is nearly every frame. Forced calls that land inside the window are now merged into a single trailing forced update; awaiting callers resolve when that one has settled, so the fence semantics are unchanged. * fix: don't cull geometry before a camera is set `ViewManager` only builds a real frustum once `useCamera()` has been called. Until then it shipped a default `THREE.Frustum`, whose six planes are all normal=(1,0,0) constant=0 — a shape that discards every item whose bounding box lies entirely at x < 0. On a model centred near the origin that is roughly half the geometry. Nothing throws and nothing warns: the items stay loaded and reachable through the data APIs, they just never reach the renderer. Note this does not disable culling. With no camera there is simply nothing meaningful to cull against, so `refreshView` builds a frustum from the model's own bounding box — one that contains all of it — and the worker culls against that exactly as it would any other frustum, discarding nothing. Encoding the intent in the frustum, rather than beside it Two earlier drafts signalled "no camera" out of band: omitting the frustum, then adding a `cameraApplied` flag. Both are cleaner in the abstract and both are wrong here, because the worker ships as a separate artifact that consumers pin or self-host. A main thread running ahead of its worker is a real pairing, and every worker published so far dereferences `view.cameraFrustum.planes` unconditionally: - omitting it throws `Cannot read properties of null (reading 'planes')` on every frame; - a flag avoids the crash but an old worker ignores it and keeps the bug, while new state has to stay in sync across the boundary. A containing frustum needs no agreement at all. It is structurally an ordinary frustum, so an old worker paired with this main thread does not merely avoid crashing — it renders the model correctly without knowing the fix exists. The wire format is unchanged and there is no version to negotiate. The extent comes from the model bounds rather than a large constant. A fixed extent bakes in a unit assumption: 1e9 is ample in metres but a geo-referenced model authored in millimetres reaches ~1e10, and fragments does not normalise geometry — the IFC length-unit factor is applied to storey-height properties only. A bounds-derived frustum cannot clip the models a "big enough" constant is meant to protect. Both paths emit the frustum in model space, which is where the worker culls: `VirtualBoxController.get()` returns raw flatbuffer coordinates with only the per-sample transform applied, and the `modelPlacement` carried on the view is written but never read. The real-camera path maps its world-space frustum through the inverse placement; the camera-less path maps the world-space `FragmentsModel.box` the same way before building from it. Worker-side changes are hardening, not mechanism `safeCopyFrustum`, `setupViewPlanes` and `getCurrentViewOrientation` all dereferenced the frustum unguarded and now tolerate its absence, so a main thread that sends nothing degrades instead of throwing every frame. The last is easy to miss: `setupView()` runs `updateOrientationIfNeeded()`, which reads `cameraFrustum.planes[4]`, *before* `setupViewPlanes()` — so guarding only the culling site is not enough. Tests Unit tests drive the real `ViewManager.refreshView`, `ThreadViewRefresher.execute` and `VirtualTilesController.setupView`, and replay an unpatched worker's own dereferences verbatim to prove that pairing both survives and culls nothing. Two pin assumptions that are otherwise invisible: a millimetre-scale model at ~1e10 survives, and fails if the extent is pinned to a constant; and a model placed 1e7 from the origin survives, which fails if the world-to-model transform is dropped. Both need a non-degenerate case to be observable at all — under an identity placement the two spaces coincide. End-to-end tests load `resources/frags/small_test.frag` (which straddles the YZ plane, x from -5.18 to 2.28) through a real `VirtualFragmentsModel`, run the update loop to completion and assert on the geometry actually handed to the renderer: - the default frustum drops 49,200 of 228,216 indices — 21.6% of the model, three whole tiles — while the containing frustum draws all of it; - culling still works with a real camera, the regression risk this shape introduces: a camera containing the model draws exactly what the camera-less path draws, and one facing away draws nothing. Reverting any of the five source hunks fails the suite. Closes ThatOpen#255 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: collectDeps no longer reports nonexistent ids * fix: skip delta rebuild on data-only edits Every editor.edit() call rebuilds the whole delta model, even when the requests cannot change the rendered picture (property set items without samples, relations, attribute updates, spatial structure). Flows that recreate elements with their psets (paste, library placement) issue several such edit rounds per element, and each rebuild produces a visible flash. Two changes: * edit() now classifies the request batch: if no request affects geometry, materials, transforms or deletes an element, the current delta visuals are kept and the delta reload is skipped entirely. The virtual model still applies the requests (ids, undo history and data queries stay correct), and the next render-affecting edit rebuilds the delta from the full request history as before. An empty request array (the undo/redo refresh path) always rebuilds. * When the delta IS rebuilt, the outgoing delta object is hidden synchronously right after the new one is added to the scene — dispose() is async, so a frame could otherwise render both deltas on top of each other (z-fighting flash). * fix: index items by category for getItemsOfCategories getItemsOfCategories tested every regex against every item's category on every call — O(items x regexes) per call, ~750 ms on a 43 MB model with ~1.5M items. The flatbuffer category data is immutable, so a lazily built category -> localIds Map (keyed by the underlying buffer identity) lets each call test regexes once per distinct category name instead. Measured on real IFC-derived fragments data (43 MB, 23 categories): repeated calls ~750-1100 ms -> 0.3-10 ms, first call equal or faster; results byte-identical (including key order and id order). Edit-request handling (created/updated/deleted items) is unchanged. * fix: index edit requests for faster property reads VirtualPropertiesController scanned the whole requests list once per item read (getItemAttributes, getItemRelations, getItemsCategories, the created items in getItemsByAttribute) and rebuilt the deletedItems Set from the full list per item in getItemData, so a bulk read of N items with E pending requests cost O(N x E). Add EditRequestIndex: requests grouped by the localId they target (in push order) plus a persistent deletedItems Set. VirtualFragmentsModel keeps it in step on edit/save (push), undo (pop) and redo (push), and rebuilds it when the requests array is replaced (reset, setRequests, selectRequest); the index also re-syncs itself if the tracked array is swapped or grows behind its back. The six per-item scans become O(k) lookups in the number of requests for that item. Per-call enumerations (getCategories, getItemsOfCategories, the itemIds-less branch of getItemsByAttribute) are unchanged. Tests cover the index bookkeeping and, on resources/frags/small_test.frag, equivalence with the previous full scans across create/update/relate/ delete, undo/redo/reset, restored histories and external mutation. * fix: fill tiles sorted by size A tile's index buffer is laid out in the order samples were appended and the per-sample LOD decision depends on screen size, so with file-ordered samples the geometry / wires / invisible cut through a tile produced dozens of interleaved visibility runs. Each run becomes a geometry.groups entry on the main thread, i.e. its own draw call. Feeding generate() the existing size-sorted _samplesDimensions order keeps the cut to one or two runs. Tile membership and geometry are unchanged. * fix: rotate thread updater across workers updateAllModels() always iterated the model list from the beginning and breaks after the 16 ms budget is spent. When several models share a worker and one of them is mid-way through a long cull pass, that model consumed the whole budget on every tick and the models after it in the list were starved until the pass completed. The tick now starts where the previous one left off, so every model gets a slot. A partial sweep also no longer reports the thread as fully updated — models the tick never reached may still have pending work, so backing off to the idle delay based on a partial sweep let pending work sit for updateDelay ms. * fix: hierarchical frustum culling via the box structure The per-view cull pass tested every sample's box against the frustum and clipping planes individually — a flat O(N) sweep per model on every real view change, even when most of the model was far outside the view (typical for interior navigation). The spatial hierarchy that already exists for raycasting (VirtualBoxStructure) now feeds the same pass: - VirtualBoxCollider gains an allocation-free mask traversal (frustumFillOutsideMask) sharing the exact walk and plane semantics of frustumCollide; collide() is refactored onto the shared traverse. - VirtualTilesController rebuilds a per-sample outside-mask on every real view change. Samples in branches that provably miss the view skip the per-sample plane math; every candidate still runs the exact original test, so the final classification of any sample is unchanged (verified: identical rendered triangle counts across fixed camera poses, within the pre-existing run-to-run noise of the LRU-based mesh cache; a same-build control run shows equal noise). - Fully-included branches collect their leaves without per-leaf box tests, so the whole-model-in-view case stays fast too. Measured with 13 real IFC-derived models (headless Chromium): interior look-around CPU -14%, settle after orbiting 44 ms -> 23 ms, idle and whole-building orbit unchanged. * fix: compact tile indices into one draw range per tile A shell tile whose samples are partly culled or at a different LOD was drawn as one geometry.group per visible run — each its own draw call. On a large model with many small items this is 10–40 draw calls per tile (measured 289 shell meshes → 4 739 draw calls on a 38 MB IFC). The tile index is now kept on the CPU and, when a tile has three or more visible runs, the visible ranges are copied into the GPU index buffer in place and drawn as a single group (drawRange bounds the count). Highlighted tiles and tiles with fewer runs keep the group path, restoring the full index first. The GL buffer is never reallocated, so nothing leaks across updates. * test(view-manager): stub _boxes/_outsideMask in tiles-controller mock PR ThatOpen#285's hierarchical culling added updateOutsideMask() to setupView, which reads this._boxes.lookup. PR ThatOpen#256's bare Object.create mock predates that and lacks the field, so the null-frustum test crashed. Production is unaffected; this only completes the mock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: cap the tile cache per worker VirtualTilesController tracks graphic memory in a static counter, which is per worker — but the graphicThreshold sent from the main thread was the full global GPU estimate. With the worker pool distributing N models over N workers, each worker independently allowed the full budget, so the cache of invisible tiles could grow to N x capacity and eviction effectively never happened. On top of that, the screen-size heuristic (width x height x dpr^2 x 200) explodes on hidpi displays: a 4K screen at devicePixelRatio 2 yields ~6.6 GB per worker. Changes: - The main thread divides the global estimate by the number of active workers (new FragmentsConnection.activeThreadCount) so the combined cache stays within one global budget regardless of model count. - GPU.estimateCapacity() is capped at 1 GB. The budget only bounds the cache of invisible tiles — visible geometry is never evicted — so the cost of a tighter cap is at most some re-uploads when the camera returns to a previously culled area. Measured with 13 real IFC-derived models after 10 s of orbiting (headless Chromium): tile meshes 3872 -> 1595, GPU geometries 1838 -> 876, main-thread JS heap 148 MB -> 47 MB, with no change in settle time after the camera stops (~65 ms both). * fix(parser): streaming STEP tokenizer with web-ifc tape parity * feat(): IfcParserStream * test * review * fix: reuse identical preserved material definitions * fix: settle update fences when no models remain * fix: include property names in rendered material identity * fix: setSample passes real visibility to updateTile VirtualTilesController.setSample() computes the correct `vis` parameter (the real value set by setVisible()/toggleVisible()) and stores it via this._samples.setVisible(id, vis), but then calls updateTile(tileId, id, high, high === 0) - passing high === 0 (true for almost any non-highlighted sample) as updateTile's `visible` argument instead of `vis`. updateTile()'s fourth parameter flows straight into updateTileData(tile, sample, visible, highlight), which writes tile.visibilities.update(id, visible) - the buffer that actually controls whether the tile's rendered geometry shows this sample. Since that buffer never receives the real visibility value, a setVisible()/ toggleVisible() call correctly updates the model's own state (confirmed via getVisible(), which reads a separate itemConfig-backed store) without the corresponding geometry ever being added to or removed from the render - even after repeated update(true) calls. One-line fix: pass `vis` instead of `high === 0`. * fix: survive grids without ObjectPlacement and report per-grid failures (ThatOpen#263) IFCGRID's ObjectPlacement is optional in the IFC schema, but FragmentsIfcUtils.getAbsolutePlacement read ObjectPlacement.value unguarded, and GridReader.read wrapped the whole grid loop in a single try/catch. One placement-less grid therefore threw and silently dropped EVERY grid in the file. - getAbsolutePlacement now falls back to the identity placement when ObjectPlacement is missing, still applying the IFC -> three.js basis change. Present placements behave exactly as before. This also guards the other caller (SpaceBoundaryReader). - GridReader.read catches per grid, warns with the failing grid's express id, and keeps reading the remaining grids. - A placement-less grid is kept (identity placement) and reported with a console.warn instead of being dropped silently. Regression tests convert crafted IFC fixtures (resources/ifc) through the IfcImporter in Node and inspect the produced flatbuffer: on the unpatched code the two-grid fixture yielded 0 ThatOpenGrid items. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: depth bias fields on MaterialDefinition to resolve coplanar z-fighting (ThatOpen#266) Exactly coplanar faces (e.g. an IFCRAILING whose base plane sits exactly in the top plane of its IFCSLAB) z-fight with no way to resolve them: MaterialDefinition exposed no depth bias, depthTest/depthWrite both make things worse, and mutating THREE materials directly is wiped on re-stream. This adds optional polygonOffsetFactor/polygonOffsetUnits to MaterialDefinition so applications can express "railings sit on floors, not in them" per definition, surviving re-streams. - model-types.ts: two optional fields, default 0 = exactly today's behavior (polygonOffset stays disabled). - material-manager.ts: the filled-primitive (SHELL) factory enables THREE polygonOffset when either field is non-zero and forwards factor/units. The LINE/LodMaterial branch is untouched: polygonOffset only affects filled primitives (GL POLYGON_OFFSET_FILL). - highlight-helper.ts: both fields added to the _highlightProps merge whitelist so a later highlight without bias inherits the previous highlight's bias on the legacy merge path. Also adds the previously missing depthTest/depthWrite there (pre-existing quirk found in review; the issue's use case is highlight-time bias). Runtime-only: no flatbuffer schema change. Bias is a view/app policy applied per definition at highlight time; the persistent struct Material carries neither depth flag today (same precedent as depthTest and depthWrite), and .frag compatibility is untouched. Material dedup and worker/main serialization need no changes: the cache key serializes definition entries generically (definitions without bias hash exactly as today), and definitions cross threads as structured- cloned plain objects. Tests: definitions differing only in bias must not alias; factory enables polygonOffset with correct factor/units when biased and keeps polygonOffset=false/0/0 without bias (default regression lock); the highlight merge path passes bias and depth flags through. The factory and merge tests fail on unpatched main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: awaitable setup and clean disposal for SingleThreadedFragmentsModel (ThatOpen#261, ThatOpen#262) The SingleThreadedFragmentsModel constructor fired setupData() and dropped the promise: callers could not await, observe, cancel or catch the setup (ThatOpen#261). It also created a MeshConnection whose 64 ms interval kept the Node process alive forever and whose callback dereferenced an undefined connection on the single-threaded path, and dispose() did not stop the in-flight setup chain (ThatOpen#262). Both fixes land together because aborting the setup on dispose() requires the stored setup promise plus an attached rejection handler -- otherwise the abort itself becomes a new unhandled rejection. - Store the constructor's setup promise and expose it as a public readonly `ready: Promise<void>`. It is memoized: awaiting it any number of times never re-runs tile generation (a public setupData() forwarder would have re-appended every sample into the tile buffers). A no-op .catch is attached internally so a setup failure or abort on an unawaited model never surfaces as an unhandled rejection; awaiting `ready` still rethrows the original error. - dispose() now sets an abort flag that the generation loop's existing throwIfAborted hook checks at every yield point, so the setup chain stops at its next tick with a LoadAbortedError instead of running to completion against torn-down controllers. dispose() is idempotent. - MeshConnection no longer starts its updater interval when constructed without a connection (nothing could ever consume the requests, and the timer kept the event loop alive), drops process() requests on the connection-less path instead of growing an unbounded list, and guards refresh() against a missing connection, mirroring the existing _onTransferMaterial guard. Node-only vitest coverage in single-threading/index.test.ts; all four tests fail on unpatched main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: add getItemCategory wrapper to the virtual model (ThatOpen#267) Item.getCategory() invokes "getItemCategory" on the virtual model through the worker, but VirtualFragmentsModel only exposed getItemsCategories, so ThreadExecutor's dispatch threw "TypeError: virtualModel[input.function] is not a function". Add the singular wrapper, delegating to getItemsCategories. Unknown ids are skipped by that method (empty result array), so the wrapper falls back to null for them, matching the rest of the item API. The regression test drives the real ThreadExecutor dispatch with a request built by the same helper FragmentsConnection.invoke() uses, so the exact failing expression is pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: surface unsupported grid axis curves instead of dropping them (ThatOpen#264) GridReader.getGridAxes only understands point-list axis curves (IFCPOLYLINE and IFCINDEXEDPOLYCURVE-style CoordList curves). Axes defined with IFCCIRCLE, IFCLINE or IFCTRIMMEDCURVE hit a silent 'continue' and vanished from the converted grid without a trace. Tessellating those curves is out of scope here; instead the importer now tells you what it dropped: - GridData gains an optional unsupportedAxes: { tag, curveType }[] field listing the skipped axes with their IFC curve type (from webIfc.GetNameFromTypeCode, uppercased to STEP spelling). Optional, so fragments serialized before this field existed are unaffected. - A console.warn at import lists the skipped axes per grid. - Axes that end up with an empty curve are never emitted: the grids label code slices the first/last points of each curve, so an empty one would produce NaN label positions. Regression test: a crafted radial-grid fixture (resources/ifc) converts to a grid that keeps its polyline axes and reports R1/L1/T1 as IFCCIRCLE/IFCLINE/IFCTRIMMEDCURVE. On the unpatched code the field was absent and no warning fired. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: note string-key hashing for index performance (ThatOpen#250) The ThatOpen#250 discussion settled that index getters should keep structured clone (typed arrays clone at memcpy speed; transferring FlatBuffer views would require a copy anyway and shared mutable bytes are a support burden). The agreed takeaway for consumers is recorded where the API lives: hash long string keys to uint32 at build time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: validate PRs with install, build and tests (ThatOpen#254) Adds the missing PR validation workflow. Note: intentionally not enabled for releases/tagging here; the release half of ThatOpen#254 is a process decision tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: stamp generator and version into model metadata (ThatOpen#273) Every fragments file now records which package and version created it, in the metadata JSON of the model: - New src/Utils/version.ts is the single source of truth: it exposes FRAGMENTS_GENERATOR ("@thatopen/fragments") and FRAGMENTS_VERSION, injected at build time from package.json via the vite `define` option (with a runtime "unknown" fallback when unbundled), plus getProvenanceMetadata() returning { generator, version }. - IfcPropertyProcessor.getMetadataOffset spreads the provenance keys into the metadata it serializes alongside schema/names/descriptions. - newModel() stamps the same provenance instead of writing "{}". - vite.config-worker.ts gains the same __FRAGMENTS_VERSION__ define so the worker bundle (which now includes version.ts) bakes the real version too. - FragmentsModels/index.ts reuses FRAGMENTS_VERSION for the unpkg worker URL instead of a local declare of __FRAGMENTS_VERSION__. Covered by src/Utils/version.test.ts: importer and newModel stamping (verified failing before the fix), and that pre-stamp .frag files still load with no provenance keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: keep web-ifc out of the worker bundle (ThatOpen#289) The Utils barrel re-exported ifc-parsing-utils, ifc-stream and ifc-splitter, and the worker imports the barrel, so the STEP parsing layer's web-ifc dependency (IfcAPI glue plus the FromRawLineData schema tables) was retained in the worker bundle, growing it from 3.3 MB to 7.4 MB. These three modules are now exported from the package root instead of the barrel: the public API is unchanged and the worker no longer reaches them. Verified: worker rebuild drops to 3.5 MB with zero web-ifc markers, and the library bundle still exposes the parsing API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(splitter): inline fixture conversion and test timeouts * fix(splitter): tests * revert frag file * increase timeout * test: pin setSample tile visibility against highlight mixup PR ThatOpen#288 fixed setSample() forwarding high === 0 as updateTile()'s visible argument instead of the real vis value - silently decoupling a sample's rendered tile visibility from setVisible()/toggleVisible()'s real intent, confirmed reproducible on a real 18,382-sample/12,338-item model but not yet covered by an automated test. The original repro file is a private client model and can't be committed here. Adds a purely synthetic replacement instead: resources/frags/ synthetic_large_grid.frag, a 13,824-item grid of plain boxes (2 materials each, so most items carry 3 samples - 41,472 samples total, similar order of magnitude and per-item sample ratio to the original repro) generated with OpenSKP's writer + Fragments exporter. No real geometry or client data of any kind. Extensive tracing (documented in the test's own comment) found that a single, synchronous setVisible() call can't actually land a sample on setSample()'s current===past fast path in an in-process test - fetchLodLevel() reads the live visibility flag directly, so `current` always immediately reflects a toggle, routing through the always-correct updateVisible() path instead. The real trigger likely needs actual worker/event-loop interleaving between a setVisible() RPC and a concurrently-firing update() RPC, which this synchronous harness structurally can't produce - tried across single toggles, rapid re-toggles, toggleVisible() x2, and overlapping batches, on both a single- and multi-representation synthetic fixture, none of which discriminated the bug. Given that, this pins the fixed line directly instead - the same way view-manager.test.ts pins its own guard in isolation: calling setSample() with the exact (vis=false, high=0) combination that silently passed under the bug, asserting on tile.visibilities (the buffer the bug actually corrupts) rather than getVisible() (correct even under the bug, which is why the symptom was invisible to state-only assertions). Verified to fail on the pre-fix code and pass on main. * fix(split): configurable IfcSplitter type lists and options * feat(split): config * feat(split): export config defaults; harden config merge Review follow-up on ThatOpen#268: - export ELEMENT_TYPES, SPATIAL_TYPES and listIdxByType so consumers can extend the defaults instead of only replacing them - {@link listIdxByType} in IfcSplitterConfig now resolves - merge the config field by field, so an explicit `undefined` can no longer overwrite a default and poison Required<IfcSplitterConfig> - cover the merge and each option's effect on the output with tests Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * rm dead test --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(IfcImporter): order-sensitive vertex hashing in the geometry dedup key * fix(IfcImporter): add vertex key to geometry dedup hash * fix order coupling * fix(IfcImporter): use an ordered polynomial for the vertex key The commutative sum could cancel: JS `%` keeps the sign of the dividend, so per-vertex hashes stayed signed and two vertices hashing to exact negatives dropped out of the key entirely. Fold the coordinates with an ordered polynomial instead, normalizing each into [0, MODULUS) first. web-ifc emits vertices in face-iteration order, so a repeated representation comes back byte-identical and still dedups. The only case the ordered fold gives up is the same shape authored with a different triangle order, which costs memory rather than correctness, and it buys back collision resistance. Adds the ThatOpen#237 repro as a regression test: two plates sharing outline, area, volume, centroid and bounding box, differing only in where their bolt holes sit. Before the fix both hashed alike and the second rendered with the first one's holes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(IfcImporter): explain the vertex key fold Rewrite the comments around the dedup hash to teach the mechanism rather than restate it: the fold is decimal place-value with a prime base, which is what makes it order-sensitive. Drop the claim that avoiding bitwise operators buys key width - MODULUS is just under 2 ** 32, so the key is ~32 bits either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(IfcImporter): correct the note on FACTOR and MODULUS The previous wording credited FACTOR being prime for the fold's distribution and implied exceeding 2 ** 53 would break the hash. Neither is quite right: MODULUS being prime is what makes multiplication invertible, FACTOR only has to be large and not a multiple of it, and past 2 ** 53 the fold stays deterministic but distributes worse. Also notes that the base doesn't outrun coordinates at building scale, so the digit-per-coordinate picture is a mental model rather than a literal one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(IfcImporter): fold the vertex key with MurmurHash3's mixer Replaces the modular-arithmetic polynomial with MurmurHash3's 32-bit mixer. Math.imul and the bitwise operators are defined on 32-bit two's complement, so the sign normalization and the 2 ** 53 headroom argument the previous fold needed both disappear - there is nothing to normalize and nothing that can overflow. Picked the mixer by measurement, not preference. Building coordinates are highly structured, and over 68k plates differing only in hole position a word-wise FNV-1a fold collided 13 times against a birthday expectation of 0.5; MurmurHash3 collided once. Byte-wise FNV-1a sat between the two at 2, and cost an allocation and four times the iterations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(IfcImporter): extract the vertex fold into hashCoordinates Moves the coordinate fold out of loadShellGeometry into its own module next to it, inlining the mixing steps rather than splitting them across helpers. The eslint-disable for no-bitwise now covers a file that is entirely 32-bit hash arithmetic instead of sitting mid-method. Kept private: no barrel re-exports it, so it stays out of the package's public API and off the rolled-up .d.ts. Being standalone makes it directly testable, so this also adds unit tests for the properties the dedup key depends on - determinism, order sensitivity, opposite values not cancelling, one quantization step separating, sub-step float noise collapsing, the ThatOpen#237 plates separating, and collisions staying at chance over 90k structured boxes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(): replace hand written hashing with `xxhash-wasm` * init logic * refactor(): Hasher class * refine test --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(splitter): pin geometry shape instead of dedup-dependent absolute ids The extract test pinned literal sampleId/representationId values, which depend on the importer's dedup internals: the ThatOpen#238 hash change shifted them and turned main red. The cross-model comparisons in the same test already carry the real equivalence, so the literals pin only rot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(FragmentsModels): don't crash when a shell needs more render buffers than predicted 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) and 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 'Cannot read properties of undefined (reading indexCount)' on every render frame - a model that would never finish loading. 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) - downstream code (VirtualTilesController.setupTileSampleAttributes) treats those counts as authoritative when copying a shell's data into the merged render tile, so the safe-upper-bound allocation can't be left as the reported size. Extensive empirical testing (large synthetic grid meshes, both ShellType.NONE and ShellType.BIG, matching the triangulated-Face3-only shape a Fragments exporter like openskp actually produces) found no real-world case where the two passes' formulas genuinely disagree at production scale, so the exact organic trigger for the reported crash remains unconfirmed. This fix is a direct, always-safe guard against that failure MODE regardless of what causes it - 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. --------- Co-authored-by: Riho Kirss <riho@kirss.ee> Co-authored-by: arbirk <arbirk@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com> Co-authored-by: rihokirss <84241469+rihokirss@users.noreply.github.com> Co-authored-by: Antonio González Viegas <antoniogviegas@hotmail.com> Co-authored-by: maxkrut <maxkrut@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Config initial work #180
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following:
feat(examples): add hello-world example).fixes #123).