diff --git a/src/viser/_gui_handles.py b/src/viser/_gui_handles.py index 6df1dd45a..da243989c 100644 --- a/src/viser/_gui_handles.py +++ b/src/viser/_gui_handles.py @@ -1,5 +1,6 @@ from __future__ import annotations +import asyncio import base64 import dataclasses import json @@ -72,6 +73,7 @@ SplitPlacement, ) from ._scene_api import _encode_image_binary +from ._threadpool_exceptions import print_task_error from .infra import ClientId if TYPE_CHECKING: @@ -260,7 +262,12 @@ def value(self, value: T | np.ndarray) -> None: # reduces the likelihood of many common race conditions. cb_out = cb(GuiEvent(client_id=None, client=None, target=self)) if isinstance(cb_out, Coroutine): - self._impl.gui_api._event_loop.create_task(cb_out) + # run_coroutine_threadsafe, not create_task: assignment + # typically happens on a user thread, and create_task is + # neither thread-safe nor guaranteed to wake the loop. + asyncio.run_coroutine_threadsafe( + cb_out, self._impl.gui_api._event_loop + ).add_done_callback(print_task_error) @property def update_timestamp(self) -> float: @@ -387,10 +394,14 @@ def _colors_to_int_tuple(value: Any, *, warn_stacklevel: int) -> tuple[int, ...] assignment path needs 5.""" if isinstance(value, np.ndarray): assert value.ndim == 1, f"Expected a 1D color, got shape {value.shape}." + # Materialize once up front: we iterate twice (the float-channel check and + # the tuple build below), so a generator input would arrive exhausted at + # the second pass and silently yield an empty/partial color. + value = tuple(value) if any(not np.issubdtype(type(v), np.integer) and v > 1.0 for v in value): warnings.warn( "Float color channels are interpreted on [0, 1] and scaled to " - f"[0, 255]; values > 1.0 are clamped to 255 (got {tuple(value)!r}). " + f"[0, 255]; values > 1.0 are clamped to 255 (got {value!r}). " "Use ints for absolute [0, 255] channels.", stacklevel=warn_stacklevel, ) @@ -762,8 +773,8 @@ def _rebuild_tab_props(self) -> None: icon change) rebuilds through here, so the parallel tuples can never desync in length or order.""" if self._impl.removed: - # Tear-down path: PanelHandle.remove() tombstones BEFORE draining - # its tabs (the tombstone must be an atomic check-and-set against + # Tear-down path: both mixers' remove() tombstone BEFORE draining + # their tabs (the tombstone must be an atomic check-and-set against # concurrent removers), so the drain's write-backs land here after # removal. Skip the wire write-back: props_setattr would # (correctly) reject a props write on a removed handle, and the @@ -792,27 +803,29 @@ def __post_init__(self) -> None: def remove(self) -> None: """Remove this tab group and all contained GUI elements.""" - # Warn if already removed. - if self._impl.removed: - warnings.warn( - f"Attempted to remove an already removed {self.__class__.__name__}.", - stacklevel=2, - ) - return - - # Remove tabs first. Each tab.remove() writes back to this group's - # tab-list props (_tab_labels / _tab_icons_html / _tab_container_ids), so - # we must NOT mark the group removed until afterwards -- otherwise the - # removed-handle guard in props_setattr raises on those writes, leaving - # the group half-removed (still in its parent's _children with - # removed=True). A subsequent gui.reset() then spins forever, since its - # `while root._children: child.remove()` loop hits that group whose - # remove() now no-ops via the already-removed guard. + gui_api = self._impl.gui_api + # Tombstone CHECK-AND-SET under the lifecycle lock, mirroring + # PanelHandle.remove(): add_tab() takes the same lock for its + # removed-check + append, so an unlocked tombstone here let a + # concurrent add_tab interleave between our tab snapshot and the + # tombstone -- registering a live container entry for a group that no + # longer exists, which silently accepts children forever. + with gui_api._panel_lifecycle_lock: + if self._impl.removed: + warnings.warn( + f"Attempted to remove an already removed {self.__class__.__name__}.", + stacklevel=2, + ) + return + self._impl.removed = True + gui_api._websock_interface.queue_message(GuiRemoveMessage(self._impl.uuid)) + # Only the tombstone winner reaches here. The tab drain runs AFTER the + # tombstone and OUTSIDE the (non-reentrant) lock: each tab.remove() + # writes back to this group's tab tuples, which _rebuild_tab_props + # skips for a removed group (props_setattr would reject the write; the + # client drops the whole entity via the remove message anyway). for tab in tuple(self._tab_handles): tab.remove() - self._impl.removed = True - gui_api = self._impl.gui_api - gui_api._websock_interface.queue_message(GuiRemoveMessage(self._impl.uuid)) parent = gui_api._container_handle_from_uuid[self._impl.parent_container_id] parent._children.pop(self._impl.uuid) @@ -1470,7 +1483,13 @@ def submit(self) -> None: for cb in self._submit_cb: cb_out = cb(GuiEvent(client_id=None, client=None, target=self)) if isinstance(cb_out, Coroutine): - gui_api._event_loop.create_task(cb_out) + # run_coroutine_threadsafe, not create_task: submit() is + # typically called from a user thread (e.g. a button's + # on_click handler), and create_task is neither thread-safe + # nor guaranteed to wake the loop. + asyncio.run_coroutine_threadsafe( + cb_out, gui_api._event_loop + ).add_done_callback(print_task_error) # Broadcast to clients so they reset dirty state. gui_api._websock_interface.queue_message( GuiFormSubmitMessage(uuid=self._impl.uuid) diff --git a/src/viser/_scene_api.py b/src/viser/_scene_api.py index d25b0918f..2c1ea83c7 100644 --- a/src/viser/_scene_api.py +++ b/src/viser/_scene_api.py @@ -3276,8 +3276,11 @@ def _fire_scene_pointer_done_callbacks(self) -> None: # must not starve its siblings or leave the list uncleared. for cleanup in list(self._scene_pointer_done_cb): if asyncio.iscoroutinefunction(cleanup): - task = self._event_loop.create_task(cleanup()) - task.add_done_callback(print_task_error) + # run_coroutine_threadsafe, not create_task: callback removal + # can run on a user thread, and create_task is neither + # thread-safe nor guaranteed to wake the loop. + future = asyncio.run_coroutine_threadsafe(cleanup(), self._event_loop) + future.add_done_callback(print_task_error) else: try: cleanup() diff --git a/src/viser/_tunnel.py b/src/viser/_tunnel.py index b25463f58..845b913ba 100644 --- a/src/viser/_tunnel.py +++ b/src/viser/_tunnel.py @@ -84,6 +84,15 @@ def on_connect(self, callback: Callable[[int], None]) -> None: def wait_job() -> None: try: self._connect_event.wait() + # close() sets _connect_event to release this thread for + # tunnels that never connected (a "failed" tunnel otherwise + # parks it forever, pinning the mp.Manager and leaking its + # child process on every failed-tunnel replacement). Only a + # real connection runs the callback: _connect_job stores + # "connected" BEFORE setting the event, so this read is + # race-free. + if self._shared_state["status"] != "connected": + return except EOFError: return callback(self._shared_state["max_conn_count"]) @@ -141,14 +150,35 @@ def close(self) -> None: if self._thread is not None: assert self._event_loop is not None - @self._event_loop.call_soon_threadsafe - def _() -> None: - assert self._close_event is not None - self._close_event.set() + try: + + @self._event_loop.call_soon_threadsafe + def _() -> None: + assert self._close_event is not None + self._close_event.set() + + except RuntimeError: + # The tunnel job already tore its loop down (a failed + # connection exits and closes the loop); there's nothing left + # to signal, but we still want the join + disconnect below. + pass self._thread.join() self._disconnect_event.set() + # Release the on_connect watcher thread. For a tunnel that never + # connected, _connect_event was never set, so wait_job would block + # forever -- keeping the mp.Manager proxies (and therefore the + # SyncManager child process) alive long after close(). wait_job's + # status guard keeps this from firing the connect callback. The + # process/thread teardown above already happened, so a late + # _connect_job can no longer flip the status to "connected". + try: + self._connect_event.set() + except (EOFError, BrokenPipeError, ConnectionResetError): + # Manager already gone; its threads died with it. + pass + def _connect_job( connect_event: threading.Event, diff --git a/src/viser/_viser.py b/src/viser/_viser.py index 619c641f0..e2d777ffb 100644 --- a/src/viser/_viser.py +++ b/src/viser/_viser.py @@ -4,6 +4,7 @@ import atexit import dataclasses import io +import math import mimetypes import os import threading @@ -376,6 +377,19 @@ def min_orbit_distance(self) -> float: @min_orbit_distance.setter def min_orbit_distance(self, min_orbit_distance: float) -> None: + # Validate BEFORE the no-op short-circuit: a bad value must raise even + # when it happens to be "close" to the stored one (e.g. re-assigning + # the same bad value after a raise). + if not np.isfinite(min_orbit_distance) or min_orbit_distance <= 0.0: + raise ValueError( + f"min_orbit_distance ({min_orbit_distance}) must be a " + "positive, finite number." + ) + if min_orbit_distance > self._state.max_orbit_distance: + raise ValueError( + f"min_orbit_distance ({min_orbit_distance}) must be <= " + f"max_orbit_distance ({self._state.max_orbit_distance})." + ) if np.allclose(self._state.min_orbit_distance, min_orbit_distance): return self._state.min_orbit_distance = min_orbit_distance @@ -388,7 +402,8 @@ def min_orbit_distance(self, min_orbit_distance: float) -> None: def max_orbit_distance(self) -> float: """How far the camera may be dollied out from its orbit (look-at) point. Distinct from :attr:`far`, which clips rendering rather than camera - travel. Defaults to 1e4. Synchronized automatically when assigned. + travel. Defaults to 1e4; set to ``float("inf")`` for the unbounded + pre-1.1.0 behavior. Synchronized automatically when assigned. Dolly is multiplicative per wheel event, so a very large maximum means a long scroll — a trackpad's inertial tail, for instance — can walk the camera out to @@ -399,6 +414,19 @@ def max_orbit_distance(self) -> float: @max_orbit_distance.setter def max_orbit_distance(self, max_orbit_distance: float) -> None: + # Validate BEFORE the no-op short-circuit; see min_orbit_distance. + # +inf is allowed: it is the pre-1.1.0 unbounded behavior (and the + # camera-controls default), the opt-out for large-scale scenes. + if math.isnan(max_orbit_distance) or max_orbit_distance <= 0.0: + raise ValueError( + f"max_orbit_distance ({max_orbit_distance}) must be a " + "positive number (or float('inf') for unbounded)." + ) + if max_orbit_distance < self._state.min_orbit_distance: + raise ValueError( + f"max_orbit_distance ({max_orbit_distance}) must be >= " + f"min_orbit_distance ({self._state.min_orbit_distance})." + ) if np.allclose(self._state.max_orbit_distance, max_orbit_distance): return self._state.max_orbit_distance = max_orbit_distance @@ -1093,6 +1121,18 @@ async def _(conn: infra.WebsockClientConnection) -> None: # Start the server. server.start() + # The share-tunnel slot must exist BEFORE stop() is registered with + # atexit below: stop() reads it, and if __init__ fails later, the + # atexit-time stop() on the partially-built server would otherwise + # miss the attribute and fall into DeprecatedAttributeShim.__getattr__ + # (whose `self.scene` lookup recurses without `scene` set --> + # RecursionError at interpreter exit). + self._share_tunnel: ViserTunnel | None = None + # Guards the share-tunnel slot's check-then-act. The handlers below run + # on pool threads (not serialized on the event loop), so two + # concurrent requests could both see None and each create a tunnel, + # orphaning (leaking) the first. + self._share_tunnel_lock = threading.Lock() # server.start() registered the infra-level WebsockServer.stop with # atexit; also register the full ViserServer.stop, which runs first # (LIFO) and supersedes it (WebsockServer.stop unregisters itself). @@ -1169,13 +1209,6 @@ async def _(conn: infra.WebsockClientConnection) -> None: ) ) - self._share_tunnel: ViserTunnel | None = None - # Guards the share-tunnel slot's check-then-act. The handlers now run - # on pool threads (not serialized on the event loop), so two - # concurrent requests could both see None and each create a tunnel, - # orphaning (leaking) the first. - self._share_tunnel_lock = threading.Lock() - # Create share tunnel if requested. # This is deprecated: we should use get_share_url() instead. share = _deprecated_kwargs.get("share", False) @@ -1336,6 +1369,14 @@ def request_share_url(self, verbose: bool = True) -> str | None: # OUTSIDE the lock so requests don't serialize on the connection. with self._share_tunnel_lock: tunnel = self._share_tunnel + if tunnel is not None and tunnel.get_status() in ("failed", "closed"): + # A tunnel that failed to connect never signals disconnect, so + # nothing else clears the slot: without this, every later + # request would see the dead tunnel and return None forever. + # Close it (its worker already exited, so this is bookkeeping, + # not blocking) and let this request create a fresh one. + tunnel.close() + tunnel = self._share_tunnel = None we_created = tunnel is None if we_created: if verbose: @@ -1363,10 +1404,14 @@ def _() -> None: rich.print("[bold](viser)[/bold] Disconnected from share URL") with self._share_tunnel_lock: - # Only clear the slot if it still points at OUR tunnel (a - # newer request may have replaced it). - if self._share_tunnel is tunnel: - self._share_tunnel = None + # Only clear the slot (and broadcast the loss) if it still + # points at OUR tunnel. A newer request may have replaced it: + # broadcasting None here would clobber the replacement's URL + # for every client -- and, since the message persists in the + # broadcast buffer, for every late joiner too. + if self._share_tunnel is not tunnel: + return + self._share_tunnel = None self._websock_server.queue_message(_messages.ShareUrlUpdated(None)) @tunnel.on_connect @@ -1484,8 +1529,11 @@ def on_client_connect( # connect between the two lines. for client in clients: if asyncio.iscoroutinefunction(cb): - task = self._event_loop.create_task(cb(client)) - task.add_done_callback(print_task_error) + # run_coroutine_threadsafe, not create_task: registration + # typically happens on a user thread, and create_task is + # neither thread-safe nor guaranteed to wake the loop. + future = asyncio.run_coroutine_threadsafe(cb(client), self._event_loop) + future.add_done_callback(print_task_error) else: self._thread_executor.submit(cb, client).add_done_callback( print_threadpool_errors diff --git a/src/viser/client/src/App.tsx b/src/viser/client/src/App.tsx index b34e9ec05..f1ca09a7b 100644 --- a/src/viser/client/src/App.tsx +++ b/src/viser/client/src/App.tsx @@ -414,8 +414,18 @@ function AppLayout({ canvases: React.ReactNode; }) { const mantineTheme = useMantineTheme(); - const useMobileView = - useMediaQuery(`(max-width: ${mantineTheme.breakpoints.xs})`) ?? false; + // `getInitialValueInEffect: false` makes the first render read matchMedia + // synchronously. With the effect-based default, a mobile-width load rendered + // desktop-first (`useMobileView === false`), mounted the canvas inside + // , then flipped on the next render -- changing the + // element structure around `canvasContent` and remounting the entire canvas + // subtree (new WebGL context). This is a client-only app, so there is no SSR + // path that needs the deferred read. + const useMobileView = useMediaQuery( + `(max-width: ${mantineTheme.breakpoints.xs})`, + false, + { getInitialValueInEffect: false }, + ); // The floating layout runs on the docking library: the control panel is a // dock panel over the canvas (draggable, dockable to either edge, resizable, // minimizable). Sidebar layouts and the mobile bottom sheet are unchanged. diff --git a/src/viser/client/src/ControlPanel/ControlPanel.tsx b/src/viser/client/src/ControlPanel/ControlPanel.tsx index c42961973..1b2dc78e5 100644 --- a/src/viser/client/src/ControlPanel/ControlPanel.tsx +++ b/src/viser/client/src/ControlPanel/ControlPanel.tsx @@ -11,7 +11,7 @@ import { GuiComponentContext } from "./GuiComponentContext"; import { htmlIconWrapper } from "../components/ComponentStyles.css"; import { GuiDockContext } from "./GuiDockContext"; import { DockContext } from "../dock/DockContext"; -import { shallowObjectKeysEqual } from "../utils/shallowObjectKeysEqual"; +import { panelsSelectorEquality } from "./panelsEquality"; import { ActionIcon, Anchor, @@ -260,7 +260,9 @@ function MobilePanelSection({ panel }: { panel: GuiPanelMessage }) { * honored on this path too); sections sort by the server-side order. */ function PanelsFallback() { const viewer = React.useContext(ViewerContext)!; - const panels = viewer.useGui((state) => state.panels, shallowObjectKeysEqual); + // Value-level equality, NOT key-set equality -- see panelsEquality.ts, + // which panelUpdates.test.ts pins. + const panels = viewer.useGui((state) => state.panels, panelsSelectorEquality); // On the dock surface, StandalonePanelSync places panels as dock groups -- so // this inline fallback must NOT also render them (that would double-render). const dockCtx = React.useContext(DockContext); diff --git a/src/viser/client/src/ControlPanel/panelUpdates.test.ts b/src/viser/client/src/ControlPanel/panelUpdates.test.ts new file mode 100644 index 000000000..78d7c6d14 --- /dev/null +++ b/src/viser/client/src/ControlPanel/panelUpdates.test.ts @@ -0,0 +1,84 @@ +// Pins the equality function used by PanelsFallback's `state.panels` +// selector. Regression: shallowObjectKeysEqual compares KEY SETS only, but +// updatePanel replaces the panel object for the updated uuid without changing +// the key set -- so prop updates (visible/order/tab metadata) never reached +// the mobile sheet. The selector must use value-level shallowObjectEqual. + +import { describe, expect, it } from "vitest"; +import { + shallowObjectEqual, + shallowObjectKeysEqual, +} from "../utils/shallowObjectKeysEqual"; +import { createStore } from "../store"; +import { panelsSelectorEquality } from "./panelsEquality"; +import controlPanelSource from "./ControlPanel.tsx?raw"; + +describe("panels selector equality", () => { + it("shallowObjectKeysEqual misses same-key value changes (the old bug)", () => { + // Same key set, different value: the keys-only compare reports "equal", + // which is exactly why panel prop updates were swallowed. + expect(shallowObjectKeysEqual({ a: 1 }, { a: 2 })).toBe(true); + }); + + it("shallowObjectEqual detects same-key value changes", () => { + expect(shallowObjectEqual({ a: 1 }, { a: 2 })).toBe(false); + expect(shallowObjectEqual({ a: 1 }, { a: 1 })).toBe(true); + }); + + it("a subscriber using shallowObjectEqual sees an updatePanel-style change", () => { + type Panel = { uuid: string; props: { visible: boolean; order: number } }; + const panelA: Panel = { uuid: "a", props: { visible: true, order: 0 } }; + const store = createStore({ + panels: { a: panelA } as Record, + }); + + // Mirror the useStore snapshot logic (selector + equality cache) without + // React: on each notification, keep the cached slice iff equalityFn says + // it's unchanged. + const track = (equalityFn: (a: any, b: any) => boolean) => { + let cached = store.get().panels; + const seen: Record[] = []; + store.subscribe(() => { + const next = store.get().panels; + if (!equalityFn(cached, next)) { + cached = next; + seen.push(next); + } + }); + return seen; + }; + const seenByKeys = track(shallowObjectKeysEqual); + const seenByValues = track(shallowObjectEqual); + + // updatePanel-style change: replace the panel object for one uuid, + // keeping the key set identical (GuiState.updatePanel). + store.set((state) => ({ + panels: { + ...state.panels, + a: { ...panelA, props: { ...panelA.props, visible: false } }, + }, + })); + + expect(seenByKeys).toHaveLength(0); // the old bug: update invisible + expect(seenByValues).toHaveLength(1); + expect(seenByValues[0].a.props.visible).toBe(false); + }); + + // The tests above pin the UTILITIES; these two pin the WIRING. Without + // them, reverting the component to the keys-only comparator would leave + // this whole suite green while the mobile-sheet regression returns. + it("panelsSelectorEquality is the value-level comparator", () => { + expect(panelsSelectorEquality).toBe(shallowObjectEqual); + }); + + it("PanelsFallback's panels selector subscribes with panelsSelectorEquality", () => { + // Source-level pin (the component renders a Mantine tree, so a render + // test would drag in far more than this assertion needs): the selector + // must pass the pinned comparator, and the component must not import a + // comparator directly. + expect(controlPanelSource).toMatch( + /useGui\(\s*\(state\) => state\.panels,\s*panelsSelectorEquality,?\s*\)/, + ); + expect(controlPanelSource).not.toContain("shallowObjectKeysEqual"); + }); +}); diff --git a/src/viser/client/src/ControlPanel/panelsEquality.ts b/src/viser/client/src/ControlPanel/panelsEquality.ts new file mode 100644 index 000000000..744ef235c --- /dev/null +++ b/src/viser/client/src/ControlPanel/panelsEquality.ts @@ -0,0 +1,14 @@ +import { shallowObjectEqual } from "../utils/shallowObjectKeysEqual"; + +/** Equality used by PanelsFallback's `state.panels` selector (the mobile + * bottom sheet's panel list). + * + * This MUST stay value-level (`shallowObjectEqual`), NOT key-set equality: + * `updatePanel` replaces the panel object for the updated uuid without + * changing the key set, so a keys-only compare swallows server-side + * visible/order/tab-metadata updates and leaves the sheet rendering stale + * panel props. The choice lives in this dedicated module so + * `panelUpdates.test.ts` can pin the exact function the component subscribes + * with -- a test importing the utility alone couldn't catch the component + * quietly switching comparators. */ +export const panelsSelectorEquality = shallowObjectEqual; diff --git a/src/viser/client/src/Splatting/SplatSortWorker.test.ts b/src/viser/client/src/Splatting/SplatSortWorker.test.ts new file mode 100644 index 000000000..8cfd3f38f --- /dev/null +++ b/src/viser/client/src/Splatting/SplatSortWorker.test.ts @@ -0,0 +1,124 @@ +// Pins the sort worker's buffer-staleness fix: an updateBuffer that arrives +// while a sort is running used to be dropped by the throttle, and the +// deferred retry only re-sorted on a VIEW change -- so with a stationary +// camera (the main thread only posts setTz_camera_groups when groups move +// w.r.t. the camera) the stale order persisted until the camera moved. +// +// The worker module is importable directly: it only touches `self`, `fetch`, +// and the WASM sorter module, all of which we stub before import. + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const hoisted = vi.hoisted(() => ({ + sorters: [] as FakeSorter[], +})); + +class FakeSorter { + buffer: Uint32Array; + // Buffer identity at the time of each sort() call. + sortedBuffers: Uint32Array[] = []; + constructor(buffer: Uint32Array) { + this.buffer = buffer; + hoisted.sorters.push(this); + } + setBuffer(buffer: Uint32Array) { + this.buffer = buffer; + } + sort(): Uint32Array { + this.sortedBuffers.push(this.buffer); + return new Uint32Array(1); + } +} + +vi.mock("./WasmSorter/Sorter.mjs", () => ({ + default: async () => ({ Sorter: FakeSorter }), +})); +vi.mock("./WasmSorter/Sorter.wasm?url", () => ({ default: "sorter.wasm" })); + +async function importWorker() { + const posted: unknown[] = []; + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ arrayBuffer: async () => new ArrayBuffer(0) })), + ); + const fakeSelf = { + onmessage: null as ((e: { data: unknown }) => void) | null, + postMessage: (msg: unknown) => posted.push(msg), + close: () => {}, + }; + vi.stubGlobal("self", fakeSelf); + await import("./SplatSortWorker"); + const send = (data: unknown) => fakeSelf.onmessage!({ data }); + return { posted, send }; +} + +const flush = () => new Promise((resolve) => setTimeout(resolve, 20)); + +describe("SplatSortWorker buffer staleness", () => { + beforeEach(() => { + vi.resetModules(); + hoisted.sorters.length = 0; + }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("sorts on setTz_camera_groups", async () => { + const { posted, send } = await importWorker(); + send({ + setBuffer: new Uint32Array(8), + setGroupIndices: new Uint32Array(1), + }); + send({ setTz_camera_groups: new Float32Array(4) }); + await flush(); + expect(posted).toHaveLength(1); + }); + + it("re-sorts a buffer swapped in while a sort was running, without a view change", async () => { + const { posted, send } = await importWorker(); + const bufferV1 = new Uint32Array(8); + const bufferV2 = new Uint32Array(8); + + send({ setBuffer: bufferV1, setGroupIndices: new Uint32Array(1) }); + // setTz starts a sort of V1; sortRunning stays true until its deferred + // setTimeout(0) fires. The updateBuffer below is handled on the promise + // chain (a microtask), so it lands INSIDE that window and its + // throttledSort() call is dropped -- the dirty flag alone must trigger + // the retry, since the view never changes. + send({ setTz_camera_groups: new Float32Array(4) }); + send({ + updateBuffer: bufferV2, + updateGroupIndices: new Uint32Array(1), + updateNumGroups: 1, + }); + await flush(); + + expect(posted).toHaveLength(2); + expect(hoisted.sorters).toHaveLength(1); + // Identity, not toEqual: both buffers hold the same (zero) content, so + // only reference checks prove the retry sorted the SWAPPED-IN buffer. + expect(hoisted.sorters[0].sortedBuffers.map((b) => b === bufferV1)).toEqual( + [true, false], + ); + expect(hoisted.sorters[0].sortedBuffers[1]).toBe(bufferV2); + }); + + it("does not re-sort after an idle sort with unchanged view and buffer", async () => { + const { posted, send } = await importWorker(); + send({ + setBuffer: new Uint32Array(8), + setGroupIndices: new Uint32Array(1), + }); + send({ setTz_camera_groups: new Float32Array(4) }); + await flush(); + // Buffer update while idle: exactly one additional sort (the immediate + // one), no spurious dirty-flag retry afterwards. + send({ + updateBuffer: new Uint32Array(8), + updateGroupIndices: new Uint32Array(1), + updateNumGroups: 1, + }); + await flush(); + expect(posted).toHaveLength(2); + }); +}); diff --git a/src/viser/client/src/Splatting/SplatSortWorker.ts b/src/viser/client/src/Splatting/SplatSortWorker.ts index 0e9a65c82..5ff957e42 100644 --- a/src/viser/client/src/Splatting/SplatSortWorker.ts +++ b/src/viser/client/src/Splatting/SplatSortWorker.ts @@ -24,6 +24,13 @@ export type SorterWorkerIncoming = let sorter: any = null; let Tz_camera_groups: Float32Array | null = null; let sortRunning = false; + // Set when updateBuffer swaps the sorter's contents; cleared when a sort + // actually begins (that sort reads the new buffer). Without it, an + // updateBuffer arriving while a sort runs is dropped by the throttle, and + // the deferred retry below only re-sorts on a VIEW change -- so with a + // stationary camera (the main thread only posts setTz_camera_groups when + // groups move w.r.t. the camera) the stale order persisted indefinitely. + let bufferDirty = false; const throttledSort = () => { if (sorter === null || Tz_camera_groups === null) { setTimeout(throttledSort, 1); @@ -32,6 +39,7 @@ export type SorterWorkerIncoming = if (sortRunning) return; sortRunning = true; + bufferDirty = false; // This sort consumes the latest buffer. const lastView = Tz_camera_groups; // Important: we clone the output so we can transfer the buffer to the main @@ -48,6 +56,7 @@ export type SorterWorkerIncoming = sortRunning = false; if (Tz_camera_groups === null) return; if ( + bufferDirty || !lastView.every( // Cast is needed because of closure... (val, i) => val === (Tz_camera_groups as Float32Array)[i], @@ -74,6 +83,10 @@ export type SorterWorkerIncoming = // Update existing sorter with new buffer data. if (sorter !== null) { sorter.setBuffer(data.updateBuffer, data.updateGroupIndices); + // Mark the swapped buffer as unsorted so the post-sort retry re-sorts + // even when the view hasn't changed (throttledSort clears this when a + // sort starts). + bufferDirty = true; // If the group COUNT changed, the current Tz_camera_groups is the // wrong size: sort() derives num_groups from Tz's length, so it // would gather transforms past Tz's end (OOB heap read) for the new diff --git a/src/viser/client/src/dock/DockContext.tsx b/src/viser/client/src/dock/DockContext.tsx index 5a1558506..09ed9d022 100644 --- a/src/viser/client/src/dock/DockContext.tsx +++ b/src/viser/client/src/dock/DockContext.tsx @@ -4,6 +4,7 @@ // split tree by hand. import React from "react"; +import { GestureSlot } from "./gestures"; import { NodeId, AreaId, @@ -45,6 +46,13 @@ export interface DockApi { export interface DockContextValue { panes: PaneRegistry; + /** The one-gesture mutex (spec §4: "One active gesture at a time; extra + * pointers are ignored"): the shared slot EVERY gesture entry point -- + * move drags (via the start* gestures below, which check it internally) + * and the resize surfaces (dividers, window grips, the region resizer) -- + * acquires. Ref-backed and identity-stable across renders; DockManager + * owns the ref and runs the parked cleanup if it unmounts mid-gesture. */ + gestureSlot: GestureSlot; /** Imperative pane lifecycle API (stable identity). */ api: DockApi; /** The committed layout, for sync layers that need to observe where things diff --git a/src/viser/client/src/dock/DockManager.tsx b/src/viser/client/src/dock/DockManager.tsx index 4430bc191..b20a9be9b 100644 --- a/src/viser/client/src/dock/DockManager.tsx +++ b/src/viser/client/src/dock/DockManager.tsx @@ -162,6 +162,11 @@ export function DockManager({ // the drag's cached target rects stale (a window growing mid-drag). const markDragTargetsStaleRef = React.useRef<(() => void) | null>(null); // Cleanup for an in-flight gesture, run if the manager unmounts mid-drag. + // Doubles as the dock-wide one-gesture mutex (spec §4: "One active gesture + // at a time; extra pointers are ignored"): move drags (dragController) and + // every resize surface (dividers, window grips, the region resizer -- + // threaded as `gestureSlot` via context/props) park their cleanup here, so + // a press on any gesture surface while one is live is ignored. const activeCleanup = React.useRef<(() => void) | null>(null); React.useEffect(() => () => activeCleanup.current?.(), []); @@ -565,6 +570,8 @@ export function DockManager({ layout, groups: layout.groups, areas: layout.areas ?? {}, + // Identity-stable ref: safe under the memo (never invalidates it). + gestureSlot: activeCleanup, ...stableGestures, activateTab, expandToTab, @@ -1015,6 +1022,7 @@ export function DockManager({ {resizable && ( regions[edge].reservedWidth} diff --git a/src/viser/client/src/dock/FloatingWindowView.tsx b/src/viser/client/src/dock/FloatingWindowView.tsx index 3c0ffc984..43533641a 100644 --- a/src/viser/client/src/dock/FloatingWindowView.tsx +++ b/src/viser/client/src/dock/FloatingWindowView.tsx @@ -7,10 +7,11 @@ import { Box, Paper } from "@mantine/core"; import React from "react"; import { flushSync } from "react-dom"; import { useDock } from "./DockContext"; -import { dragGesture } from "./gestures"; +import { exclusiveDragGesture } from "./gestures"; import { cappedWindowHeight, cascadeResize, + stackDividerHeightPlan, windowAllMinimized, } from "./layoutOps"; import { StackHandleBar } from "./handles"; @@ -270,8 +271,10 @@ export const FloatingWindowView = React.memo(function FloatingWindowView({ // Cancel an in-flight grip gesture if this window unmounts mid-resize (e.g. // another client docks it away), so its listeners can't fire afterwards. One - // ref serves all grips: only one resize gesture may be active per window (a - // second finger on another grip is ignored while one is running). + // ref serves all grips for unmount cleanup; gesture EXCLUSION is the + // dock-wide slot (spec §4: one active gesture at a time, extra pointers + // ignored) -- a second finger on any grip, divider, or drag surface is + // ignored while one gesture is running anywhere in the dock. const activeGrip = React.useRef<(() => void) | null>(null); React.useEffect(() => () => activeGrip.current?.(), []); // True while a height resize is magnetized to the content-height detent (the @@ -314,14 +317,14 @@ export const FloatingWindowView = React.memo(function FloatingWindowView({ const widthResizeHandler = (side: "left" | "right") => (event: React.PointerEvent) => { if (event.button !== 0) return; - if (activeGrip.current !== null) return; + if (dock.gestureSlot.current !== null) return; // one gesture at a time (§4) event.stopPropagation(); const startX = event.clientX; const { startWidth, widthFrom, xFor, startXRestore } = wResizeStart(side); let pending = startWidth; let pendingX: number | undefined = undefined; - activeGrip.current = dragGesture({ + activeGrip.current = exclusiveDragGesture(dock.gestureSlot, { grip: event.currentTarget, pointerId: event.pointerId, update: (e) => { @@ -422,7 +425,7 @@ export const FloatingWindowView = React.memo(function FloatingWindowView({ (vside: "top" | "bottom") => (event: React.PointerEvent) => { if (event.button !== 0) return; - if (activeGrip.current !== null) return; + if (dock.gestureSlot.current !== null) return; // one gesture at a time (§4) event.stopPropagation(); // Per-frame height writes must track the cursor 1:1: suppress the D34 // height ease (windowCollapseAnim) on this window for the drag. @@ -438,7 +441,7 @@ export const FloatingWindowView = React.memo(function FloatingWindowView({ } = vResizeStart(vside); let pending = startHeight; - activeGrip.current = dragGesture({ + activeGrip.current = exclusiveDragGesture(dock.gestureSlot, { grip: event.currentTarget, pointerId: event.pointerId, update: (e) => { @@ -463,7 +466,7 @@ export const FloatingWindowView = React.memo(function FloatingWindowView({ (side: "left" | "right", vside: "top" | "bottom" = "bottom") => (event: React.PointerEvent) => { if (event.button !== 0) return; - if (activeGrip.current !== null) return; + if (dock.gestureSlot.current !== null) return; // one gesture at a time (§4) event.stopPropagation(); // Corner drags also write height per frame: suppress the D34 ease. paperRef.current?.setAttribute("data-dock-resizing", ""); @@ -483,7 +486,7 @@ export const FloatingWindowView = React.memo(function FloatingWindowView({ let pendingX: number | undefined = undefined; let pendingH = startHeight; // Suppress the width-ease while dragging the corner (width tracks 1:1). - activeGrip.current = dragGesture({ + activeGrip.current = exclusiveDragGesture(dock.gestureSlot, { grip: event.currentTarget, pointerId: event.pointerId, update: (e) => { @@ -723,6 +726,9 @@ export const FloatingWindowView = React.memo(function FloatingWindowView({ onSetStackWeights(win.id, weights) } isFixed={fixedHeight} + // The STORED pin (uncapped): what a cancel/motionless + // press must restore (stackDividerHeightPlan). + pinnedPx={pinnedPx} setWindowHeight={(px) => onResizeHeight(win.id, px)} /> )} @@ -775,6 +781,7 @@ function FloatingStackDivider({ weightOf, onSetWeights, isFixed, + pinnedPx, setWindowHeight, }: { stackRef: React.RefObject; @@ -792,9 +799,15 @@ function FloatingStackDivider({ * pinned mode reproduces the exact on-screen layout and a motionless * click cannot move anything. */ isFixed: boolean; + /** The STORED pin (pinnedPxOf(win.height)) -- uncapped, unlike the paper's + * rendered offsetHeight -- so a cancel/motionless press restores exactly + * it (stackDividerHeightPlan / P2). Undefined for an auto window. */ + pinnedPx: number | undefined; /** Pin (px) or restore auto (undefined) -- the window height writer. */ setWindowHeight: (px: number | undefined) => void; }) { + // The dock-wide one-gesture mutex (spec §4), shared via context. + const { gestureSlot } = useDock(); // Cancel the in-flight gesture if the divider unmounts mid-drag (the stack // can be restructured by another client), so the window listeners can't fire // after unmount and the shared `resizing` flag can't stick true. @@ -807,7 +820,9 @@ function FloatingStackDivider({ const onPointerDown = (event: React.PointerEvent) => { if (!resizable) return; if (event.button !== 0) return; - if (activeDrag.current !== null) return; // one drag per divider + // One gesture at a time, dock-wide (spec §4): a press while any gesture + // is live is ignored -- checked BEFORE the pin/weight side effects below. + if (gestureSlot.current !== null) return; event.stopPropagation(); const container = stackRef.current; if (container === null) return; @@ -818,8 +833,16 @@ function FloatingStackDivider({ // + the auto-height FLIP) stays out of per-frame height writes. container.setAttribute("data-dock-resizing", ""); paperRef.current?.setAttribute("data-dock-resizing", ""); + // The RENDERED height (capped) for live drag math, and the height plan + // separating it from the STORED pin a cancel must commit + // (stackDividerHeightPlan: committing the rendered height on a + // motionless press rewrote a stored 700px pin to the capped px). const paperStartH = paperRef.current?.offsetHeight ?? containerPx; const wasFixed = isFixed; + const heightPlan = stackDividerHeightPlan({ + storedPinnedPx: wasFixed ? pinnedPx : undefined, + renderedPx: paperStartH, + }); // Snapshot the cells' RENDERED heights BEFORE any mode change: these // seed the pinned-mode weights, so flipping an auto window to fixed // reproduces the exact on-screen layout (the old path seeded stored @@ -839,8 +862,8 @@ function FloatingStackDivider({ stack.forEach((g) => { startWeights[g] = weightOf(g); }); - if (!wasFixed) { - setWindowHeight(paperStartH); + if (heightPlan.pinOnDown !== null) { + setWindowHeight(heightPlan.pinOnDown); onSetWeights(renderedPx); } const start = event.clientY; @@ -892,7 +915,7 @@ function FloatingStackDivider({ const y = paperRef.current.offsetTop; return Math.max(paperStartH, parent.clientHeight - y); }; - activeDrag.current = dragGesture({ + activeDrag.current = exclusiveDragGesture(gestureSlot, { grip: event.currentTarget, pointerId: event.pointerId, update: (e) => { @@ -960,10 +983,14 @@ function FloatingStackDivider({ // Escape OR a motionless click: full restore -- weights AND the // height mode (an auto window a click briefly pinned reverts to // auto; P2: layout, sizes, and modes return to pre-gesture - // values). The detent never touches this path: cancel restores - // the exact pre-gesture mode/values regardless of any snap. + // values). The height committed is the STORED pin, never the + // rendered offsetHeight (heightPlan doc; the grip's + // startHeightCommit rule): a motionless press on a pinned window + // is a no-op on stored state. The detent never touches this + // path: cancel restores the exact pre-gesture mode/values + // regardless of any snap. onSetWeights(startWeights); - setWindowHeight(wasFixed ? paperStartH : undefined); + setWindowHeight(heightPlan.cancelCommit); return; } // THE SEMANTIC ARM (D56): releasing with EVERY cell of the stack at diff --git a/src/viser/client/src/dock/RegionResizer.tsx b/src/viser/client/src/dock/RegionResizer.tsx index dbd5aa88c..fde153334 100644 --- a/src/viser/client/src/dock/RegionResizer.tsx +++ b/src/viser/client/src/dock/RegionResizer.tsx @@ -2,7 +2,7 @@ import { Box } from "@mantine/core"; import React from "react"; -import { dragGesture } from "./gestures"; +import { exclusiveDragGesture, GestureSlot } from "./gestures"; import { DockEdge } from "./types"; // How far the grab zone extends onto the canvas side of the region boundary. @@ -19,10 +19,13 @@ const GRIP_BAR_CLEARANCE_PX = 48; export function RegionResizer({ edge, + gestureSlot, makeOnResize, getStart, }: { edge: DockEdge; + /** The dock-wide one-gesture mutex (spec §4) owned by DockManager. */ + gestureSlot: GestureSlot; /** Called once per drag (at pointer down) so the handlers can snapshot the * columns' start widths (and the drag-start layout); returns the per-frame * resize handler plus the release/cancel handler (called after the final @@ -39,13 +42,17 @@ export function RegionResizer({ React.useEffect(() => () => activeDrag.current?.(), []); const onPointerDown = (event: React.PointerEvent) => { if (event.button !== 0) return; - if (activeDrag.current !== null) return; // one drag per resizer + // One gesture at a time, dock-wide (spec §4): a press while ANY gesture + // is live -- a move drag, another resize -- is ignored, via the same + // shared slot dragController's armPress checks. Checked before the + // makeOnResize side effects (it sets data-dock-resizing). + if (gestureSlot.current !== null) return; event.stopPropagation(); const startX = event.clientX; const startWidth = getStart(); const handlers = makeOnResize(); let pending = startWidth; - activeDrag.current = dragGesture({ + activeDrag.current = exclusiveDragGesture(gestureSlot, { grip: event.currentTarget, pointerId: event.pointerId, update: (e) => { diff --git a/src/viser/client/src/dock/SplitView.tsx b/src/viser/client/src/dock/SplitView.tsx index 2620d064a..4459e00a7 100644 --- a/src/viser/client/src/dock/SplitView.tsx +++ b/src/viser/client/src/dock/SplitView.tsx @@ -15,7 +15,7 @@ import { Box, Paper } from "@mantine/core"; import React from "react"; import { useDock } from "./DockContext"; -import { dragGesture, focusDockControl } from "./gestures"; +import { exclusiveDragGesture, focusDockControl } from "./gestures"; import { collapseAnim } from "./DockStyles.css"; import { cascadeResize, expandedFlags, setNodeWeights } from "./layoutOps"; import { ColumnCollapseChevron, StackHandleBar } from "./handles"; @@ -625,6 +625,9 @@ function SplitDivider({ contentDetents?: () => number[]; }) { const isRow = dir === "row"; + // The dock-wide one-gesture mutex (spec §4), shared with every other + // gesture surface via context. + const { gestureSlot } = useDock(); // Cancel the in-flight gesture if the divider unmounts mid-drag (its region // can be restructured by another client), so the window listeners can't fire // after unmount. @@ -637,7 +640,9 @@ function SplitDivider({ const onPointerDown = (event: React.PointerEvent) => { if (event.button !== 0) return; if (!resizable) return; // nothing to resize between minimized strips - if (activeDrag.current !== null) return; // one drag per divider + // One gesture at a time, dock-wide (spec §4): a press while any gesture + // -- a move drag, another resize -- is live is ignored. + if (gestureSlot.current !== null) return; event.stopPropagation(); const container = containerRef.current; if (container === null) return; @@ -653,7 +658,7 @@ function SplitDivider({ // the drag's duration, or cells ease-lag behind the divider. container.setAttribute("data-dock-resizing", ""); let latest = start; - activeDrag.current = dragGesture({ + activeDrag.current = exclusiveDragGesture(gestureSlot, { grip: event.currentTarget, pointerId: event.pointerId, update: (e) => { diff --git a/src/viser/client/src/dock/dock-ux-spec.md b/src/viser/client/src/dock/dock-ux-spec.md index e7111ef82..b1012fc22 100644 --- a/src/viser/client/src/dock/dock-ux-spec.md +++ b/src/viser/client/src/dock/dock-ux-spec.md @@ -631,7 +631,16 @@ constants in `hitTest.ts`; changing one is a spec change. band would shadow a whole 36px strip whose own 8px sliver already resolves to the same seam insert, and packed strips tile the whole region, so dock-beside there is entirely the rails' own slivers - (edge case 13). Suppressed where they'd duplicate the per-cell + (edge case 13). The yield covers each band's WHOLE run wherever it + horizontally overlaps a collapsed cell (at the pointer's height + within that cell), the cell's own pixels and the band's few-px + overhang past a 36px strip alike: the overhang sits on the strip's + adjacent seam between two zones that resolve there, so it must + resolve there too — the same-seam rule above — never to the region + boundary (which made the line hop outward and back across a 3px + sweep). The rail HEADER run is unaffected: no cell owns the pointer + at that height, so the bands still claim those pixels (§5.3). + Suppressed where they'd duplicate the per-cell resolution (a single-column, single-leaf region) and while a floating window's paper rect owns the pointer (§3.5). 4. **Per-target zones**: the cell-, rail-, and bar-level zones of diff --git a/src/viser/client/src/dock/floatingWindowHeight.test.ts b/src/viser/client/src/dock/floatingWindowHeight.test.ts index 710ce980b..12935120e 100644 --- a/src/viser/client/src/dock/floatingWindowHeight.test.ts +++ b/src/viser/client/src/dock/floatingWindowHeight.test.ts @@ -6,7 +6,7 @@ // D33 header/face-bar constancy constant. import { describe, expect, it } from "vitest"; -import { cappedWindowHeight } from "./layoutOps"; +import { cappedWindowHeight, stackDividerHeightPlan } from "./layoutOps"; import { collapsedWindowHeightCss, FACE_BAR_EM, @@ -50,6 +50,39 @@ describe("cappedWindowHeight", () => { }); }); +// P2 pin for the stack divider's cancel path (spec §4's floating +// height-divider row: "a motionless press restores everything"): what a +// cancel/motionless press commits is the STORED pin (win.height), never the +// paper's rendered offsetHeight -- which is cappedWindowHeight(pin, +// container) and therefore SMALLER whenever the container is shorter than +// the pin. Committing the rendered height rewrote a stored 700 to 392 in a +// 400px container on a motionless divider press. +describe("stackDividerHeightPlan (cancel restores the stored pin)", () => { + it("pinned window: motionless press / Escape commit the stored 700, not the capped 392", () => { + const stored = 700; + const rendered = cappedWindowHeight(stored, 400); // 392 on screen + expect(rendered).toBe(392); + const plan = stackDividerHeightPlan({ + storedPinnedPx: stored, + renderedPx: rendered, + }); + // Already pinned: the press itself commits nothing... + expect(plan.pinOnDown).toBeNull(); + // ...and cancel restores the stored pin exactly (700 stays 700). + expect(plan.cancelCommit).toBe(700); + expect(plan.cancelCommit).not.toBe(rendered); + }); + + it("auto window: press pins the rendered height; cancel reverts to auto", () => { + const plan = stackDividerHeightPlan({ + storedPinnedPx: undefined, + renderedPx: 300, + }); + expect(plan.pinOnDown).toBe(300); + expect(plan.cancelCommit).toBeUndefined(); + }); +}); + // D34: a collapsed window's height is a DETERMINISTIC calc() over its chrome // (header + bars + dividers) -- the honest numeric endpoint the height // transition eases to, with no DOM measurement. diff --git a/src/viser/client/src/dock/gestures.ts b/src/viser/client/src/dock/gestures.ts index 3570fb4f5..989ca14b2 100644 --- a/src/viser/client/src/dock/gestures.ts +++ b/src/viser/client/src/dock/gestures.ts @@ -72,6 +72,44 @@ export function grabbingCursor(): () => void { }; } +/** The one-gesture mutex (spec §4: "One active gesture at a time; extra + * pointers are ignored"). ONE ref, owned by DockManager, holds the active + * gesture's cleanup: move drags (dragController's armPress / window-drag + * teardown) store theirs directly, and every resize surface routes its + * dragGesture through exclusiveDragGesture below. A press on ANY gesture + * surface while the slot is held is IGNORED -- never queued. Without the + * shared slot a second finger could start a window drag during a live + * resize (or vice versa): both gestures called suppressTextSelection, and + * the first to end re-enabled text selection under the still-live one. */ +export type GestureSlot = { current: (() => void) | null }; + +/** Run dragGesture under the shared single-gesture slot: parks the gesture's + * cancel in the slot for its whole lifetime and releases the slot exactly + * once when the gesture ends (dragGesture's onEnd runs once across release, + * Escape, browser cancel, and unmount-cancel, so the release can't + * double-run). Callers must check `slot.current !== null` BEFORE their own + * pointer-down side effects (ignore-the-press, armPress's semantics); this + * re-checks defensively and starts nothing for an already-held slot. */ +export function exclusiveDragGesture( + slot: GestureSlot, + opts: Parameters[0], +): () => void { + if (slot.current !== null) return () => {}; + const { onEnd } = opts; + const cancel = dragGesture({ + ...opts, + onEnd: (cancelled) => { + slot.current = null; + onEnd?.(cancelled); + }, + }); + // No event can have ended the gesture between dragGesture() returning and + // this assignment (ends are event-driven), so the park always happens + // before any possible release. + slot.current = cancel; + return cancel; +} + /** Run a rAF-throttled drag gesture: capture the pointer on `grip`, record the * latest pointer state via `update(e)` on every move, and apply it via * `flush()` at most once per animation frame. On a real release, a still- diff --git a/src/viser/client/src/dock/hitTest.test.ts b/src/viser/client/src/dock/hitTest.test.ts index 0cf7e698b..4bb81aaf1 100644 --- a/src/viser/client/src/dock/hitTest.test.ts +++ b/src/viser/client/src/dock/hitTest.test.ts @@ -1183,6 +1183,78 @@ describe("P9 (D55): one seam resolves to one result and one line across all its }); }); +// =========================================================================== +// D55 "one seam, one drop" at a side band's OVERHANG past a rail: on +// [rail | a] the 40px region side band overhangs the 36px strip by a few px. +// The band yields to the collapsed cell across its WHOLE run there (not just +// the cell's own pixels), so the overhang resolves -- like both its +// neighbors (the rail's inner sliver, the divider gap) -- to the seam BESIDE +// the rail, never hopping to the region-boundary seam 0 mid-sweep. +// =========================================================================== +describe("D55: side-band overhang past a rail resolves with its neighbors (no hint hop)", () => { + // Left region [rail | a], rendered 300px wide: the rail is a fixed 36px + // strip, then the divider gap, then the expanded column. + const RAIL_W = 36; + const railSpec = colSplit([leaf("s")]); + if (railSpec.kind === "col") railSpec.column.railed = true; + const aSpec = leaf("a"); + const tree = rowSplit([railSpec, aSpec]); + const layout = layoutWith({ left: tree }); + const railLeafId = leafIdsOf(tree)[0]; + const aLeafId = leafIdsOf(tree)[1]; + const A_LEFT = RAIL_W + SPLIT_DIVIDER_PX; + const targets = (): GroupTarget[] => [ + { + groupId: "s", + // Rail cell rect: starts below the ~16px header chrome (D53), extends + // to the strip bottom (last cell). + rect: rect(0, 16, RAIL_W, 784), + stripRect: null, + tabs: [{ paneId: "s.0", rect: rect(0, 40, RAIL_W, 70) }], + ctx: { kind: "docked", nodeId: railLeafId, edge: "left" }, + collapsed: true, + }, + dockedTarget("a", aLeafId, "left", rect(A_LEFT, 0, 300 - A_LEFT, 800)), + ]; + + it("sweeping x across the rail's inner edge: one seam, one hint (no 34.5->0->34.5 hop)", () => { + // x=30..45 spans the rail's inner sliver, the band overhang past the + // rail (x 37..39 -- inside the 40px band but past the 36px strip), the + // divider gap, and a's left side band; y mid-strip. Every sample must + // resolve to the SAME columnInsert seam (index 1, beside the rail) with + // a pixel-identical seam-centered hint. + const first = run(layout, targets(), 30, 400)!; + expect(first.result).toEqual({ + kind: "columnInsert", + edge: "left", + index: 1, + }); + for (let x = 30; x <= 45; x += 1) { + const out = run(layout, targets(), x, 400); + expect(out, `null at x=${x}`).not.toBeNull(); + expect(out!.result, `result changed at x=${x}`).toEqual(first.result); + expect(out!.hint, `hint moved at x=${x}`).toEqual(first.hint); + } + // The one line is region-tall and centered on the rail|a seam. + expect(first.hint.height).toBe(CONTAINER.height); + expect(first.hint.left + first.hint.width / 2).toBeCloseTo( + (RAIL_W + A_LEFT) / 2, + 1, + ); + }); + + it("the band still fires over the rail HEADER run (no cell owns the pointer there)", () => { + // y=8: above the rail cell's top (the header chrome, spec 5.3) -- the + // region band claims those pixels, resolving to seam 0. + const out = run(layout, targets(), 20, 8)!; + expect(out.result).toEqual({ + kind: "columnInsert", + edge: "left", + index: 0, + }); + }); +}); + // A content-sized minimized strip reads top-to-bottom: a thin top/bottom EDGE // band stacks a new cell above/below, the spine-label rows insert at a tab // position, and the + cap (just inside the top edge) merges. The cap must NOT be @@ -1218,25 +1290,63 @@ describe("collapsed-target vertical zones (content-sized strip)", () => { return l; }; - it("the + cap (just inside the top edge) -> add to the group, not split-above", () => { + it("the + cap (just inside the top edge) -> merge into the group, not split-above", () => { const l = baseLayout(); const strip = mkStrip([ { paneId: "p0", rect: rect(left, 124, STRIP, 26) }, { paneId: "p1", rect: rect(left, 152, STRIP, 26) }, ]); const targets: DropTargets = { groups: [strip] }; - // y=115: the + cap, above the first row (at y=124) but past the 6px edge - // band (ends at y=106). It must ADD the panel to the group (insertTab at the - // top), NOT a split-above -- regression: the cap used to be swallowed by the - // "above" split zone, leaving no way to drop INTO a minimized strip. + // y=115: the + cap, above the first row (at y=124, and clear of its 8px + // insert-proximity run starting at y=116) but past the edge band (ends at + // y=108). Spec 5.3: "the rest, cap included: merge into that group" -- so + // the cap MERGES (appends), NOT a split-above (regression: the cap used + // to be swallowed by the "above" split zone, leaving no way to drop INTO + // a minimized strip) and NOT an insertTab (regression: nearest-row + // insertion had no distance bound, making merge unreachable anywhere on + // the strip). const out = hitTest(l, SW, CONTAINER, targets, midX, 115)?.result; expect(out).toMatchObject({ - kind: "insertTab", + kind: "merge", targetGroupId: "s", - index: 0, }); }); + it("near a spine row -> insertTab; far from any row (the cap) -> merge (spec 5.3)", () => { + const l = baseLayout(); + // A taller strip with a taller cap run: rows start at y=140, so the cap + // spans ~[108..132) (past the 8px top edge band, clear of the first + // row's 8px insert-proximity run starting at y=132). + const strip: GroupTarget = { + groupId: "s", + rect: rect(left, 100, STRIP, 120), + stripRect: null, + tabs: [ + { paneId: "p0", rect: rect(left, 140, STRIP, 26) }, + { paneId: "p1", rect: rect(left, 168, STRIP, 26) }, + ], + ctx: { kind: "docked", nodeId: "Ls", edge: "right" }, + collapsed: true, + }; + const targets: DropTargets = { groups: [strip] }; + // Within 8px above the first row: still an insert (index 0). + expect(hitTest(l, SW, CONTAINER, targets, midX, 135)?.result).toMatchObject( + { kind: "insertTab", targetGroupId: "s", index: 0 }, + ); + // Between the rows: insert between them (index 1). + expect(hitTest(l, SW, CONTAINER, targets, midX, 167)?.result).toMatchObject( + { kind: "insertTab", targetGroupId: "s", index: 1 }, + ); + // Over the cap, beyond the proximity bound: merge, per 5.3's "the rest, + // cap included". + for (const y of [112, 120, 128]) { + expect( + hitTest(l, SW, CONTAINER, targets, midX, y)?.result, + `y=${y}`, + ).toMatchObject({ kind: "merge", targetGroupId: "s" }); + } + }); + it("thin top/bottom edges -> split; over a row -> insertTab", () => { const l = baseLayout(); const strip = mkStrip([ @@ -1317,6 +1427,60 @@ describe("floating snap zones", () => { }); }); +// =========================================================================== +// P11/D4 on the bottom split band: the band is 25% of the content height, +// so on a short cell it dips under the 8px zone minimum -- and a sub-minimum +// zone is REMOVED, not shrunk (misfire converter). Short cell -> no bottom +// zone (the body stays merge); tall cell -> zone present. Applies to both +// variants that share the band math: the docked bottom-split and the +// floating snap-below. +// =========================================================================== +describe("P11: sub-8px bottom split band is removed, not shrunk", () => { + it("docked short cell (band would be <8px): bottom edge merges, no split", () => { + // Cell 70px tall: strip [112..142], content [142..172] -> ch=28, + // 25% = 7px < 8px floor -> zone removed. + const tree = leaf("g"); + const layout = layoutWith({ left: tree }); + const tgt = dockedTarget( + "g", + leafIdOf(tree), + "left", + rect(0, 100, 300, 70), + ); + const out = run(layout, [tgt], 150, 170)!; + expect(out.result).toEqual({ kind: "merge", targetGroupId: "g" }); + }); + + it("docked tall cell: the bottom split band is present", () => { + // Cell 442px tall: content [142..542] -> ch=400, band = 100px (capped). + const tree = leaf("g"); + const layout = layoutWith({ left: tree }); + const tgt = dockedTarget( + "g", + leafIdOf(tree), + "left", + rect(0, 100, 300, 442), + ); + const out = run(layout, [tgt], 150, 535)!; + expect(out.result).toMatchObject({ kind: "split", region: "bottom" }); + }); + + it("floating short cell: no snap-below band, bottom edge merges", () => { + const layout = layoutWith({}); + // 70px cell: content [242..270] -> ch=28 -> band removed. + const tgt = floatingTarget("g", "w1", 0, rect(400, 200, 300, 70)); + const out = run(layout, [tgt], 550, 268)!; + expect(out.result).toEqual({ kind: "merge", targetGroupId: "g" }); + }); + + it("floating tall cell: the snap-below band is present (existing behavior)", () => { + const layout = layoutWith({}); + const tgt = floatingTarget("g", "w1", 0, rect(400, 200, 300, 300)); + const out = run(layout, [tgt], 550, 490)!; + expect(out.result).toEqual({ kind: "snap", windowId: "w1", index: 1 }); + }); +}); + // =========================================================================== // Nested dockable AREA targets. An area is a FLAT tab group: the only drops are // insert-at-a-tab-position (over its strip) or merge/append (anywhere else, diff --git a/src/viser/client/src/dock/hitTest.ts b/src/viser/client/src/dock/hitTest.ts index 0bd0607fe..b467b51d1 100644 --- a/src/viser/client/src/dock/hitTest.ts +++ b/src/viser/client/src/dock/hitTest.ts @@ -443,20 +443,46 @@ export function hitTest( }; const skipRegionEdges = overInsertableStrip() || overFloatingTarget(); - // Is the pointer over a collapsed strip cell of the given docked edge? Such a - // cell owns its own (short, content-tall) drop zones -- tab-insert / merge / - // stack -- so the region-wide "dock beside" band must yield to it there. Used - // to let a sole minimized strip's empty region area below/around it still dock - // a full-height sibling column without the band eating the strip's own zones. - const overCollapsedCell = (edge: DockEdge): boolean => { + // Is the pointer over a collapsed strip cell of the given docked edge -- or + // in a side band's OVERHANG past one? Such a cell owns its own (short, + // content-tall) drop zones -- tab-insert / merge / stack -- so the + // region-wide "dock beside" band must yield to it there. Used to let a sole + // minimized strip's empty region area below/around it still dock a + // full-height sibling column without the band eating the strip's own zones. + // + // `bands` are the edge's side-band x-runs (client coords). The yield covers + // a band's WHOLE run wherever it horizontally overlaps a collapsed cell (at + // the pointer's y within that cell), not just the cell's own pixels: a 40px + // band overhanging a 36px rail by a few px otherwise resolved that sliver + // to the region-boundary seam while both its neighbors (the rail's own + // sliver, the divider gap) resolve to the rail's adjacent seam -- one seam, + // one drop (D55), but the hint hopped outward and back across the overhang. + // Yielded, the overhang falls through to the divider-gap recovery / the + // flanking zones, i.e. the same seam as its neighbors. The y-bound keeps + // the rail HEADER run resolving at region level (spec 5.3: no cell owns + // the pointer there). + const overCollapsedCell = ( + edge: DockEdge, + bands: { start: number; end: number }[], + ): boolean => { for (const t of targets.groups) { if ( - t.collapsed === true && - t.ctx.kind === "docked" && - t.ctx.edge === edge && - inside(t.rect, clientX, clientY) + t.collapsed !== true || + t.ctx.kind !== "docked" || + t.ctx.edge !== edge ) - return true; + continue; + if (clientY < t.rect.top || clientY > t.rect.bottom) continue; + if (clientX >= t.rect.left && clientX <= t.rect.right) return true; + for (const b of bands) { + if ( + clientX >= b.start && + clientX <= b.end && + t.rect.left < b.end && + t.rect.right > b.start + ) + return true; + } } return false; }; @@ -557,8 +583,21 @@ export function hitTest( // sliver already docks a column beside it (sweep invariant: every // target reachable). This yield also covers packed regions whole -- // their strips tile the full region, so dock-beside there is entirely - // the rails' own slivers (edge case 13). - if (overCollapsedCell(edge)) continue; + // the rails' own slivers (edge case 13) -- and each band's OVERHANG + // past a collapsed cell it shadows (overCollapsedCell doc: the whole + // overhang resolves with the divider-gap/flanking zones to the cell's + // adjacent seam, D55's one-seam-one-drop). + const bands = [ + { + start: crect.left + regionLeft, + end: crect.left + regionLeft + sideBand, + }, + { + start: crect.left + regionRight - sideBand, + end: crect.left + regionRight, + }, + ]; + if (overCollapsedCell(edge, bands)) continue; // Both bands resolve to the canonical seam (D55): the outer/inner // boundary is seam 0 / seam N of the region's columns, with the one // seam-centered region-tall line (a new column lands beside @@ -1030,6 +1069,19 @@ export function hitTest( ), }; } + // Spec 5.3: "over a spine row -> insert at that tab position; the + // rest, cap included: merge." verticalTabInsertion picks the nearest + // row by Y with no distance bound, so without one every strip pixel + // resolved to insert-around-the-nearest-row and merge (the cap's own + // outcome) was unreachable -- the vertical analog of the bar-path + // bound above. Past the last row the docked branch already routes to + // stack-below (contentBottom); the bound here keeps the cap run above + // the first row (and any tail on a floating strip) a merge. + const first = g.tabs[0]; + const lastRow = g.tabs[g.tabs.length - 1]; + if (first !== undefined && clientY < first.rect.top - 8) return null; + if (lastRow !== undefined && clientY > lastRow.rect.bottom + 8) + return null; const ins = verticalTabInsertion(g.tabs, clientY); return ins === null ? null @@ -1201,8 +1253,18 @@ export function hitTest( // also pixel-capped so the band doesn't balloon on a wide panel (a docked // control panel can be 320-384px wide -- 22% of that reads as "most of the // way in" rather than "near the edge"). + // + // P11/D4: a zone that cannot afford its 8px minimum is REMOVED, not + // shrunk. The band is vBand*ch px in its narrow dimension; on a short + // content area (ch < 32px) the 25% fraction dips under the floor, so the + // zone is dropped entirely (band = 0) and the short body stays merge -- + // mirroring the collapsed branch's edgeBand removal above. Shared by the + // docked bottom-split, the merge-suppressed top band, and the floating + // snap-below band (all read vBand). const vBand = - ch > 0 ? Math.min(SPLIT_BAND_V, SPLIT_BAND_V_MAX_PX / ch) : SPLIT_BAND_V; + ch <= 0 || SPLIT_BAND_V * ch < MINIMIZED_EDGE_BAND_PX + ? 0 + : Math.min(SPLIT_BAND_V, SPLIT_BAND_V_MAX_PX / ch); const hBand = r.width > 0 ? Math.min(SPLIT_BAND, SPLIT_BAND_H_MAX_PX / r.width) diff --git a/src/viser/client/src/dock/layoutOps.ts b/src/viser/client/src/dock/layoutOps.ts index 71d64636d..708951b0f 100644 --- a/src/viser/client/src/dock/layoutOps.ts +++ b/src/viser/client/src/dock/layoutOps.ts @@ -942,6 +942,39 @@ export function cappedWindowHeight( return Math.min(pinnedHeight, Math.max(floor, containerHeight - 8)); } +/** The stack divider's height plan, snapshotted at pointer down (spec §4's + * floating height-divider row + P2). Two distinct heights are in play and + * conflating them was a bug: + * - `renderedPx` -- the paper's on-screen height, i.e. + * cappedWindowHeight(pin, container) when pinned. LIVE DRAG math (the + * auto->pinned seed, push-through budgets) runs on this: it is what the + * user sees and trades. + * - the STORED pin (`storedPinnedPx`, win.height) -- what a CANCEL or + * motionless press must commit. Committing the rendered height instead + * silently rewrote a stored 700px pin to ~container px on a motionless + * press (P2: cancel restores the exact pre-gesture stored state; a + * motionless press is a no-op on stored state). Same rule as the window + * grip's startHeightCommit. An auto window restores to auto. */ +export function stackDividerHeightPlan(opts: { + /** The stored pin (pinnedPxOf(win.height)); undefined for an auto window. */ + storedPinnedPx: number | undefined; + /** The paper's rendered height at drag start (offsetHeight). */ + renderedPx: number; +}): { + /** Pin committed at pointer down for an AUTO window (the rendered seed, so + * entering pinned mode reproduces the on-screen layout); null when already + * pinned -- a press changes nothing. */ + pinOnDown: number | null; + /** What cancel / a motionless press commits: the stored pin (uncapped), + * or undefined to restore auto mode. */ + cancelCommit: number | undefined; +} { + return { + pinOnDown: opts.storedPinnedPx === undefined ? opts.renderedPx : null, + cancelCommit: opts.storedPinnedPx, + }; +} + /** Set a floating window's explicit height (px), switching it from auto-height * to fixed-height with its contents scrolling -- or pass `undefined` to clear * the pin and return it to auto-height (the window tracks its content again). diff --git a/src/viser/client/src/dock/regionResize.test.ts b/src/viser/client/src/dock/regionResize.test.ts new file mode 100644 index 000000000..0240bbd18 --- /dev/null +++ b/src/viser/client/src/dock/regionResize.test.ts @@ -0,0 +1,101 @@ +// Region-resize gesture tests (makeRegionResizeHandlers), driven the way +// RegionResizer drives it: per-frame onFrame(reservedPx) calls, then -- on +// Escape -- a replayed onFrame(startWidth) followed by onEnd(true) (the +// resizer's cancel choreography). +// +// P2 / spec section 4 pin: "Escape ... restores the exact pre-gesture +// layout". pushFloatsAheadOfSeam is stateless and grow-only, so the cancel's +// width-restore frame cannot un-push floats the drag displaced -- and after a +// SHRINK, that restore frame is itself a grow that pushes floats the drag +// never touched. The handlers therefore snapshot pre-gesture float x +// positions and restore them on the cancelled onEnd. + +import { describe, expect, it } from "vitest"; +import { makeRegionResizeHandlers, RegionResizeDeps } from "./regionResize"; +import { leaf, makeLayout } from "./testUtils"; +import { DockLayout } from "./types"; + +function makeDeps(layout: DockLayout, startReserved: number) { + const layoutRef = { current: layout }; + const reservedWidthRef = { current: { left: startReserved, right: 0 } }; + const deps: RegionResizeDeps = { + layoutRef, + containerRef: { current: null }, + containerWidthRef: { current: 1000 }, + containerHeightRef: { current: 800 }, + reservedWidthRef, + regionResizeDraggingRef: { current: false }, + draggingWindowIdRef: { current: null }, + onCommitRef: { current: undefined }, + onRegionResizeFrameRef: { current: undefined }, + applyOp: (next) => { + layoutRef.current = next; + }, + runProgrammatic: (fn) => fn(), + }; + return { deps, layoutRef, reservedWidthRef }; +} + +/** Drive one drag the way RegionResizer does: onFrame per pointer frame, with + * the rendered reserved width (what DockManager's re-render feeds back into + * reservedWidthRef) tracking each committed frame. */ +function driveDrag(startReserved: number, layout: DockLayout) { + const { deps, layoutRef, reservedWidthRef } = makeDeps(layout, startReserved); + const handlers = makeRegionResizeHandlers("left", deps); + const frame = (px: number) => { + handlers.onFrame(px); + reservedWidthRef.current.left = px; + }; + const escape = () => { + // RegionResizer's cancel choreography: replay the start width, then the + // cancelled end. + frame(startReserved); + handlers.onEnd(true); + }; + const release = () => handlers.onEnd(false); + return { frame, escape, release, layoutRef }; +} + +describe("region resize Escape restores pre-gesture float positions (P2)", () => { + it("grow pushes a float; Escape puts it back exactly", () => { + const layout = makeLayout({ + left: leaf("a"), + floating: [{ id: "w", stack: ["f"], x: 400, y: 120, width: 200 }], + }); + const { frame, escape, layoutRef } = driveDrag(300, layout); + frame(600); // grow: the seam sweeps past the float at x=400... + expect(layoutRef.current.floating[0].x).toBe(600); // ...pushing it flush + escape(); + const w = layoutRef.current.floating[0]; + expect(w.x).toBe(400); // exact pre-gesture position + expect(w.y).toBe(120); + }); + + it("shrink-then-Escape leaves an untouched overlapping float unmoved", () => { + // A float OVERLAPPING the region pre-gesture (x=200 under a 300px + // region): the drag never touches it (pushes are grow-only and skip + // already-overlapping floats), so Escape must leave it at x=200. The + // cancel's width-restore frame (150 -> 300, a grow) would otherwise + // push it flush to the restored seam. + const layout = makeLayout({ + left: leaf("a"), + floating: [{ id: "w", stack: ["f"], x: 200, y: 50, width: 150 }], + }); + const { frame, escape, layoutRef } = driveDrag(300, layout); + frame(150); // shrink: sweeps past nothing + expect(layoutRef.current.floating[0].x).toBe(200); + escape(); + expect(layoutRef.current.floating[0].x).toBe(200); + }); + + it("a real release (no cancel) keeps the pushed position", () => { + const layout = makeLayout({ + left: leaf("a"), + floating: [{ id: "w", stack: ["f"], x: 400, y: 120, width: 200 }], + }); + const { frame, release, layoutRef } = driveDrag(300, layout); + frame(600); + release(); // real release: no restore frame, no snapshot restore + expect(layoutRef.current.floating[0].x).toBe(600); + }); +}); diff --git a/src/viser/client/src/dock/regionResize.ts b/src/viser/client/src/dock/regionResize.ts index b7dc1fb47..68fbf83b3 100644 --- a/src/viser/client/src/dock/regionResize.ts +++ b/src/viser/client/src/dock/regionResize.ts @@ -128,6 +128,15 @@ export function makeRegionResizeHandlers( runProgrammatic, } = deps; const layout0 = layoutRef.current; + // Pre-gesture float x positions, for the Escape restore (P2 / spec §4: + // "Escape ... restores the exact pre-gesture layout" -- pushed floats + // included). pushFloatsAheadOfSeam is stateless and grow-only, so the + // cancel's width-restore frame cannot un-push floats the drag displaced -- + // and when the drag had SHRUNK the region, that restore frame is itself a + // grow that pushes floats the drag never touched. `x` is the only float + // field the gesture writes, so `x` is what the snapshot restores (a + // concurrent programmatic move of anything else is left alone, P6). + const floatX0 = new Map(layout0.floating.map((w) => [w.id, w.x])); // Per-frame width writes must land instantly: suppress the D34 width/flex // transitions under the whole dock for the drag's duration // (regionWidthAnim/collapseAnim read the [data-dock-resizing] ancestor), or @@ -135,11 +144,35 @@ export function makeRegionResizeHandlers( containerRef.current?.setAttribute("data-dock-resizing", ""); const onEnd = (cancelled: boolean) => { containerRef.current?.removeAttribute("data-dock-resizing"); - // Skip on cancel (Escape restored the start widths) and on a click without - // motion (no frame committed, layout unchanged). Otherwise fire one - // user-attributed commit spanning the whole drag: layout state is already - // final, this only informs the host's ownership diff. - if (cancelled || layout0 === layoutRef.current) return; + if (cancelled) { + // Escape: put every float back where the gesture found it. Runs AFTER + // the resizer's width-restore frame (RegionResizer replays + // onFrame(startWidth) before calling onEnd(true)), so this is the + // gesture's last write and cannot be re-pushed by that frame. A + // programmatic commit (an Escape-cancel must leave no user-attributed + // trace), like the per-frame commits. + const cur = layoutRef.current; + let changed = false; + const floating = cur.floating.map((w) => { + const x0 = floatX0.get(w.id); + if (x0 === undefined || x0 === w.x) return w; + changed = true; + return { ...w, x: x0 }; + }); + if (changed) { + // Same clone rule as pushFloatsAheadOfSeam: the draft flows into + // applyOp, whose normalize/reconcile steps mutate their input. + const next = ops.cloneLayout(cur); + next.floating = floating; + runProgrammatic(() => applyOp(next)); + } + return; + } + // Skip on a click without motion (no frame committed, layout unchanged). + // Otherwise fire one user-attributed commit spanning the whole drag: + // layout state is already final, this only informs the host's ownership + // diff. + if (layout0 === layoutRef.current) return; onCommitRef.current?.(layout0, layoutRef.current, false); }; const tree0 = layout0.docked[edge]; diff --git a/src/viser/client/src/vendor/instanced-mesh/LOCAL_PATCHES.md b/src/viser/client/src/vendor/instanced-mesh/LOCAL_PATCHES.md index 3406ffbfb..46805a290 100644 --- a/src/viser/client/src/vendor/instanced-mesh/LOCAL_PATCHES.md +++ b/src/viser/client/src/vendor/instanced-mesh/LOCAL_PATCHES.md @@ -31,3 +31,26 @@ lists every divergence from upstream. - **Re-apply check:** confirm upstream still creates `instanceIndex` as a raw `GLInstancedBufferAttribute` (untracked by three). If upstream adds its own disposal for it, this patch may become redundant -- verify before dropping. + +### 2. Make a frame drawn in the dispose->unmount window safe + +- **File:** `core/InstancedMesh2.ts` +- **What:** three additions on top of patch 1's dispose hardening: + `dispose()` nulls `instanceIndex` and deletes the geometry's + `instanceIndex` attribute -- for the main mesh AND every LOD level's own + geometry (LOD levels are separate meshes with per-level geometries); + `onBeforeShadow()` gains the same `!this.instanceIndex` early-return that + `onBeforeRender()` already had. +- **Why:** React commits the swap to a new mesh asynchronously, so a frame can + render the old mesh after `dispose()`. Without the nulling, that frame binds + the freed GL buffer (undefined behavior); without the shadow gate, it + crashes -- the shadow pass runs BEFORE the main pass, `onBeforeShadow` only + guarded its culling call with `instanceIndex &&`, and the unconditional + `instanceIndex.update(...)` below it dereferences null (any shadow-casting + batched mesh under a shadow-mapped light). `onAfterRender`'s re-init branch + then restores a valid attribute for any later frames, at the accepted cost + of one fresh buffer. Covered by `core/InstancedMesh2.dispose.test.ts`. +- **Re-apply check:** if upstream restructures the render/shadow hooks, the + invariant to preserve is "every `instanceIndex` dereference in a hook is + behind a null gate, and dispose leaves no geometry (main or LOD) retaining + the attribute." diff --git a/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.dispose.test.ts b/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.dispose.test.ts index e523c7f9a..3ea7f30bd 100644 --- a/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.dispose.test.ts +++ b/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.dispose.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect } from "vitest"; import * as THREE from "three"; import { InstancedMesh2 } from "./InstancedMesh2.js"; +// The feature modules (Instances, LOD, Capacity, ...) register their +// prototype extensions (addInstances / addLOD / ...) at import time; the +// package index imports them all, exactly as production code does. +import "../index.js"; // Minimal WebGL2 mock that only implements the buffer calls // GLInstancedBufferAttribute touches, plus counters so we can assert that the @@ -22,6 +26,7 @@ function makeMockGl() { }, bindBuffer() {}, bufferData() {}, + bufferSubData() {}, deleteBuffer(buffer: object) { deleted++; live.delete(buffer); @@ -53,6 +58,130 @@ describe("InstancedMesh2 GPU buffer disposal", () => { expect(stats().live).toBe(0); }); + it("nulls instanceIndex on dispose so a late frame re-inits instead of binding the deleted buffer", () => { + const { gl, stats } = makeMockGl(); + // onAfterRender's unpatchMaterial touches renderer.properties, so the mock + // needs the object to exist (its `get` is only restored, never called). + const renderer = { + getContext: () => gl, + properties: { get: undefined }, + } as unknown as THREE.WebGLRenderer; + + const mesh = new InstancedMesh2( + new THREE.BoxGeometry(), + new THREE.MeshBasicMaterial(), + { capacity: 128, renderer }, + ); + expect(mesh.instanceIndex).not.toBeNull(); + + mesh.dispose(); + + // The reference must be dropped along with the GL buffer: onBeforeRender / + // onAfterRender gate on falsiness, so a dangling reference would bind a + // deleted buffer on a frame drawn before React commits the unmount -- and + // permanently skip the re-init branch. + expect(mesh.instanceIndex).toBeNull(); + // The geometry must not retain the deleted buffer either. + expect(mesh.geometry.getAttribute("instanceIndex")).toBeUndefined(); + + // A post-dispose render takes the initIndexAttribute re-init branch, + // allocating a fresh GL buffer and re-registering the geometry attribute. + mesh.onAfterRender( + renderer, + new THREE.Scene(), + new THREE.PerspectiveCamera(), + mesh.geometry, + new THREE.MeshBasicMaterial(), + null, + ); + expect(mesh.instanceIndex).not.toBeNull(); + expect(mesh.geometry.getAttribute("instanceIndex")).toBeDefined(); + expect(stats().created).toBe(2); // initial + re-init + expect(stats().live).toBe(1); // only the re-init buffer survives + }); + + it("survives a post-dispose shadow pass (shadow maps render BEFORE the main pass, so onBeforeShadow is the first hook a late frame hits)", () => { + const { gl } = makeMockGl(); + // onBeforeShadow runs patchMaterial + updateTextures before the gate under + // test, so the renderer mock needs the properties/initTexture surface those + // touch. patchMaterial swaps properties.get wholesale; updateTextures takes + // the initTexture branch when the (empty) texture properties record has no + // __webglTexture. + const renderer = { + getContext: () => gl, + properties: { get: () => ({}) }, + initTexture: () => {}, + capabilities: { maxTextures: 16 }, + info: { render: { frame: 1 } }, + } as unknown as THREE.WebGLRenderer; + + const mesh = new InstancedMesh2( + new THREE.BoxGeometry(), + new THREE.MeshBasicMaterial(), + { capacity: 128, renderer }, + ); + // A live instance keeps `count` positive: pre-fix, the disposed-mesh crash + // needed count > 0 to get past the early return and dereference the nulled + // instanceIndex. + mesh.addInstances(1, () => {}); + + const shadowArgs = [ + renderer, + new THREE.Scene(), + new THREE.PerspectiveCamera(), + new THREE.PerspectiveCamera(), + mesh.geometry, + new THREE.MeshDepthMaterial(), + null, + ] as const; + + // Sanity: a live mesh's shadow hook runs without throwing. Pair it with + // onAfterShadow exactly like a real shadow pass does -- it restores the + // properties.get that patchMaterial swapped in, without which the second + // patch would wrap its own callback and recurse. + expect(() => mesh.onBeforeShadow(...shadowArgs)).not.toThrow(); + mesh.onAfterShadow(...shadowArgs); + + mesh.dispose(); + + // The late frame's shadow pass must be a no-op, not a TypeError: culling + // is skipped (instanceIndex null) so `count` keeps its stale positive + // value, and pre-fix the unconditional instanceIndex.update() crashed the + // frame inside WebGLShadowMap.render. + expect(() => mesh.onBeforeShadow(...shadowArgs)).not.toThrow(); + }); + + it("clears every LOD level's own geometry on dispose (LOD levels are separate meshes with per-level geometries)", () => { + const { gl, stats } = makeMockGl(); + const renderer = { getContext: () => gl } as unknown as THREE.WebGLRenderer; + + const mesh = new InstancedMesh2( + new THREE.BoxGeometry(), + new THREE.MeshBasicMaterial(), + { capacity: 64, renderer }, + ); + mesh.addLOD( + new THREE.BoxGeometry(0.5, 0.5, 0.5), + new THREE.MeshBasicMaterial(), + 10, + ); + + const child = mesh.LODinfo!.objects.find((obj) => obj !== mesh)!; + expect(child).toBeDefined(); + expect(child.geometry.getAttribute("instanceIndex")).toBeDefined(); + + mesh.dispose(); + + // Both the child's buffer and its geometry's retained attribute must go: + // a late frame drawing the LOD level binds through the CHILD's geometry, + // so leaving the attribute there re-binds the freed buffer even though + // the reference on the mesh was nulled. + expect(child.instanceIndex).toBeNull(); + expect(child.geometry.getAttribute("instanceIndex")).toBeUndefined(); + expect(mesh.geometry.getAttribute("instanceIndex")).toBeUndefined(); + expect(stats().live).toBe(0); + }); + it("does not leak instanceIndex buffers across repeated create/dispose", () => { const { gl, stats } = makeMockGl(); const renderer = { getContext: () => gl } as unknown as THREE.WebGLRenderer; diff --git a/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.ts b/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.ts index fe2ef86e8..d0f85bf03 100644 --- a/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.ts +++ b/src/viser/client/src/vendor/instanced-mesh/core/InstancedMesh2.ts @@ -359,6 +359,16 @@ export class InstancedMesh2< this.performFrustumCulling(shadowCamera, camera); } + // === VISER LOCAL PATCH (see vendor/instanced-mesh/LOCAL_PATCHES.md) === + // Disposed mesh (instanceIndex nulled): skip, mirroring onBeforeRender's + // null gate. The shadow pass runs BEFORE the main pass in a frame, so a + // frame drawn in the dispose->unmount window reaches this hook before + // onAfterRender's re-init branch can restore the attribute; without this + // gate the culling short-circuit above leaves `count` at its stale + // positive value and the update() below dereferences null. + if (!this.instanceIndex) return; + // === END VISER LOCAL PATCH === + if (this.count === 0) return; this.instanceIndex.update(this._renderer, this.count); @@ -1078,9 +1088,26 @@ export class InstancedMesh2< const gl = this._renderer.getContext() as WebGL2RenderingContext; this.instanceIndex?.dispose(gl); if (this.LODinfo) { - for (const obj of this.LODinfo.objects) obj.instanceIndex?.dispose(gl); + for (const obj of this.LODinfo.objects) { + obj.instanceIndex?.dispose(gl); + obj.instanceIndex = null; + // LOD levels are separate meshes with their OWN geometries (addLOD + // takes a per-level geometry; `objects` includes `this`, a harmless + // repeat of the deleteAttribute below): each geometry must drop its + // retained instanceIndex attribute too, or a late frame drawing + // that level binds the freed buffer through the child's geometry. + obj._geometry?.deleteAttribute("instanceIndex"); + } } } + // Null the reference (and the geometry's copy of it) after freeing the GL + // buffer: onBeforeRender/onAfterRender gate on `this.instanceIndex` + // falsiness, so a frame drawn between dispose() and React's unmount commit + // would otherwise bind the deleted buffer -- and never take the + // initIndexAttribute re-init branch. initIndexAttribute re-creates both + // the attribute and the geometry entry, so this doesn't break re-init. + this.instanceIndex = null; + this._geometry?.deleteAttribute("instanceIndex"); // === END VISER LOCAL PATCH === } diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 9bcf4ef07..9336f46d4 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -4,11 +4,6 @@ import os import shutil -import subprocess -import tempfile -import time -import urllib.error -import urllib.request from pathlib import Path from typing import Generator @@ -18,6 +13,7 @@ import viser import viser._client_autobuild +from . import vite_manager from .utils import find_free_port, wait_for_connection, wait_for_server_ready TEST_RESULTS_DIR = Path(__file__).resolve().parent.parent.parent / "test-results" @@ -61,8 +57,6 @@ def pytest_configure(config: pytest.Config) -> None: the CLI) to re-enable ``retain-on-failure`` capture when debugging a flaky or hard-to-reproduce failure. """ - import os - if not os.environ.get("VISER_E2E_CAPTURE"): return option = config.option @@ -194,45 +188,14 @@ def viser_page( # server (HMR off, serving the client SOURCE at /dock_test.html) per session/ # xdist worker, instead of one per module: dock tests don't touch the prebuilt # client bundle, and a shared server cuts repeated ~5s startups. +# +# The server process itself lives in vite_manager.ViteServerManager so that +# dock_helpers.open_playground can RESTART it mid-session: on CI the dev +# server has twice wedged permanently partway through a run (HTML still +# served, module graph never completing -- see vite_manager's module +# docstring), and a restart is the only recovery. # --------------------------------------------------------------------------- -DOCK_CLIENT_DIR = Path(__file__).resolve().parents[2] / "src" / "viser" / "client" - -# Wrap the client's vite config with HMR disabled: the playground tests drive -# deterministic DOM interactions, and an HMR websocket reconnect mid-test can -# reload the page under the pointer. Also disable file watching entirely (the -# tests never edit source mid-run) -- vite otherwise recursively watches the -# whole client dir, including the huge `.nodeenv`/`node_modules` trees, which can -# exhaust the OS inotify watcher limit and crash the dev server on startup -# (ENOSPC). usePolling:false + ignored globs keeps the server lightweight. -_DOCK_HMR_OFF_CONFIG = """\ -import base from "./vite.config.mts"; -export default async (env) => { - const resolved = typeof base === "function" ? await base(env) : base; - return { - ...resolved, - server: { - ...(resolved.server || {}), - hmr: false, - watch: { ignored: ["**/.nodeenv/**", "**/node_modules/**"] }, - }, - }; -}; -""" - - -def _wait_for_http(port: int, path: str, timeout: float = 30.0) -> None: - deadline = time.monotonic() + timeout - url = f"http://localhost:{port}{path}" - last_err: Exception | None = None - while time.monotonic() < deadline: - try: - with urllib.request.urlopen(url, timeout=1.0) as resp: - if resp.status == 200: - return - except (urllib.error.URLError, ConnectionError, OSError) as err: - last_err = err - time.sleep(0.1) - raise RuntimeError(f"Vite dev server not ready at {url}: {last_err}") +DOCK_CLIENT_DIR = vite_manager.DOCK_CLIENT_DIR @pytest.fixture(scope="session") @@ -243,30 +206,12 @@ def vite_server() -> Generator[int, None, None]: if not (DOCK_CLIENT_DIR / "node_modules").exists(): pytest.skip("client node_modules missing; run `npm install` in the client dir") - cfg_fd, cfg_name = tempfile.mkstemp( - prefix="vite.e2e.hmroff.", suffix=".mts", dir=str(DOCK_CLIENT_DIR) - ) - cfg_path = Path(cfg_name) - with os.fdopen(cfg_fd, "w") as f: - f.write(_DOCK_HMR_OFF_CONFIG) - - port = find_free_port() - proc = subprocess.Popen( - ["npx", "vite", "--config", str(cfg_path), "--port", str(port), "--strictPort"], - cwd=str(DOCK_CLIENT_DIR), - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - ) + manager = vite_manager.ViteServerManager(find_free_port()) try: - _wait_for_http(port, "/dock_test.html") - yield port + manager.start() + yield manager.port finally: - proc.terminate() - try: - proc.wait(timeout=10) - except subprocess.TimeoutExpired: - proc.kill() - cfg_path.unlink(missing_ok=True) + manager.close() @pytest.fixture(scope="module") diff --git a/tests/e2e/dock_helpers.py b/tests/e2e/dock_helpers.py index c41658f55..321413923 100644 --- a/tests/e2e/dock_helpers.py +++ b/tests/e2e/dock_helpers.py @@ -15,6 +15,8 @@ from playwright.sync_api import Error as PlaywrightError from playwright.sync_api import Page +from .vite_manager import restart_for_port + PLAYGROUND_PATH = "/dock_test.html" # Page-open readiness is a liveness wait, not an assertion: a generous timeout @@ -38,9 +40,31 @@ def open_playground(dock_context, port: int, w: int = 1280, h: int = 800) -> Pag Retries once on a fresh page: waiting longer cannot rescue a page whose module-graph fetch got stuck, but a second page reuses the context's warm HTTP cache, so the retry is cheap. + + If BOTH attempts fail, the worker's shared Vite dev server is restarted + and one final attempt runs against the fresh server. Two consecutive + boot failures are the signature of the server itself being wedged (CI + 2026-07-27 and 2026-07-29: goto kept succeeding but the module graph + never completed, permanently, with esbuild children hung -- so every + later playground test on the worker burned the full timeout and failed). + A page-level retry cannot recover from that; a server restart does. """ last_err: Exception | None = None - for _ in range(2): + for attempt in range(3): + if attempt == 2: + # Third attempt exists only to run against a freshly restarted + # server; without a managed server to restart it would just + # repeat the same failure against the same wedged process. If the + # restart itself fails, surface the PAGE symptom (the diagnostic + # that matters) with the restart failure chained as context, + # rather than letting the restart error replace it. + try: + restarted = restart_for_port(port) + except Exception as restart_err: + assert last_err is not None + raise last_err from restart_err + if not restarted: + break pg = dock_context.new_page() pg.set_viewport_size({"width": w, "height": h}) try: diff --git a/tests/e2e/test_mobile_breakpoint.py b/tests/e2e/test_mobile_breakpoint.py new file mode 100644 index 000000000..64ca72560 --- /dev/null +++ b/tests/e2e/test_mobile_breakpoint.py @@ -0,0 +1,210 @@ +"""E2E regression tests for the mobile (<=576px-wide) breakpoint. + +Two bugs fixed in the v1.0.30 regression audit are pinned here: + +1. Mobile canvas remount (App.tsx): ``useMediaQuery`` used to read the mobile + media query in an effect, so a mobile-width load rendered desktop-first, + mounted the canvas inside the dock surface, then flipped on the next render + -- remounting the entire canvas subtree and creating (then throwing away) a + WebGL context. The fix reads matchMedia synchronously on first render, so a + mobile load creates exactly as many WebGL contexts as a desktop load and + never tears a canvas element down. An init script installed before any page + JS counts WebGL context acquisitions AND canvas-element removals; both are + compared against a desktop-load self-baseline instead of hardcoded values. + (The removal probe is what catches the remount deterministically: in fast + headless runs the desktop-first canvas is destroyed before R3F's async GL + init, so the throwaway context -- the extra count -- may never material- + ize, while the create-then-destroy of the canvas element always does.) + +2. Mobile bottom-sheet panel updates (ControlPanel.tsx / PanelsFallback): the + panel-list subscription used key-set equality, but ``updatePanel`` replaces + the panel object for the updated uuid (same key set), so server-side + visible/order updates never re-rendered the sheet. The fix subscribes with + value-level equality; we assert a server-side ``visible`` toggle actually + hides (and re-shows) a panel's section in the sheet. + +Both tests drive real mobile-sized browser contexts (the conftest ``page`` +fixture is desktop-sized, so contexts are created per test here). +""" + +from __future__ import annotations + +from playwright.sync_api import Browser, BrowserContext, Page, expect + +import viser + +from .utils import wait_for_connection + +# The suite's standard desktop viewport (conftest._E2E_VIEWPORT), used for the +# self-baseline load; comfortably above the 576px (36em) mobile breakpoint. +_DESKTOP_VIEWPORT = {"width": 960, "height": 600} +# iPhone-ish portrait viewport; what matters is width <= 576px so the client's +# `(max-width: ${theme.breakpoints.xs})` query matches. +_MOBILE_VIEWPORT = {"width": 390, "height": 844} + +# Installed via context.add_init_script, so it runs before any page JS. +# +# Two probes, both read against a desktop-load self-baseline: +# +# * __webglContextCount: patches getContext to count each canvas that +# successfully acquires a webgl/webgl2 context AT MOST ONCE (repeat +# getContext calls on the same canvas return the same context and must not +# inflate the count); 2D contexts are ignored. When the throwaway canvas +# lives long enough to initialize GL (real GPUs, slow runners), the pre-fix +# remount shows up as one extra count. +# * __canvasRemovals: a MutationObserver counting elements removed +# from the document. The pre-fix desktop-first render mounted the canvas +# subtree and REMOVED it when the media-query effect flipped to mobile -- +# observable even when the flip wins the race against R3F's async GL init +# (fast headless runs), where the context count alone would miss it. The +# observer targets `document` (documentElement does not exist yet at +# init-script time). +_WEBGL_COUNT_INIT = """ +(() => { + window.__webglContextCount = 0; + window.__canvasRemovals = 0; + const counted = new WeakSet(); + const orig = HTMLCanvasElement.prototype.getContext; + HTMLCanvasElement.prototype.getContext = function (type, ...args) { + const ctx = orig.call(this, type, ...args); + if ( + ctx !== null && + (type === "webgl" || type === "webgl2" || type === "experimental-webgl") && + !counted.has(this) + ) { + counted.add(this); + window.__webglContextCount += 1; + } + return ctx; + }; + const countCanvases = (node) => { + if (!(node instanceof Element)) return 0; + return ( + (node instanceof HTMLCanvasElement ? 1 : 0) + + node.querySelectorAll("canvas").length + ); + }; + const observer = new MutationObserver((mutations) => { + for (const m of mutations) { + for (const removed of m.removedNodes) { + window.__canvasRemovals += countCanvases(removed); + } + } + }); + observer.observe(document, { childList: true, subtree: true }); +})(); +""" + + +def _load_and_measure_canvas_churn( + browser: Browser, port: int, viewport: dict +) -> tuple[int, int]: + """Load the client at `viewport` in a fresh context (with the probing + init script installed) and return ``(webgl_context_count, + canvas_removals)`` observed during startup.""" + context = browser.new_context( + viewport=viewport, device_scale_factor=1, reduced_motion="reduce" + ) + try: + context.add_init_script(_WEBGL_COUNT_INIT) + page = context.new_page() + wait_for_connection(page, port) + # At least the live scene canvas must have acquired a context. + page.wait_for_function("() => window.__webglContextCount >= 1", timeout=15_000) + # The pre-fix remount happened on the render right after the + # media-query effect (a frame or two after first mount), so give the + # probes a generous settle window before reading them -- a remount + # inside this window lands as an extra count / removal. + page.wait_for_timeout(800) + return ( + page.evaluate("() => window.__webglContextCount"), + page.evaluate("() => window.__canvasRemovals"), + ) + finally: + context.close() + + +def test_mobile_load_does_not_remount_canvas( + browser: Browser, viser_server: viser.ViserServer +) -> None: + """A mobile-width load must not create-then-destroy the canvas subtree: + no extra WebGL context and no canvas removals beyond what a desktop load + shows (pre-fix: the desktop-first render mounted the canvases inside the + dock surface and remounted them when the media-query effect flipped).""" + port = viser_server.get_port() + + # Self-baseline: what a desktop load does (measured, not hardcoded -- + # capability probes may legitimately touch extra canvases). + desktop_count, desktop_removals = _load_and_measure_canvas_churn( + browser, port, _DESKTOP_VIEWPORT + ) + assert desktop_count >= 1 + + mobile_count, mobile_removals = _load_and_measure_canvas_churn( + browser, port, _MOBILE_VIEWPORT + ) + + # No canvas element may be torn down during a mobile load (the remount is + # observable as removals even when it wins the race against R3F's async + # GL init, in which case the thrown-away canvas never acquired a context). + assert mobile_removals == desktop_removals, ( + f"mobile-width load removed {mobile_removals} canvas element(s) vs " + f"{desktop_removals} on desktop -- the canvas subtree was mounted " + "desktop-first and remounted for mobile" + ) + # And when the throwaway canvas DID live long enough to initialize GL, + # it shows up as an extra context creation. + assert mobile_count == desktop_count, ( + f"mobile-width load created {mobile_count} WebGL context(s) vs " + f"{desktop_count} on desktop -- an extra context means a " + "created-then-destroyed canvas" + ) + + +def _mobile_context(browser: Browser) -> BrowserContext: + return browser.new_context( + viewport=_MOBILE_VIEWPORT, device_scale_factor=1, reduced_motion="reduce" + ) + + +def _section(page: Page, label: str): + """A collapsed panel section row in the mobile bottom sheet + (MobilePanelSection renders the collapsed header as a role=button with an + 'Expand panel