From 539a0f0a3edc66677041ee887e56a05665369802 Mon Sep 17 00:00:00 2001 From: Senrokai Date: Thu, 17 Sep 2026 19:03:11 +0200 Subject: [PATCH] Answer the review: budget in drawn pixels, re-ask when the budget moves, sort only the band it runs out in - The budget counted CSS pixels; lines are drawn in device pixels, so a screen scaled to 150% or 200% drew 1.5-2x the calibrated line. It now counts the canvas's drawn pixels. - A graph was re-asked only when the drawn stars changed, so with a star budget covering the whole catalogue, or a resize, its budget and centre stayed wherever the layer was turned on. A view that chose its stars again now asks, and a graph is rebuilt when the stars, the range or the budget changed (the budget by more than half the margin, or its centre by more than 5 pc). - From inside a system the budget was worked out in astronomical units about the system's origin. Graphs are now asked for in parsec space only; the flight back out asks. - Comparing budgets let a request re-asked with a slightly different one supersede its twin, and the twin's rejection cleared the state of the request that replaced it. A rejection now clears it only for the latest request. - The worker sorted every link to keep a few thousand, 2.2x an unbudgeted build. It now bands links by distance, keeps every band before the one the budget runs out in, and sorts only that one: 142-168 ms on the real catalogue against 233-388 ms, 103 ms unbudgeted, returning early when all fit. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016jxMkwA2rbicdGxHosecYi --- .../galaxy-system-scene.component.spec.ts | 68 +++++++++++++++- .../galaxy-system-scene.component.ts | 54 ++++++++++--- src/app/shared/astro/jump-links.spec.ts | 55 +++++++++++++ src/app/shared/astro/jump-links.ts | 77 +++++++++++++------ 4 files changed, 219 insertions(+), 35 deletions(-) diff --git a/src/app/features/galaxy-system/galaxy-system-scene.component.spec.ts b/src/app/features/galaxy-system/galaxy-system-scene.component.spec.ts index b861e2d..e669b9b 100644 --- a/src/app/features/galaxy-system/galaxy-system-scene.component.spec.ts +++ b/src/app/features/galaxy-system/galaxy-system-scene.component.spec.ts @@ -133,6 +133,13 @@ class FakeEngineService { resize(): void {} + /** The canvas's device pixels per CSS pixel, as the renderer was told. */ + pixelRatio = 1; + + getRenderer(): { getPixelRatio(): number } { + return { getPixelRatio: () => this.pixelRatio }; + } + /** Test helper: simulates one rendered frame by invoking every registered tick callback. */ tick(deltaSeconds: number): void { for (const callback of this.tickCallbacks) { @@ -475,14 +482,71 @@ describe('GalaxySystemSceneComponent camera-flight transitions', () => { it('asks for as much of the graph as a million pixels of line make, around where the view is centred', async () => { const links = vi.fn((_rangePc: number, _drawn: Uint32Array, _budget?: LinkBudget) => Promise.resolve(new Float32Array(0))); Object.defineProperty((fixture.nativeElement as HTMLElement).querySelector('canvas')!, 'clientHeight', { value: 1080 }); + // A screen scaled to 200%: 1080 CSS pixels are 2160 drawn ones, and the lines are drawn in those. + engine.pixelRatio = 2; linkScene(links); await settle(); const budget = links.mock.calls[0][2]; - // The view opens centred on the Sun: its frame's half-height there, over 540 pixels, is a pixel's worth of parsecs. + // The view opens centred on the Sun: its frame's half-height there, over 1080 drawn pixels, is a pixel's worth of parsecs. const halfHeight = engine.getCamera().position.length() * Math.tan((50 * Math.PI) / 360); expect(budget?.centre).toEqual({ x: 0, y: 0, z: 0 }); - expect(budget?.lengthPc).toBeCloseTo((1_000_000 * halfHeight) / 540, 3); + expect(budget?.lengthPc).toBeCloseTo((1_000_000 * halfHeight) / 1080, 3); + }); + + it('asks again once the view has zoomed past the budget it asked with, though the drawn stars are the same', async () => { + // All three stars fit the star budget, so the drawn set never changes: only the budget can. + const links = vi.fn((_rangePc: number, _drawn: Uint32Array, _budget?: LinkBudget) => Promise.resolve(new Float32Array(0))); + Object.defineProperty((fixture.nativeElement as HTMLElement).querySelector('canvas')!, 'clientHeight', { value: 1080 }); + const component = linkScene(links); + await advanceFrames(engine, 0.3); + await settle(); + const asked = links.mock.calls.length; + + const camera = engine.getCamera(); + camera.position.sub(component.controls.target).multiplyScalar(0.5).add(component.controls.target); + await advanceFrames(engine, 0.3); + await settle(); + + expect(links.mock.calls.length).toBe(asked + 1); + expect(links.mock.calls.at(-1)![1]).toBe(links.mock.calls[0][1]); + }); + + it('asks for no graph from inside a system, where distances are in astronomical units', async () => { + const links = vi.fn((_rangePc: number, _drawn: Uint32Array, _budget?: LinkBudget) => Promise.resolve(new Float32Array(0))); + navigationStore.selectStar(SUN.id); + await flushAsync(); + await advanceFrames(engine, 2.5); + + linkScene(links); + await settle(); + + expect(links).not.toHaveBeenCalled(); + }); + + it('keeps what it asked for when an older request it replaced is rejected', async () => { + // Off and on again while a graph is still waiting: the waiting one is replaced, and its + // rejection must not be taken for the request that replaced it. + const pending: Array<{ resolve: (segments: Float32Array) => void; reject: (error: Error) => void }> = []; + const setSegments = vi.spyOn(JumpLinkRenderer.prototype, 'setSegments'); + const component = linkScene(() => new Promise((resolve, reject) => pending.push({ resolve, reject }))); + await settle(); + component.display.update((display) => ({ ...display, jumpLinks: false })); + TestBed.tick(); + await settle(); + component.display.update((display) => ({ ...display, jumpLinks: true })); + TestBed.tick(); + await settle(); + expect(pending).toHaveLength(2); + + pending[0].reject(new Error('Superseded by a newer request')); + await flushAsync(); + const graph = new Float32Array(6); + pending[1].resolve(graph); + await flushAsync(); + + expect(setSegments).toHaveBeenLastCalledWith(graph); + setSegments.mockRestore(); }); it('gives a view on the move a new graph at least every quarter second, rather than waiting for it to stop', async () => { diff --git a/src/app/features/galaxy-system/galaxy-system-scene.component.ts b/src/app/features/galaxy-system/galaxy-system-scene.component.ts index 1598c40..971993d 100644 --- a/src/app/features/galaxy-system/galaxy-system-scene.component.ts +++ b/src/app/features/galaxy-system/galaxy-system-scene.component.ts @@ -77,6 +77,19 @@ const LABEL_REACH_NDC = 0.3; * emits per pixel; and how often at most a view on the move gets a graph for its new drawn stars. */ const JUMP_LINK_REBUILD_DELAY_MS = 250; +/** + * Whether a graph asked for with one budget still serves another: the same, unless the view has + * zoomed by more than half its margin or its centre has moved by more than a fifth of the + * neighbourhood drawn whole. + */ +function servesTheSame(asked: LinkBudget | undefined, now: LinkBudget | undefined): boolean { + if (!asked || !now) { + return asked === now; + } + const moved = Math.hypot(now.centre.x - asked.centre.x, now.centre.y - asked.centre.y, now.centre.z - asked.centre.z); + return Math.abs(now.lengthPc / asked.lengthPc - 1) <= VIEW_MARGIN / 2 && moved <= STAR_FIELD_REFOCUS_PC; +} + /** * How much jump-link line the layer draws, in pixels of length on screen: about a million, measured * where lines are longest. @@ -388,6 +401,9 @@ export class GalaxySystemSceneComponent implements AfterViewInit, OnDestroy { /** The range and the stars the drawn graph was last asked for, so a rebuild is skipped when neither moved. */ private drawnJumpRangePc: number | null = null; private linkedStars: Uint32Array | null = null; + private linkedBudget: LinkBudget | undefined; + /** Counts graph requests, so a rejection can tell whether it is for the latest one. */ + private linkRequest = 0; private jumpLinkRebuild?: ReturnType; /** The current system's neighbours, resolved on arrival: id, name, distance and bearing. */ private neighbours: readonly { star: StarRecord; distancePc: number; direction: THREE.Vector3 }[] = []; @@ -840,7 +856,7 @@ export class GalaxySystemSceneComponent implements AfterViewInit, OnDestroy { const pinned = () => pinnedIds.map((id) => neighbourhood.indexOf(id)).filter((index): index is number => index !== undefined); const centre = this.controls?.target ?? GALAXY_OVERVIEW_TARGET; - const drawnBefore = this.starField.drawnStars; + let chose = false; // At galactic scale the whole catalogue is a smudge a few pixels across, and the view sweeps // hundreds of parsecs a pass: chosen once for the whole sky on the way out, then left alone, @@ -850,6 +866,7 @@ export class GalaxySystemSceneComponent implements AfterViewInit, OnDestroy { this.starField.refocus({ centre, pinned: pinned(), hosts: this.hostStars }); this.starFieldCamera = null; this.starFieldPins = pins; + chose = true; } } else { const halfHeight = this.engine.visibleHalfHeight(camera.position.distanceTo(centre)); @@ -882,13 +899,15 @@ export class GalaxySystemSceneComponent implements AfterViewInit, OnDestroy { this.starFieldFocus.copy(centre); this.starFieldHalfHeight = halfHeight; this.starFieldPins = pins; + chose = true; } } - // The graph links the drawn stars, so a new set wants a new graph. Not one per pass while the - // view keeps moving, and not one pushed back by every pass either, or an orbit would never get - // one: at most one every `JUMP_LINK_REBUILD_DELAY_MS`. - if (this.starField.drawnStars !== drawnBefore && this.jumpLinkRebuild === undefined) { + // The graph links the drawn stars, and spends its budget around the view's centre, so a view that + // has moved may want a new one; `refreshJumpLinks` asks only if the stars or the budget changed. + // Not one per pass while the view keeps moving, and not one pushed back by every pass either, or + // an orbit would never get one: at most one every `JUMP_LINK_REBUILD_DELAY_MS`. + if (chose && this.jumpLinkRebuild === undefined) { this.scheduleJumpLinks(); } } @@ -1561,27 +1580,39 @@ export class GalaxySystemSceneComponent implements AfterViewInit, OnDestroy { this.jumpLinks.setSegments(new Float32Array(0)); this.drawnJumpRangePc = null; this.linkedStars = null; + this.linkedBudget = undefined; } return; } + // Asked for in parsec space only: inside a system the camera and its centre are in astronomical + // units about the system's own origin, which would make a budget of the wrong size in the wrong + // place. The flight back out chooses the drawn stars again, and that asks. + if (!this.galaxyGroup.visible) { + return; + } const drawn = this.starField.drawnStars; - if (this.drawnJumpRangePc === rangePc && this.linkedStars === drawn) { + const budget = this.jumpLinkBudget(); + if (this.drawnJumpRangePc === rangePc && this.linkedStars === drawn && servesTheSame(this.linkedBudget, budget)) { return; } this.drawnJumpRangePc = rangePc; this.linkedStars = drawn; - void this.routing.links(rangePc, drawn, this.jumpLinkBudget()).then( + this.linkedBudget = budget; + const request = ++this.linkRequest; + void this.routing.links(rangePc, drawn, budget).then( (segments) => { if (this.drawnJumpRangePc === rangePc) { this.jumpLinks?.setSegments(segments); } }, () => { - // Replaced by a newer request, or failed. Either way this graph is not drawn, and must not - // be remembered as if it were, or asking for it again would be skipped. - if (this.drawnJumpRangePc === rangePc && this.linkedStars === drawn) { + // Replaced by a newer request, or failed. Only the latest request's rejection means no graph + // is on its way; then nothing is remembered as drawn, so asking again is not skipped. An older + // one's says nothing about the request that replaced it, which may ask the same thing. + if (request === this.linkRequest) { this.drawnJumpRangePc = null; this.linkedStars = null; + this.linkedBudget = undefined; } } ); @@ -1592,7 +1623,8 @@ export class GalaxySystemSceneComponent implements AfterViewInit, OnDestroy { * `JUMP_LINK_PIXEL_BUDGET` pixels of line make at that depth. None without a canvas to measure. */ private jumpLinkBudget(): LinkBudget | undefined { - const heightPx = this.canvasRef().nativeElement.clientHeight; + // In the pixels the lines are drawn in, not in CSS pixels: a scaled or HiDPI screen draws more of them. + const heightPx = this.canvasRef().nativeElement.clientHeight * this.engine.getRenderer().getPixelRatio(); if (heightPx === 0) { return undefined; } diff --git a/src/app/shared/astro/jump-links.spec.ts b/src/app/shared/astro/jump-links.spec.ts index 6aae244..b070a93 100644 --- a/src/app/shared/astro/jump-links.spec.ts +++ b/src/app/shared/astro/jump-links.spec.ts @@ -168,6 +168,30 @@ function linksDrawn(segments: Float32Array, points: readonly StarPoint[]): strin return links; } +/** + * What a budget should keep, worked out the slow way: every link sorted by how near its nearer end + * is to the centre, then taken until one does not fit. Lengths and distances as the float32 buffer + * holds them. + */ +function nearestFirst(points: readonly StarPoint[], rangePc: number, centre: { x: number; y: number; z: number }, lengthPc: number): string[] { + const all = jumpLinkSegments(index([...points]), rangePc); + const links = Array.from({ length: all.length / 6 }, (_, link) => { + const v = Array.from(all.subarray(link * 6, link * 6 + 6)); + const nearer = Math.fround(Math.sqrt(Math.min((v[0] - centre.x) ** 2 + (v[1] - centre.y) ** 2 + (v[2] - centre.z) ** 2, (v[3] - centre.x) ** 2 + (v[4] - centre.y) ** 2 + (v[5] - centre.z) ** 2))); + return { link, nearer, length: Math.fround(Math.hypot(v[3] - v[0], v[4] - v[1], v[5] - v[2])), key: linksDrawn(all.subarray(link * 6, link * 6 + 6), points)[0] }; + }).sort((a, b) => a.nearer - b.nearer || a.link - b.link); + const kept: string[] = []; + let total = 0; + for (const { length, key } of links) { + if (total + length > lengthPc) { + break; + } + total += length; + kept.push(key); + } + return kept; +} + /** Stars a parsec apart along x, as points, for reading a segment buffer back. */ function chainPoints(count: number): StarPoint[] { return Array.from({ length: count }, (_, i) => ({ id: i, x: i, y: 0, z: 0 })); @@ -202,6 +226,37 @@ describe('jumpLinkSegments', () => { expect(segments.buffer.byteLength).toBe(segments.byteLength); }); + it('keeps exactly the links a full nearest-first sort would, without sorting them all', () => { + let seed = 7; + const random = () => ((seed = (seed * 1103515245 + 12345) % 2147483648) / 2147483648) * 40 - 20; + const points: StarPoint[] = Array.from({ length: 600 }, (_, id) => ({ id, x: random(), y: random(), z: random() })); + const centre = { x: 3, y: -2, z: 1 }; + + for (const lengthPc of [0, 5, 60, 900, 4000, 1e9]) { + expect(linksDrawn(jumpLinkSegments(index(points), 4, { centre, lengthPc }), points).sort()).toEqual(nearestFirst(points, 4, centre, lengthPc).sort()); + } + }); + + it('sorts the distance band the budget runs out in, and stops at the first link there that does not fit', () => { + // One pair 4 kpc out makes each band about a parsec deep, so dozens of short links near the + // centre share the band the budget ends in, in whatever order the grid walks them. + let seed = 3; + const random = () => (seed = (seed * 1103515245 + 12345) % 2147483648) / 2147483648; + const points: StarPoint[] = [{ id: 0, x: 4000, y: 0, z: 0 }, { id: 1, x: 4000.03, y: 0, z: 0 }]; + for (let pair = 0; pair < 40; pair++) { + const r = 0.05 + random() * 0.9; + const theta = random() * Math.PI * 2; + const x = r * Math.cos(theta); + const y = r * Math.sin(theta); + points.push({ id: 2 + pair * 2, x, y, z: 0 }, { id: 3 + pair * 2, x, y, z: 0.005 + random() * 0.04 }); + } + const centre = { x: 0, y: 0, z: 0 }; + + for (const lengthPc of [0.1, 0.3, 0.5]) { + expect(linksDrawn(jumpLinkSegments(index(points), 0.05, { centre, lengthPc }), points).sort()).toEqual(nearestFirst(points, 0.05, centre, lengthPc).sort()); + } + }); + it('counts the budget in parsecs of link, not in links', () => { // Stars at 0, 1 and 3: a 2 pc link nearest the centre, then a 1 pc one. Two and a half parsecs // hold the first and not both, though two links would fit a count of two and a half. diff --git a/src/app/shared/astro/jump-links.ts b/src/app/shared/astro/jump-links.ts index d854b83..fca2f03 100644 --- a/src/app/shared/astro/jump-links.ts +++ b/src/app/shared/astro/jump-links.ts @@ -222,11 +222,10 @@ export interface LinkBudget { } /** - * Keys pack a link's nearer-end distance, in thousandths of a parsec, above its index, so one - * numeric sort of plain doubles orders the links nearest first: room for four million links and - * two thousand kiloparsecs, inside a double's exact integers. + * How many distance bands a budgeted graph is split into to find where its budget runs out, so that + * only the links in that one band are sorted rather than all of them. */ -const LINK_INDEX_SPAN = 2 ** 22; +const DISTANCE_BANDS = 4096; /** * Every link within `rangePc` between two of the stars `index` holds, each pair once, as vertex @@ -260,31 +259,65 @@ export function jumpLinkSegments(index: StarNeighbourhood, rangePc: number, budg return vertices.slice(0, length); } + // Each link's nearer end's distance from the centre, and its length. const { centre } = budget; const count = length / 6; - const keys = new Float64Array(count); + const nearness = new Float32Array(count); + const lengths = new Float32Array(count); + let totalPc = 0; + let farthest = 0; for (let link = 0; link < count; link++) { const at = link * 6; - const nearer = Math.min( - Math.hypot(vertices[at] - centre.x, vertices[at + 1] - centre.y, vertices[at + 2] - centre.z), - Math.hypot(vertices[at + 3] - centre.x, vertices[at + 4] - centre.y, vertices[at + 5] - centre.z) - ); - keys[link] = Math.floor(nearer * 1000) * LINK_INDEX_SPAN + link; + const ax = vertices[at] - centre.x; + const ay = vertices[at + 1] - centre.y; + const az = vertices[at + 2] - centre.z; + const bx = vertices[at + 3] - centre.x; + const by = vertices[at + 4] - centre.y; + const bz = vertices[at + 5] - centre.z; + nearness[link] = Math.sqrt(Math.min(ax * ax + ay * ay + az * az, bx * bx + by * by + bz * bz)); + lengths[link] = Math.hypot(bx - ax, by - ay, bz - az); + totalPc += lengths[link]; + farthest = Math.max(farthest, nearness[link]); + } + if (totalPc <= budget.lengthPc) { + return vertices.slice(0, length); } - keys.sort(); - const kept = new Float32Array(length); - let keptLength = 0; - let totalPc = 0; - for (const key of keys) { - const at = (key % LINK_INDEX_SPAN) * 6; - const linkPc = Math.hypot(vertices[at + 3] - vertices[at], vertices[at + 4] - vertices[at + 1], vertices[at + 5] - vertices[at + 2]); - if (totalPc + linkPc > budget.lengthPc) { + // Nearest first, without sorting them all: every link in the bands before the one where the budget + // runs out fits, and only that band's links are sorted to see how many of them do. Sorting all + // 730 000 links at 30 pc from the Sun to keep 4 400 doubled the time a graph took in the worker. + const bands = new Uint16Array(count); + const bandLengths = new Float64Array(DISTANCE_BANDS); + const bandsPerPc = farthest > 0 ? DISTANCE_BANDS / farthest : 0; + for (let link = 0; link < count; link++) { + bands[link] = Math.min(DISTANCE_BANDS - 1, Math.floor(nearness[link] * bandsPerPc)); + bandLengths[bands[link]] += lengths[link]; + } + let lastBand = 0; + let keptPc = 0; + while (keptPc + bandLengths[lastBand] <= budget.lengthPc) { + keptPc += bandLengths[lastBand++]; + } + const keptLinks: number[] = []; + const boundary: number[] = []; + for (let link = 0; link < count; link++) { + const band = bands[link]; + if (band < lastBand) { + keptLinks.push(link); + } else if (band === lastBand) { + boundary.push(link); + } + } + boundary.sort((a, b) => nearness[a] - nearness[b] || a - b); + for (const link of boundary) { + if (keptPc + lengths[link] > budget.lengthPc) { break; } - totalPc += linkPc; - kept.set(vertices.subarray(at, at + 6), keptLength); - keptLength += 6; + keptPc += lengths[link]; + keptLinks.push(link); } - return kept.slice(0, keptLength); + + const kept = new Float32Array(keptLinks.length * 6); + keptLinks.forEach((link, at) => kept.set(vertices.subarray(link * 6, link * 6 + 6), at * 6)); + return kept; }