fix(FragmentsModels): don't cull against an uninitialised frustum before useCamera() - #256
fix(FragmentsModels): don't cull against an uninitialised frustum before useCamera()#256arbirk wants to merge 1 commit into
Conversation
|
Thanks, this is a thorough piece of work. Verified it here: build is clean, your tests pass, and I checked the with-camera path separately since that was the regression risk. Indices drawn are identical to main both with the camera on the model and facing away, so culling still functions. Also confirmed visually in a viewer, main loses the negative-X half of Good catch on the two extra dereference sites. Passing null on main throws at One thing before merging. The worker ships as a separate artifact and plenty of people pin or self-host its URL, so a main thread updated ahead of its worker is a real scenario. With this shape it doesn't degrade, it throws Could you make the worker side tolerate a missing frustum, so an old worker paired with a new main thread degrades to current behaviour rather than crashing? |
e530578 to
3417a88
Compare
…s 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>
3417a88 to
706ca40
Compare
|
@agviegas I had to rely on my agents to help me with this one, as I am only a consumer of thatopen's libraries. This summarizes the fix against your comment:
|
Fixes #255.
The bug
ViewManager._updateCameraFrustumEventstays a no-op untiluseCamera()runs, sosetup()ships a defaultTHREE.Frustum. three.js builds that as six planes atnormal
(1,0,0), constant0— i.e. six copies of the planex = 0— andPlanesUtils.collidesthen rejects any item whose bounding box lies entirely atx < 0.Nothing reports it. The geometry stays in the index buffer,
getLocalIds,getItemsData,getMergedBoxandgetItemsGeometryall still list it, and the meshreports
visible === truewith a fulldrawRange. Culling is expressed by omittingthe index range from
geometry.groups, which no data-side API observes. A modelcentred on the origin silently loses half its geometry; ours lost a 1.87 m sliver off
one end and read as a compiler defect for days.
The fix
Per @agviegas preference in the issue — absence is the signal, no
cameraAppliedboolean besidecameraFrustumto keep in sync across the workerboundary:
refreshViewsends no frustum at all untilcurrentCamera !== null(
view-manager.ts)setupViewPlanesskips the copy when it's absent, leaving_virtualPlanesempty(
virtual-tiles-controller.ts) — and an empty plane list means cull nothing,since
PlanesUtils.collidesreturnstrueon an empty arrayconsole.warnnaming the model, because the whole failure mode isthat nothing tells you
Two sites beyond the two discussed — both required
The two-site fix proposed in the issue isn't sufficient on its own. A null frustum has
to survive the full path, and it hits two more dereferences first:
ThreadViewRefresher.safeCopyFrustum— the worker-boundary rehydration callsMultithreadingHelper.frustum(frustum), which doesfrustum.planesunguarded. Withouta guard here the null throws at the boundary, before any of the culling code runs.
VirtualTilesController.updateOrientationIfNeeded/getCurrentViewOrientation—and note the ordering in
setupView: