From e3507e01f939440d38515437e02da08b195c61f2 Mon Sep 17 00:00:00 2001 From: Alexander Rulkens Date: Fri, 8 May 2026 01:31:58 +0200 Subject: [PATCH 1/2] feat(engine): tweenToGalaxy helper Extract the cloud-row -> camera tween dispatch shared by selectFamous, selectByAlias, and focusOn into one helper. Cam-null guard included defensively for the post-destroy / pre-bootstrap race window. Co-Authored-By: Claude Opus 4.7 --- src/services/engine/tweenToGalaxy.ts | 138 ++++++++++++++++++++ tests/services/engine/tweenToGalaxy.test.ts | 122 +++++++++++++++++ 2 files changed, 260 insertions(+) create mode 100644 src/services/engine/tweenToGalaxy.ts create mode 100644 tests/services/engine/tweenToGalaxy.test.ts diff --git a/src/services/engine/tweenToGalaxy.ts b/src/services/engine/tweenToGalaxy.ts new file mode 100644 index 0000000..77ee51b --- /dev/null +++ b/src/services/engine/tweenToGalaxy.ts @@ -0,0 +1,138 @@ +/** + * tweenToGalaxy — kick off a focus camera tween toward a galaxy. + * + * ### Why a helper + * + * Three public-handle methods on `EngineHandle` — `focusOn`, `selectFamous`, + * and `selectByAlias` — each carried the same five-line block: + * + * ```ts + * state.subsystems.tweens.start({ + * startMs: performance.now(), + * durationMs: FOCUS_TWEEN_MS, + * fromTarget: vec3.clone(cam.target as vec3), + * toTarget: vec3.fromValues(info.x, info.y, info.z), + * fromDistance: cam.distance, + * toDistance: focusDistanceMpc(info.diameterKpc), + * fromYaw: cam.yaw, + * toYaw: cam.yaw, + * fromPitch: cam.pitch, + * toPitch: cam.pitch, + * }); + * state.subsystems.scheduler.requestRender(); + * ``` + * + * Three near-identical bodies — exactly the kind of copy-paste that + * silently rots when one site adds a new field (e.g. an FOV transition) + * and the others lag behind. One helper, three call sites collapse to + * a single line each. + * + * ### Why we DON'T extend the responsibility + * + * Each call site has its own pre-tween bookkeeping that doesn't belong + * in the helper: + * - `selectFamous` / `selectByAlias` resolve a `PointInfo` via + * `buildPointInfo`, then call `setSelected` and `cb.onFocusChange` + * before tweening; + * - `focusOn` calls `cb.onFocusChange` first so the URL hash updates + * in lock-step with the user's commitment. + * + * Pulling that work into `tweenToGalaxy` would force the helper to know + * about callbacks, selection state, and source enums — turning a + * five-line dispatcher into a multi-purpose coordinator. The single + * responsibility we DO want is "given a camera-able target, start the + * tween" — keep it tiny. + * + * ### Why the cam-null guard is here + * + * `state.cam` is typed `OrbitCamera | null` because two reachable + * windows leave it null: + * - **Pre-bootstrap**: `createOrbitCamera` runs inside `wireInput` + * during the bootstrap IIFE, after `initGpu` and the first cloud + * arrival. Code that fires before then (e.g. an unlikely + * `selectByAlias` from a deep-link drain that races the very first + * cloud upload) still finds `cam` null. + * - **Post-destroy**: `handle.destroy()` detaches controls and clears + * `state.cam = null`. An in-flight focus promise that resolves + * after destroy must not crash the engine on shutdown. + * + * The three call sites all check for null themselves today — the helper + * absorbs that check so future call sites get the safe behaviour for + * free. It is genuinely needed; do not remove on the grounds of YAGNI. + * + * ### Why `TweenTarget` is a structural minimum, not `PointInfo` + * + * The helper only reads four fields off the target: `x`, `y`, `z`, and + * `diameterKpc`. Declaring the parameter as `PointInfo` would imply + * the helper might reach for ra/dec/redshift/etc., which it never does. + * The minimum-surface type doubles as documentation: "this is exactly + * the data the tween needs." Production callers pass a full + * `PointInfo` and TypeScript accepts it via structural compatibility. + */ + +import { vec3 } from 'gl-matrix'; + +import type { EngineState } from '../../@types'; +import { FOCUS_TWEEN_MS, focusDistanceMpc } from './focusTween'; + +/** + * The minimum-surface descriptor the helper actually consumes. + * + * Every existing call site already has a `PointInfo` in scope (built + * via `buildPointInfo`), and `PointInfo` is structurally a superset of + * this type — so passing `info` directly works without any field + * extraction at the call site. + */ +export type TweenTarget = { + /** World-space X in Mpc. */ + x: number; + /** World-space Y in Mpc. */ + y: number; + /** World-space Z in Mpc. */ + z: number; + /** + * Physical galaxy diameter in kpc — drives the focus distance via + * `focusDistanceMpc`. Callers that genuinely lack a diameter (none + * today) should pass the project-wide fallback explicitly rather + * than letting NaN through. + */ + diameterKpc: number; +}; + +/** + * Start a focus tween toward `target`, snapshotting the current camera + * pose so an in-flight tween hands off smoothly to the new one. + * + * Yaw and pitch are preserved — the user keeps their orientation; only + * the orbit target and distance change. The duration is the + * project-wide `FOCUS_TWEEN_MS`; the destination distance is derived + * from `target.diameterKpc` via `focusDistanceMpc` (which clamps to a + * sensible minimum so dwarfs don't end up framing the camera inside + * the disk). + * + * No-op when `state.cam` is null — see the module header for the two + * windows where that happens. + */ +export function tweenToGalaxy(state: EngineState, target: TweenTarget): void { + const cam = state.cam; + if (!cam) return; + + state.subsystems.tweens.start({ + startMs: performance.now(), + durationMs: FOCUS_TWEEN_MS, + // vec3.clone copies the target tuple so later mutation of + // cam.target (the next frame's orbit-controls update, an + // interrupting tween, …) doesn't corrupt the from-snapshot. + fromTarget: vec3.clone(cam.target as vec3), + toTarget: vec3.fromValues(target.x, target.y, target.z), + fromDistance: cam.distance, + toDistance: focusDistanceMpc(target.diameterKpc), + fromYaw: cam.yaw, + toYaw: cam.yaw, + fromPitch: cam.pitch, + toPitch: cam.pitch, + }); + // Wake the render loop — the tween's per-frame advance keeps it + // ticking via the still-animating predicate until completion. + state.subsystems.scheduler.requestRender(); +} diff --git a/tests/services/engine/tweenToGalaxy.test.ts b/tests/services/engine/tweenToGalaxy.test.ts new file mode 100644 index 0000000..34a9f36 --- /dev/null +++ b/tests/services/engine/tweenToGalaxy.test.ts @@ -0,0 +1,122 @@ +/** + * tweenToGalaxy — unit tests for the camera-tween-to-galaxy helper. + * + * Three public-handle methods (`focusOn`, `selectFamous`, `selectByAlias`) + * each open-coded the same five-line "build a CameraTween from a galaxy's + * world-space position + diameter, hand it to the tween manager, kick the + * scheduler" block. `tweenToGalaxy` reifies that block as a single + * helper. These tests exercise it without spinning up the full engine: + * + * - happy path: the tween descriptor lands in `tweens.start` with + * to-target = (info.x, info.y, info.z), to-distance derived from + * diameterKpc, and from-* snapshots cloned off the live camera so + * interrupting an in-flight tween hands off smoothly; + * - cam-null guard: when the engine has been destroyed (or is still + * bootstrapping pre-`startLoop`) `state.cam` is null — the helper + * must short-circuit silently rather than dereference null. + * + * We intentionally do NOT re-test `focusDistanceMpc` here — its own test + * suite in `focusTween.test.ts` covers the diameter-to-distance math. + * This test only verifies the *plumbing* between the helper and the + * tween manager. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { tweenToGalaxy } from '../../../src/services/engine/tweenToGalaxy'; +import { focusDistanceMpc } from '../../../src/services/engine/focusTween'; +import type { EngineState } from '../../../src/@types'; + +/** + * Build a minimal `EngineState`-shaped fixture that exposes only the + * fields `tweenToGalaxy` reads: `cam`, `subsystems.tweens.start`, and + * `subsystems.scheduler.requestRender`. Casting through `unknown` keeps + * the test honest — if the helper ever reaches for a field outside this + * trio, the test will surface it as a runtime undefined rather than a + * silently-passing stub. + */ +function makeState(opts: { + cam: { target: [number, number, number]; distance: number; yaw: number; pitch: number } | null; + start: ReturnType; + requestRender: ReturnType; +}): EngineState { + return { + cam: opts.cam, + subsystems: { + tweens: { start: opts.start }, + scheduler: { requestRender: opts.requestRender }, + }, + } as unknown as EngineState; +} + +describe('tweenToGalaxy', () => { + it('starts a CameraTween toward (info.x, info.y, info.z) with focusDistanceMpc(diameterKpc) and requests a render', () => { + const start = vi.fn(); + const requestRender = vi.fn(); + const cam = { + target: [1, 2, 3] as [number, number, number], + distance: 50, + yaw: 0.25, + pitch: -0.1, + }; + const state = makeState({ cam, start, requestRender }); + + tweenToGalaxy(state, { x: 100, y: 200, z: 300, diameterKpc: 25 }); + + expect(start).toHaveBeenCalledOnce(); + const tween = start.mock.calls[0]![0]; + // toTarget is whatever vec3.fromValues produced — a 3-element array-like + // whose contents must equal (100, 200, 300). + expect(Array.from(tween.toTarget as ArrayLike)).toEqual([100, 200, 300]); + expect(tween.toDistance).toBe(focusDistanceMpc(25)); + // from-* snapshots come straight off the live camera. + expect(tween.fromDistance).toBe(50); + expect(tween.fromYaw).toBe(0.25); + expect(tween.fromPitch).toBe(-0.1); + // Yaw / pitch are preserved — the tween only moves target + distance. + expect(tween.toYaw).toBe(cam.yaw); + expect(tween.toPitch).toBe(cam.pitch); + // Duration is the project-wide focus tween length (sourced from + // focusTween.ts so this assertion stays honest if the constant moves). + expect(typeof tween.durationMs).toBe('number'); + expect(tween.durationMs).toBeGreaterThan(0); + // startMs is `performance.now()`-shaped — finite, non-negative. + expect(Number.isFinite(tween.startMs)).toBe(true); + expect(tween.startMs).toBeGreaterThanOrEqual(0); + + expect(requestRender).toHaveBeenCalledOnce(); + }); + + it('clones cam.target so later mutation of cam.target does not corrupt the tween snapshot', () => { + const start = vi.fn(); + const requestRender = vi.fn(); + const cam = { + target: [1, 2, 3] as [number, number, number], + distance: 10, + yaw: 0, + pitch: 0, + }; + const state = makeState({ cam, start, requestRender }); + + tweenToGalaxy(state, { x: 0, y: 0, z: 0, diameterKpc: 30 }); + + const captured = start.mock.calls[0]![0].fromTarget as ArrayLike; + // Mutate the live camera target after the tween was started. + cam.target[0] = 999; + cam.target[1] = 999; + cam.target[2] = 999; + // The captured snapshot must be unchanged. + expect(Array.from(captured)).toEqual([1, 2, 3]); + }); + + it('is a no-op when state.cam is null (post-destroy / pre-startLoop race window)', () => { + const start = vi.fn(); + const requestRender = vi.fn(); + const state = makeState({ cam: null, start, requestRender }); + + tweenToGalaxy(state, { x: 100, y: 200, z: 300, diameterKpc: 25 }); + + expect(start).not.toHaveBeenCalled(); + expect(requestRender).not.toHaveBeenCalled(); + }); +}); From 50836c5e2401dee43ad09a19b327526c4cf1a80f Mon Sep 17 00:00:00 2001 From: Alexander Rulkens Date: Fri, 8 May 2026 01:35:29 +0200 Subject: [PATCH 2/2] refactor(engine): consume tweenToGalaxy in selectFamous/selectByAlias/focusOn Three duplicated cloud-row -> camera tween dispatches collapse to one helper call each. ~50 lines removed from engine.ts; focusOnHome keeps its bespoke tween (different toTarget / yaw / pitch shape). The cam-null guard stays in focusOn ahead of cb.onFocusChange so the URL-sync hook doesn't fire during the pre-bootstrap window; the helper's own guard absorbs the no-op for selectFamous and selectByAlias where the callback already fired. Co-Authored-By: Claude Opus 4.7 --- src/services/engine/engine.ts | 93 +++++++++++------------------------ 1 file changed, 30 insertions(+), 63 deletions(-) diff --git a/src/services/engine/engine.ts b/src/services/engine/engine.ts index e045ded..9b86735 100644 --- a/src/services/engine/engine.ts +++ b/src/services/engine/engine.ts @@ -113,7 +113,8 @@ import { filamentFetcher } from '../loading/fetchers/filamentFetcher'; import { famousMetaFetcher } from '../loading/fetchers/famousMetaFetcher'; import { pgcAliasFetcher, type PgcAliasMap } from '../loading/fetchers/pgcAliasFetcher'; import { TIER_TARGETS } from '../../data/tierTargets'; -import { FOCUS_TWEEN_MS, focusDistanceMpc } from './focusTween'; +import { FOCUS_TWEEN_MS } from './focusTween'; +import { tweenToGalaxy } from './tweenToGalaxy'; // ── Galaxy thumbnail subsystem ──────────────────────────────────────────── // @@ -1891,8 +1892,13 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En }, focusOn(info) { - // Camera may not be ready yet (cloud still loading); drop the call. - // Same defensive pattern as resetCamera() above. + // Camera may not be ready yet (cloud still loading); drop the + // call. This guard is *separate* from `tweenToGalaxy`'s own + // cam-null guard — we need it here to gate the + // `onFocusChange` callback below. Without the early return, + // a focus call against a still-bootstrapping engine would + // update `#focus=…` in the URL while the camera silently + // refused to move. const cam = state.cam; if (!cam) return; @@ -1903,31 +1909,14 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En // for "we just decided to focus on this galaxy." cb.onFocusChange?.(info); - // Snapshot the CURRENT camera state — not the original startup state — - // so an in-progress tween hands off smoothly to the new one. vec3.clone - // copies the target tuple so future mutation of cam.target doesn't - // corrupt the from-snapshot. - // - // The framing distance is 4× the galaxy's diameter (close-but-not- - // inside framing that scales naturally with size); when the - // PointInfo's diameter is the fallback 30 kpc, this lands on the - // pre-v4 placeholder framing exactly. - state.subsystems.tweens.start({ - startMs: performance.now(), - durationMs: FOCUS_TWEEN_MS, - fromTarget: vec3.clone(cam.target as vec3), - toTarget: vec3.fromValues(info.x, info.y, info.z), - fromDistance: cam.distance, - toDistance: focusDistanceMpc(info.diameterKpc), - fromYaw: cam.yaw, - toYaw: cam.yaw, // preserve yaw — user keeps their orientation - fromPitch: cam.pitch, - toPitch: cam.pitch, // preserve pitch - }); - // Kick the loop into motion — the tween's per-frame advance will - // keep it ticking via the still-animating predicate until the - // tween completes. - state.subsystems.scheduler.requestRender(); + // The framing distance is derived from the galaxy's diameter + // (close-but-not-inside framing that scales naturally with size); + // when the PointInfo's diameter is the fallback 30 kpc, this + // lands on the pre-v4 placeholder framing exactly. All the + // tween-construction details (yaw/pitch preservation, vec3.clone + // of cam.target, performance.now() startMs, scheduler kick-off) + // live in `tweenToGalaxy`. + tweenToGalaxy(state, info); }, selectFamous(id) { @@ -1961,25 +1950,14 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En cb.onFocusChange?.(info); // Tween the camera onto the galaxy — same tween as `focusOn`. - // We inline the tween-creation here rather than calling `handle.focusOn` - // because we're inside the object literal and `this` would be unreliable - // at call time (depending on how App.tsx invokes the handle method). - // Copying the tween-setup block keeps the behaviour identical. - const cam = state.cam; - if (!cam) return; - state.subsystems.tweens.start({ - startMs: performance.now(), - durationMs: FOCUS_TWEEN_MS, - fromTarget: vec3.clone(cam.target as vec3), - toTarget: vec3.fromValues(info.x, info.y, info.z), - fromDistance: cam.distance, - toDistance: focusDistanceMpc(info.diameterKpc), - fromYaw: cam.yaw, - toYaw: cam.yaw, - fromPitch: cam.pitch, - toPitch: cam.pitch, - }); - state.subsystems.scheduler.requestRender(); + // We don't call `handle.focusOn` directly because `this` would be + // unreliable at call time (depending on how App.tsx invokes the + // handle method) and because focusOn fires `onFocusChange` again, + // which we already did above with the prebuilt PointInfo. + // `tweenToGalaxy` is the shared kernel both methods build on top + // of — same observable behaviour, no duplication. Cam-null is + // handled inside the helper. + tweenToGalaxy(state, info); }, getCloudObjIds(source) { @@ -2047,22 +2025,11 @@ export function createEngine(canvas: HTMLCanvasElement, cb: EngineCallbacks): En // the selection. cb.onFocusChange?.(info); - // Camera focus tween — same setup as selectFamous / focusOn. - const cam = state.cam; - if (!cam) return; - state.subsystems.tweens.start({ - startMs: performance.now(), - durationMs: FOCUS_TWEEN_MS, - fromTarget: vec3.clone(cam.target as vec3), - toTarget: vec3.fromValues(info.x, info.y, info.z), - fromDistance: cam.distance, - toDistance: focusDistanceMpc(info.diameterKpc), - fromYaw: cam.yaw, - toYaw: cam.yaw, - fromPitch: cam.pitch, - toPitch: cam.pitch, - }); - state.subsystems.scheduler.requestRender(); + // Camera focus tween — same setup as selectFamous / focusOn, + // routed through the shared `tweenToGalaxy` kernel. The helper's + // own cam-null guard absorbs the post-destroy / pre-bootstrap + // race so this method doesn't need a local one. + tweenToGalaxy(state, info); }, focusOnHome() {