diff --git a/app/src/city/components/buildings/index.ts b/app/src/city/components/buildings/index.ts index 9fd4589a..8fe201fe 100644 --- a/app/src/city/components/buildings/index.ts +++ b/app/src/city/components/buildings/index.ts @@ -66,8 +66,6 @@ export interface Buildings extends SceneComponent { disposeAdPanels(): void; /** Building lookup by file path → { mesh, building, instanceId }. */ getBuildingByPath(p: string): { mesh: THREE.Mesh; building: Building; instanceId: number } | null; - /** Tallest building (layout pos + dims), or null when empty. */ - getTallest(): { x: number; y: number; w: number; d: number; h: number } | null; /** Cell map (consumed by picker / fader / outline / diff mirror). */ getCells(): Map; /** Building index, or null pre-rebuild. */ @@ -479,16 +477,6 @@ export function createBuildings(ctx: SceneContext): Buildings { onResize, dispose, getBuildingByPath: (p) => _buildingsByPath[p] || null, - getTallest: () => { - let tallest: Building | null = null; - for (const cell of _cells.values()) { - for (const b of cell.buildings) { - if (b && (!tallest || b.h > tallest.h)) tallest = b; - } - } - if (!tallest) return null; - return { x: tallest.x, y: tallest.y, w: tallest.w, d: tallest.d, h: tallest.h }; - }, getCells: () => _cells, getBuildingIndex: () => _buildingIndex, getMeshForBuilding, diff --git a/app/src/city/index.ts b/app/src/city/index.ts index 882ca96c..b5eb0ac3 100644 --- a/app/src/city/index.ts +++ b/app/src/city/index.ts @@ -108,32 +108,30 @@ export async function createCity(canvas: HTMLCanvasElement, manifest: Manifest): // BEFORE the rig (so bbox is set and the rig's first frame can frame the city). await applyManifest(manifest); - // cityState is threaded so the rig re-frames reactively when bbox changes - // and reads its world-framing inputs (bbox/gem/root street) directly. deps - // carries only the component-geometry accessors the rig can't reach via state. + // cityState is threaded so the rig re-frames reactively when bbox changes and + // reads its world-framing inputs (bbox/gem/root street/tallest building) + // directly. deps carries only the component-geometry accessors the rig can't + // reach via state. const rig = createCameraRig({ canvas, cityState, deps: { - getTallestBuilding: () => buildings.getTallest(), getRepoLabelBounds: () => repoLabel.getPanelBounds(), getTreeBoundsBySha: (sha) => trees.getRenderer()?.getTreeBoundsBySha(sha) ?? null, }, }); - // Reframe the camera on a SOURCE change (repo switch) — the one camera action - // that used to live in the view. bbox is reassigned on every non-reuse apply, - // so the rig's framing is freshly captured by its own effect just above by the - // time this fires; peeking CURRENT_SOURCE_KEY filters to repo switches only — - // a config rebuild / live-update / the skeleton apply (before the fetch layer - // commits the new key) all keep the same key → no snap. reset() is untracked - // so this effect depends only on bbox. + // Snap the camera when a NEW source's city has applied (initial load or repo + // switch). Track cityRevision (every apply), not bbox: the final manifest is a + // reuse apply that leaves bbox frozen, so a bbox-only effect would miss it. The + // key guard skips the empty boot (key null — no source yet) and same-source + // re-applies (live-updates, config saves) — only a real source change reframes. let lastReframedSourceKey: string | null = null; const stopReframe = effect(() => { - void cityState.bbox.value; + void cityState.cityRevision.value; const key = CURRENT_SOURCE_KEY.peek(); if (key !== null && key !== lastReframedSourceKey) { - untracked(() => rig.reset()); lastReframedSourceKey = key; + untracked(() => rig.reset()); } }); diff --git a/app/src/city/render/cameraRig.ts b/app/src/city/render/cameraRig.ts index 045575a2..9a1c9c81 100644 --- a/app/src/city/render/cameraRig.ts +++ b/app/src/city/render/cameraRig.ts @@ -46,11 +46,10 @@ import type { Building, PickTarget, Street } from '@/types'; import type { CityState } from '@/city/state'; /** Narrow accessor surface the rig needs from the composer for component - * geometry (buildings/repoLabel/trees). World framing inputs (bbox, gem, - * root street) come from cityState directly, not through here. createCity + * geometry (repoLabel/trees). World framing inputs (bbox, gem, root street, + * tallest building) come from cityState directly, not through here. createCity * (city/index.ts) passes a small deps literal that satisfies this. */ export interface CameraRigDeps { - getTallestBuilding(): { x: number; y: number; w: number; d: number; h: number } | null; getRepoLabelBounds(): { centerX: number; centerY: number; @@ -258,7 +257,7 @@ export function createCameraRig({ // Take the max across the 4 roof corners. HEADROOM scales D up for // breathing room above the roof (1.0 = spire flush against top edge). let heightDist = 0; - const tallest = gemPos && rootStreet ? deps.getTallestBuilding() : null; + const tallest = gemPos && rootStreet ? cityState.tallestBuilding.value : null; const labelBounds = gemPos && rootStreet ? deps.getRepoLabelBounds() : null; if (tallest || labelBounds) { const sinElev = dir.y; @@ -293,22 +292,13 @@ export function createCameraRig({ } } } - // Include the floating repo-label panel so empty worlds (no - // buildings) still frame to show the label, and crowded worlds - // never crop it off the top edge. The panel billboards to face - // the camera, so its horizontal extent could rotate either way — - // sample the top corners along BOTH world axes to bound it. + // Fit the label's top-edge HEIGHT only, not its width: the panel is + // centered on the gem and already inside the width-fit frame, and its + // width comes from the text-texture aspect, which only settles on web-font + // load — sampling it made the frame jump after load (issue #62). if (labelBounds) { const topY = labelBounds.centerY + labelBounds.halfHeight; - const r = labelBounds.halfWidth; - for (const [dx, dz] of [ - [-r, 0], - [r, 0], - [0, -r], - [0, r], - ]) { - _fitPoint(labelBounds.centerX + dx, topY, labelBounds.centerZ + dz); - } + _fitPoint(labelBounds.centerX, topY, labelBounds.centerZ); } heightDist *= TALLEST_BUILDING_HEADROOM_MULT; } @@ -397,15 +387,10 @@ export function createCameraRig({ } function reset() { - // If framing wasn't captured yet (or was cleared by a disposal cycle), - // try once more before giving up — silent no-op on R is worse than - // re-running the cheap framing computation. Returns false only when - // the city has no bbox at all (e.g., pre-manifest), in which case - // there's nothing to reset to. - if (!initialCamPos || !initialTarget) { - if (!_captureFraming()) return; - if (!initialCamPos || !initialTarget) return; - } + // Recapture from the CURRENT layout every call: the final manifest is a reuse + // apply (bbox frozen), so the bbox effect that normally caches the pose + // doesn't re-fire there — a cached pose could be stale. False only pre-manifest. + if (!_captureFraming() || !initialCamPos || !initialTarget) return; // Cancel any in-flight focus/reset animation so it can't keep // walking the camera away from the snap target. camAnimToken++; diff --git a/app/src/city/state/index.ts b/app/src/city/state/index.ts index 4a463f85..52cff6fa 100644 --- a/app/src/city/state/index.ts +++ b/app/src/city/state/index.ts @@ -7,16 +7,16 @@ // trees) rebuild off it. treePlacements is a source signal trees writes. // structureRevision / cityRevision / decorationRevision — change-notification // counters; consumers track them and peek the data. -// bbox / sceneBbox / cityHeight / latestWorldBounds / rootStreet / gemWorldPos — -// COMPUTED, never written. bbox = union of street rects + building footprints -// + footprint halo. +// bbox / sceneBbox / cityHeight / latestWorldBounds / rootStreet / gemWorldPos / +// tallestBuilding — COMPUTED, never written. bbox = union of street rects + +// building footprints + footprint halo. // // applyManifest + invalidateLayoutCache are defined here (the store owns its // transition); every scene component rebuilds reactively off the signals above. import { signal, computed, batch, type Signal, type ReadonlySignal } from '@preact/signals'; import * as THREE from 'three'; import { FOOTPRINT } from '@/state/stores/settings/footprint'; -import type { CityBbox, CityLayout, Manifest, Street } from '@/types'; +import type { Building, CityBbox, CityLayout, Manifest, Street } from '@/types'; import { getWorldBounds, type WorldBounds } from '../utils/floorBounds'; import { rectOfStreet } from '@/city/layout/rect'; import type { TreePlacement } from '../components/trees/treePlacement'; @@ -45,6 +45,9 @@ export interface CityState { treePlacements: Signal; readonly rootStreet: ReadonlySignal; readonly gemWorldPos: ReadonlySignal; + // Tallest building (by height) for the camera start-framing height-fit. From + // layout data, not the async building meshes (see the computed). + readonly tallestBuilding: ReadonlySignal; // { street dir.path → Street } from the layout. The buildings fader, pathLine, // picker, and the CityWorld debug API resolve a street by directory off this // instead of reaching into the streets component. @@ -176,6 +179,21 @@ export function createCityState(layoutClient: ReturnType(() => { + const l = layout.value; + if (!l) return null; + let tallest: Building | null = null; + for (const b of l.buildings) { + if (!tallest || b.h > tallest.h) tallest = b; + } + return tallest; + }); + // Street lookup by directory path. Tracks structureRevision + peeks layout // (ref-stable on a reuse apply, recomputes on a structure change) — a // { dir.path → Street } map derived straight from layout.streets. @@ -274,6 +292,7 @@ export function createCityState(layoutClient: ReturnType { expect(cs.rootStreet.value).toBe(r2); expect(cs.gemWorldPos.value!.x).not.toBe(z1); }); + + describe('tallestBuilding', () => { + const bld = (over: Partial): Building => + ({ x: 0, y: 0, w: 10, d: 10, h: 10, ...over }) as unknown as Building; + const layoutOf = (buildings: Building[]): CityLayout => + ({ buildings, streets: [] }) as unknown as CityLayout; + + it('is null with no layout and with no buildings', () => { + const cs = makeCityState(); + expect(cs.tallestBuilding.value).toBeNull(); + cs.layout.value = layoutOf([]); + expect(cs.tallestBuilding.value).toBeNull(); + }); + + it('returns the max-height building', () => { + const cs = makeCityState(); + const tall = bld({ x: 5, y: 6, w: 3, d: 4, h: 90 }); + cs.layout.value = layoutOf([bld({ h: 10 }), tall, bld({ h: 50 })]); + expect(cs.tallestBuilding.value).toBe(tall); + }); + + // Crux of #62: a reuse apply (skeleton placeholder → real heights) reassigns + // `layout` without bumping structureRevision, so tallestBuilding must track + // `layout` or it'd frame to the stale placeholder height. + it('updates when layout is reassigned without a structureRevision bump', () => { + const cs = makeCityState(); + cs.layout.value = layoutOf([bld({ h: 20 })]); // skeleton placeholder height + expect(cs.tallestBuilding.value!.h).toBe(20); + + cs.layout.value = layoutOf([bld({ h: 200 })]); // final real height — no structureRevision++ + expect(cs.tallestBuilding.value!.h).toBe(200); + }); + }); }); diff --git a/app/tests/city/components/buildings/index.test.ts b/app/tests/city/components/buildings/index.test.ts index 1906e956..75496eb6 100644 --- a/app/tests/city/components/buildings/index.test.ts +++ b/app/tests/city/components/buildings/index.test.ts @@ -3,7 +3,7 @@ // Tests for the persistent createBuildings(ctx) component. // API: createBuildings(ctx) → { group, rebuild(layout, dateRanges), // disposeAdPanels, tick(dt, frame), onResize(), dispose(), -// getBuildingByPath, getTallest, getCells, getBuildingIndex, +// getBuildingByPath, getCells, getBuildingIndex, // getMeshForBuilding }. // // The shared-material theme effect reacts to BUILDINGS/SCENE/BLOOM at @@ -166,9 +166,6 @@ describe('createBuildings()', () => { expect(buildings.getBuildingByPath('src/b.ts')!.building).toBe(b1); expect(buildings.getBuildingByPath('nope')).toBeNull(); - // getTallest reflects b1 (h=9). - expect(buildings.getTallest()).toEqual({ x: -20, y: -20, w: b1.w, d: b1.d, h: 9 }); - // getMeshForBuilding resolves through the live cell (cellId/slotId were // assigned during rebuild). const resolved = buildings.getMeshForBuilding(b0); diff --git a/app/tests/city/initialFraming.test.ts b/app/tests/city/initialFraming.test.ts new file mode 100644 index 00000000..57d7446f --- /dev/null +++ b/app/tests/city/initialFraming.test.ts @@ -0,0 +1,160 @@ +// Regression for issue #62: the camera must snap to a NEW source's city once it +// applies (not stay on the empty boot), and must NOT reframe on a same-source +// re-apply (live-update / config save). The snap rides cityRevision (every +// apply) so it catches the final reuse apply, gated on a CURRENT_SOURCE_KEY +// change. +// +// jsdom has no WebGL — mock the renderer + post pipeline like city/index.test.ts. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as THREE from 'three'; +import { EMPTY_MANIFEST } from '@/constants/manifest'; +import { CURRENT_SOURCE } from '@/state/stores/source'; +import { STREET_TIERS } from '@/state/stores/settings/streets'; +import type { Manifest } from '@/types'; +import { mkDir } from '../_helpers/cityFixtures'; + +vi.mock('three', async () => { + const actual = await vi.importActual('three'); + class FakeWebGLRenderer { + domElement: HTMLCanvasElement; + constructor(opts: { canvas: HTMLCanvasElement }) { + this.domElement = opts.canvas; + } + setPixelRatio() {} + setSize() {} + getSize(v: THREE.Vector2) { + return v; + } + render() {} + dispose() {} + copyTextureToTexture() {} + setRenderTarget() {} + getContext() { + return {}; + } + } + return { ...actual, WebGLRenderer: FakeWebGLRenderer }; +}); + +vi.mock('@/city/render/postFx', () => ({ + createPostFx: () => ({ + render: () => {}, + setSize: () => {}, + refresh: () => {}, + dispose: () => {}, + }), +})); + +// buildIconAtlas loads icon images, which never fire onload in jsdom and hang +// applyManifest. Icons are irrelevant to camera framing — stub it out. +vi.mock('@/city/components/buildings/atlas', async () => { + const actual = await vi.importActual( + '@/city/components/buildings/atlas' + ); + return { ...actual, buildIconAtlas: async () => null }; +}); + +import { createCity } from '@/city/index'; + +describe('initial-load framing (issue #62)', () => { + let rafSpy: ReturnType; + const DEFAULT_TIERS = STREET_TIERS.peek(); + + beforeEach(() => { + CURRENT_SOURCE.value = null; + rafSpy = vi + .spyOn(globalThis, 'requestAnimationFrame') + .mockImplementation((cb: FrameRequestCallback) => { + setTimeout(() => cb(performance.now()), 0); + return 0 as unknown as number; + }); + }); + + afterEach(() => { + rafSpy.mockRestore(); + CURRENT_SOURCE.value = null; + STREET_TIERS.value = DEFAULT_TIERS; + vi.clearAllMocks(); + }); + + function makeCanvas(): HTMLCanvasElement { + const canvas = document.createElement('canvas'); + Object.defineProperty(canvas, 'clientWidth', { value: 1280, configurable: true }); + Object.defineProperty(canvas, 'clientHeight', { value: 720, configurable: true }); + return canvas; + } + + // Directory-only tree (no files → no buildings), so the framing is driven by + // the root street width. 10 child dirs → root descendants_count 20 → a high + // street tier, so the real city frames clearly differently from the empty boot + // (root descendants 0 → the narrowest tier). STREET_TIERS can also be forced + // to a single width below. + function makeManifest(): Manifest { + const tree = mkDir( + 'repo', + Array.from({ length: 10 }, (_, i) => mkDir(`d${i}`, [])) + ); + return { + ...EMPTY_MANIFEST, + tree, + tree_signature: 'sig-repo', + signature: 'full-sig', + } as unknown as Manifest; + } + + // Force every street (incl. the root) to a single width, so a rebuild after + // changing it deterministically moves the framing. + function setRootWidth(width: number): void { + STREET_TIERS.value = { TIERS: [{ min_descendants: 0, width }] }; + } + + it('frames the city on initial load, not the empty boot', async () => { + const handle = await createCity(makeCanvas(), EMPTY_MANIFEST); + try { + // firstFrame framed the empty boot (no source committed yet → no snap). + const bootPos = handle.rig.camera.position.clone(); + + // The fetch layer commits the source, then the real manifest applies. + CURRENT_SOURCE.value = { src: 'test://repo' }; + await handle.applyManifest(makeManifest()); + const loadPos = handle.rig.camera.position.clone(); + + // What R produces now = the real city's framing. + handle.rig.reset(); + const resetPos = handle.rig.camera.position.clone(); + + // Sanity: the city frames differently from the empty boot. + expect(resetPos.distanceTo(bootPos)).toBeGreaterThan(1); + // The camera snapped to the city on load — no manual R needed. + expect(loadPos.distanceTo(resetPos)).toBeLessThan(0.5); + } finally { + handle.dispose(); + } + }); + + it('does not reframe on a same-source re-apply (live-update / config save)', async () => { + setRootWidth(100); + const handle = await createCity(makeCanvas(), EMPTY_MANIFEST); + try { + CURRENT_SOURCE.value = { src: 'test://repo' }; + const m = makeManifest(); + await handle.applyManifest(m); + const posLoaded = handle.rig.camera.position.clone(); + + // A same-source rebuild that moves the framing (here a Rebuild-route + // setting + invalidate; a live-update is the same shape) must leave the + // camera where it is — the source key didn't change. + setRootWidth(500); + handle.invalidateLayoutCache(); + await handle.applyManifest(m); + expect(handle.rig.camera.position.distanceTo(posLoaded)).toBeLessThan(0.5); + + // Confirm the framing genuinely changed (R re-frames to the new width). + handle.rig.reset(); + expect(handle.rig.camera.position.distanceTo(posLoaded)).toBeGreaterThan(1); + } finally { + handle.dispose(); + } + }); +}); diff --git a/app/tests/city/render/cameraRig.test.ts b/app/tests/city/render/cameraRig.test.ts index 18c0f2b2..49adf286 100644 --- a/app/tests/city/render/cameraRig.test.ts +++ b/app/tests/city/render/cameraRig.test.ts @@ -13,12 +13,13 @@ function makeStubWorld(overrides: Partial> = {}) { return { ..._baseWorld(), ...overrides }; } -// The rig reads its world-framing inputs (bbox / gemWorldPos / rootStreet) -// from cityState computeds, not from deps — so seed a layout that produces a -// non-empty bbox + a root street. Two crossing 1000-long streets span XZ to -// ±500 and a 200-tall building gives the Y extent: bbox = (-500,0,-500)..(500, -// 200,500). Exact magnitudes don't drive the focus assertions (they key off -// the focused node + a sub-distance clamp), only that framing captures at all. +// The rig reads its world-framing inputs (bbox / gemWorldPos / rootStreet / +// tallestBuilding) from cityState computeds, not from deps — so seed a layout +// that produces a non-empty bbox + a root street. Two crossing 1000-long streets +// span XZ to ±500 and a 200-tall building gives the Y extent: bbox = +// (-500,0,-500)..(500,200,500). Exact magnitudes don't drive the focus +// assertions (they key off the focused node + a sub-distance clamp), only that +// framing captures at all. function seedFramedCity(): CityState { const cs = makeCityState(); cs.layout.value = { @@ -34,14 +35,6 @@ function seedFramedCity(): CityState { function _baseWorld() { return { - getTallestBuilding: () => - ({ x: 100, y: 0, w: 30, d: 30, h: 200 }) as { - x: number; - y: number; - w: number; - d: number; - h: number; - } | null, getRepoLabelBounds: () => null as { centerX: number; @@ -197,3 +190,40 @@ describe('cameraRig top-down focus', () => { }); }); }); + +describe('cameraRig start framing', () => { + // Issue #62: the label's width (from the text aspect, which settles on web-font + // load) must not affect framing — only its top-edge height. A tall label drives + // heightDist; widening it 100× must not move the camera. + function labelDeps(halfWidth: number) { + return makeStubWorld({ + getRepoLabelBounds: () => ({ + centerX: 0, + centerY: 400, + centerZ: 0, + halfWidth, + halfHeight: 50, + }), + }); + } + + it('ignores repo-label width (only its top-edge height matters)', () => { + const narrow = createCameraRig({ + canvas: makeCanvas(), + deps: labelDeps(40), + cityState: seedFramedCity(), + }); + narrow.update(16); + const narrowPos = narrow.camera.position.clone(); + + const wide = createCameraRig({ + canvas: makeCanvas(), + deps: labelDeps(4000), + cityState: seedFramedCity(), + }); + wide.update(16); + const widePos = wide.camera.position.clone(); + + expect(widePos.distanceTo(narrowPos)).toBeLessThan(0.001); + }); +}); diff --git a/uv.lock b/uv.lock index c59cbd8a..50704fb6 100644 --- a/uv.lock +++ b/uv.lock @@ -730,15 +730,15 @@ wheels = [ [[package]] name = "starlette" -version = "1.2.1" +version = "1.3.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "anyio" }, { name = "typing-extensions", marker = "python_full_version < '3.13'" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/25/44/ec35f1b6e83094b997da438a02c8c9b0ade2b1e84cfc48bd4656780760a6/starlette-1.2.1.tar.gz", hash = "sha256:9b9b5ebb992e67d6093741e63c2f59e4f6fff986f81163c087867bd7b924b3f6", size = 2701854, upload-time = "2026-05-31T01:07:51.847Z" } +sdist = { url = "https://files.pythonhosted.org/packages/eb/e3/7c1dc7381d9f8ab7d854328ebfa884e62cb3f3d8549ddfd37c7814f42afa/starlette-1.3.1.tar.gz", hash = "sha256:05d0213193f2fbaae60e2ecb593b4add4262ad4e46536b54abe36f11a71724e0", size = 2703240, upload-time = "2026-06-12T09:23:11.602Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/1c/54/196d0c1db10af76baa4f64894448505d60d3cdf70ef92cbb35f46a4e4c71/starlette-1.2.1-py3-none-any.whl", hash = "sha256:4de0082d08c8f6764a85a54cf1120d6939507a19905c7768acad2a9f875d2b89", size = 73350, upload-time = "2026-05-31T01:07:50.09Z" }, + { url = "https://files.pythonhosted.org/packages/ec/bb/2799cc2ede3ed41131f8975621e7213dfc7ef4acbbaadfa440f32500c370/starlette-1.3.1-py3-none-any.whl", hash = "sha256:c7372aae11c3c3f26a42df7bd626cec2f47d03483d261d369516a615a53714c6", size = 73632, upload-time = "2026-06-12T09:23:10.017Z" }, ] [[package]]