From 807fbd751a6322d3e8f82b08c13a83aa3866dcbc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 29 May 2026 21:47:07 +0000 Subject: [PATCH 1/2] fix(editor): repair ball possession, line selection, snap indicator, right-click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - context-menu: ball button indexed the entities array with a string key (always undefined) so it did nothing. Possession is now modeled by the ball entity's position via EditorState.assignBall (schema-conformant, honors frame deltas, visible on the court). - interaction: lines could be drawn but never re-selected — selectLine() was never called. Added segment-based line hit-testing on click plus a selected-line highlight overlay so the line properties panel and delete are reachable. - interaction: snap indicator was wiped by the per-move innerHTML re-render; re-append it whenever it is detached from the DOM. - interaction: right-click now selects the entity (opening the action menu) instead of deleting it immediately without confirmation. - editor: cap offense/defense numbering at 9 (schema limit) and guard the palette/drop callers against the now-possible null. --- src/editor/context-menu.js | 36 +++++++++++------------ src/editor/editor.js | 48 ++++++++++++++++++++++++++++--- src/editor/interaction.js | 59 +++++++++++++++++++++++++++++++++++--- src/editor/palette.js | 3 +- src/main.js | 26 ++++++++++++++++- 5 files changed, 142 insertions(+), 30 deletions(-) diff --git a/src/editor/context-menu.js b/src/editor/context-menu.js index f9cd288..a35eb2b 100644 --- a/src/editor/context-menu.js +++ b/src/editor/context-menu.js @@ -79,7 +79,7 @@ export class ContextMenu { this._ballBtn.appendChild(ballLabel); this._ballBtn.addEventListener('click', (e) => { e.stopPropagation(); - this._toggleBall(); + this._assignBall(); }); // Delete button @@ -150,10 +150,17 @@ export class ContextMenu { this.menu.style.left = left + 'px'; this.menu.style.top = top + 'px'; - // Reflect has_ball state - const entity = this.state.doc.entities?.[entityKey]; - if (this._ballBtn && entity) { - this._ballBtn.classList.toggle('active', !!entity.has_ball); + // Ball possession only applies to players. Reflect whether this player + // currently holds the ball (i.e. the ball entity sits on top of them). + if (this._ballBtn) { + const type = pos.entity?.type; + const isPlayer = type === 'offense' || type === 'defense' || type === 'coach'; + this._ballBtn.style.display = isPlayer ? '' : 'none'; + const ballPos = positions.get('ball'); + const holds = isPlayer && ballPos + && Math.abs(ballPos.x - pos.x) < 0.01 + && Math.abs(ballPos.y - pos.y) < 0.01; + this._ballBtn.classList.toggle('active', !!holds); } } @@ -161,22 +168,11 @@ export class ContextMenu { this.menu.classList.add('hidden'); } - _toggleBall() { + _assignBall() { const key = this.state.selectedEntityKey; if (!key) return; - const entity = this.state.doc.entities?.[key]; - if (!entity) return; - - this.state.saveUndo(); - entity.has_ball = !entity.has_ball; - - // Only one player can have the ball at a time - if (entity.has_ball) { - for (const [k, e] of Object.entries(this.state.doc.entities || {})) { - if (k !== key) e.has_ball = false; - } - } - - this.state.notify('has-ball-changed'); + // Possession is modeled by the ball entity's position (OCF has a single + // ball). assignBall creates/moves the ball onto the selected player. + this.state.assignBall(key); } } diff --git a/src/editor/editor.js b/src/editor/editor.js index d2b4aaa..84ddfe2 100644 --- a/src/editor/editor.js +++ b/src/editor/editor.js @@ -4,7 +4,7 @@ */ import { createBlankDocument } from '../schema.js'; -import { entityKey } from '../court/renderer.js'; +import { entityKey, resolveEntityPositions } from '../court/renderer.js'; const MAX_UNDO = 30; @@ -142,13 +142,53 @@ export class EditorState { } } - /** Get next available number for a given entity type. */ + /** + * Get next available number for a given entity type. + * offense/defense are capped at 9 (schema limit); returns null if full. + */ nextEntityNr(type) { const existing = this.doc.entities.filter(e => e.type === type).map(e => e.nr || 0); - for (let i = 1; i <= 99; i++) { + const max = (type === 'offense' || type === 'defense') ? 9 : 99; + for (let i = 1; i <= max; i++) { if (!existing.includes(i)) return i; } - return existing.length + 1; + return null; + } + + /** + * Assign ball possession to a player by moving the ball entity onto them. + * Creates the ball entity if it does not exist yet. Since there is only one + * ball, possession is inherently exclusive. Honors the current frame: on + * frame 0 the base position is updated, otherwise a frame delta is written. + */ + assignBall(playerKey) { + const positions = resolveEntityPositions(this.doc, this.currentFrameIndex); + const target = positions.get(playerKey); + if (!target) return; + const round = (v) => Math.round(v * 100) / 100; + + this.saveUndo(); + + // Ensure a ball entity exists (base position seeded from frame 0). + let ball = this.doc.entities.find(e => e.type === 'ball'); + if (!ball) { + const base = resolveEntityPositions(this.doc, 0).get(playerKey) || target; + ball = { type: 'ball', x: round(base.x), y: round(base.y) }; + this.doc.entities.push(ball); + } + + // Place the ball on the player for the current frame. + if (this.currentFrameIndex === 0) { + ball.x = round(target.x); + ball.y = round(target.y); + } else { + const frame = this.doc.frames[this.currentFrameIndex]; + if (frame) { + if (!frame.entity_states) frame.entity_states = {}; + frame.entity_states['ball'] = { x: round(target.x), y: round(target.y) }; + } + } + this.notify('ball-assign'); } // --- Line Operations --- diff --git a/src/editor/interaction.js b/src/editor/interaction.js index 8d8af76..f3fe1bd 100644 --- a/src/editor/interaction.js +++ b/src/editor/interaction.js @@ -3,7 +3,17 @@ */ import { resolveEntityPositions, entityKey } from '../court/renderer.js'; -import { findSnapPosition } from '../court/positions.js'; +import { findSnapPosition, resolveCoordinate } from '../court/positions.js'; + +/** Shortest distance from point p to line segment a–b (in pixels). */ +function distToSegment(p, a, b) { + const dx = b.x - a.x, dy = b.y - a.y; + const lenSq = dx * dx + dy * dy; + let t = lenSq === 0 ? 0 : ((p.x - a.x) * dx + (p.y - a.y) * dy) / lenSq; + t = Math.max(0, Math.min(1, t)); + const cx = a.x + t * dx, cy = a.y + t * dy; + return Math.hypot(p.x - cx, p.y - cy); +} export class InteractionManager { constructor(svgElement, editorState, getTransform) { @@ -65,6 +75,34 @@ export class InteractionManager { return closest; } + /** Find which line (in the current frame) is at a given SVG position. */ + _hitTestLine(svgX, svgY) { + const t = this.getTransform(); + if (!t) return null; + const frame = this.state.doc.frames[this.state.currentFrameIndex]; + if (!frame || !frame.lines) return null; + const ruleset = this.state.doc.court?.ruleset || 'fiba'; + const customPos = this.state.doc.named_positions?.custom || {}; + const threshold = 8; // hit distance in pixels + let best = null; + let bestDist = threshold; + + for (let i = 0; i < frame.lines.length; i++) { + const pts = (frame.lines[i].coords || []).map(c => { + const abs = resolveCoordinate(c, ruleset, customPos); + return t.toSvg(abs.x, abs.y); + }); + for (let s = 0; s + 1 < pts.length; s++) { + const d = distToSegment({ x: svgX, y: svgY }, pts[s], pts[s + 1]); + if (d < bestDist) { + bestDist = d; + best = i; + } + } + } + return best; + } + _onMouseDown(e) { if (e.button !== 0) return; if (this.state.activeTool !== 'select') return; @@ -160,8 +198,13 @@ export class InteractionManager { const hitKey = this._hitTestEntity(svgPt.x, svgPt.y); if (hitKey) { this.state.select(hitKey); + return; + } + // No entity hit — try lines, otherwise deselect. + const lineIdx = this._hitTestLine(svgPt.x, svgPt.y); + if (lineIdx !== null) { + this.state.selectLine(lineIdx); } else if (!this.dragging || !this.dragging?.hasMoved) { - // Check if we clicked a line (approximate) this.state.deselect(); } } @@ -188,11 +231,14 @@ export class InteractionManager { } _onContextMenu(e) { + // Suppress the browser menu and instead select the entity so the + // floating action menu (with its Delete button) appears. Right-click no + // longer deletes immediately, which was surprising and destructive. e.preventDefault(); const svgPt = this._svgPoint(e); const hitKey = this._hitTestEntity(svgPt.x, svgPt.y); if (hitKey) { - this.state.removeEntity(hitKey); + this.state.select(hitKey); } } @@ -228,7 +274,8 @@ export class InteractionManager { entity = { type: 'station', nr: this.state.nextEntityNr('station'), x, y }; break; } - if (entity) this.state.addEntity(entity); + // nextEntityNr returns null when the numbered slots are exhausted. + if (entity && entity.nr !== null) this.state.addEntity(entity); } _onKeyDown(e) { @@ -287,6 +334,10 @@ export class InteractionManager { this.snapIndicator.setAttribute('stroke-width', '2'); this.snapIndicator.setAttribute('stroke-dasharray', '3,2'); this.snapIndicator.setAttribute('pointer-events', 'none'); + } + // renderEditor() resets svg.innerHTML on every move, detaching the + // indicator — re-append it whenever it is not currently in the DOM. + if (!this.snapIndicator.isConnected) { this.svg.appendChild(this.snapIndicator); } this.snapIndicator.setAttribute('cx', svgPt.x); diff --git a/src/editor/palette.js b/src/editor/palette.js index 0599f49..1d586a3 100644 --- a/src/editor/palette.js +++ b/src/editor/palette.js @@ -73,7 +73,8 @@ export class Palette { entity = { type: 'station', nr: this.state.nextEntityNr('station'), x: pos.x + 2, y: pos.y }; break; } - if (entity) this.state.addEntity(entity); + // nextEntityNr returns null when the numbered slots are exhausted. + if (entity && entity.nr !== null) this.state.addEntity(entity); } } diff --git a/src/main.js b/src/main.js index 5263175..0d612e2 100644 --- a/src/main.js +++ b/src/main.js @@ -5,7 +5,8 @@ import { renderOCF, resolveEntityPositions, entityKey } from './court/renderer.js'; import { createTransform } from './court/court-svg.js'; -import { straightPath } from './court/curves.js'; +import { straightPath, curvedPath } from './court/curves.js'; +import { resolveCoordinate } from './court/positions.js'; import { FramePlayer } from './player/player.js'; import { EditorState } from './editor/editor.js'; import { Palette } from './editor/palette.js'; @@ -201,6 +202,29 @@ function renderEditor() { } } + // Selected line highlight + if (editorState.selectedLineIndex !== null && transform) { + const frame = editorState.doc.frames[editorState.currentFrameIndex]; + const line = frame?.lines?.[editorState.selectedLineIndex]; + if (line) { + const ruleset = editorState.doc.court?.ruleset || 'fiba'; + const customPos = editorState.doc.named_positions?.custom || {}; + const pts = (line.coords || []).map(c => { + const abs = resolveCoordinate(c, ruleset, customPos); + return transform.toSvg(abs.x, abs.y); + }); + if (pts.length >= 2) { + const d = line.curved ? curvedPath(pts) : straightPath(pts); + if (d) { + overlay += ``; + } + for (const pt of pts) { + overlay += ``; + } + } + } + } + // Line drawing preview if (editorState.lineWaypoints.length > 0 && transform) { const pts = editorState.lineWaypoints.map(p => transform.toSvg(p.x, p.y)); From 9009845be2def6bbe3bb709e9e18017805153048 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 04:45:58 +0000 Subject: [PATCH 2/2] test: add regression tests for the editor bugfixes Covers the four repaired behaviors plus the numbering cap: #1 ball-possession button assigns the ball entity onto the selected player (base position and frame deltas) #2 drawn lines are selectable by click and deletable #3 snap indicator survives the per-move re-render (driven via synthetic mouse events for determinism) #4 right-click selects an entity instead of deleting it + offense/defense numbering stays capped at 9 Each test was verified to fail against the pre-fix code and pass after. Expose editorState / player / getCurrentTransform on the OCFEditor global so tests can drive and inspect real app state through the bundle. --- src/main.js | 9 +- tests/regressions.spec.js | 231 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 238 insertions(+), 2 deletions(-) create mode 100644 tests/regressions.spec.js diff --git a/src/main.js b/src/main.js index 0d612e2..8e8ea08 100644 --- a/src/main.js +++ b/src/main.js @@ -21,11 +21,16 @@ import { exportJSON, importJSON, toJSONString } from './export/json.js'; let currentMode = 'editor'; // 'editor' | 'viewer' let currentTransform = null; +/** Current court transform (court units ↔ viewBox units). Exposed for tests. */ +export function getCurrentTransform() { + return currentTransform; +} + // --- Editor State --- -const editorState = new EditorState({ ruleset: 'fiba', courtType: 'half_court', drillFocus: 'offense' }); +export const editorState = new EditorState({ ruleset: 'fiba', courtType: 'half_court', drillFocus: 'offense' }); // --- Frame Player --- -const player = new FramePlayer({ +export const player = new FramePlayer({ onFrameChange: (idx) => { if (currentMode === 'viewer') renderViewer(); updatePlayerControls(); diff --git a/tests/regressions.spec.js b/tests/regressions.spec.js new file mode 100644 index 0000000..4c0e72a --- /dev/null +++ b/tests/regressions.spec.js @@ -0,0 +1,231 @@ +/** + * Regression tests for the editor bugfixes: + * #1 ball-possession button (context menu) was a no-op + * #2 drawn lines could never be selected / edited / deleted + * #3 snap indicator vanished during drag (wiped by the per-move re-render) + * #4 right-click deleted an entity instead of selecting it + * + offense/defense numbering must stay capped at 9 (schema limit) + * + * These drive the real bundle via the exposed `window.OCFEditor` globals + * (`editorState`, `getCurrentTransform`). + */ + +import { test, expect } from '@playwright/test'; + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +async function openEditor(page) { + await page.goto('/'); + await expect(page.locator('#court-svg rect[fill="#d4a06a"]')).toBeVisible(); +} + +function paletteBtn(page, label) { + return page.locator('.palette-item .pal-label', { hasText: label }).locator('..'); +} + +/** Convert a court coordinate to a client (screen) point over the SVG. + * Mirrors InteractionManager._svgPoint: the handler reads `clientX - rect.left` + * and feeds it straight into the viewBox-based transform, so a viewBox unit maps + * to (box.x + svgX) regardless of any CSS scaling. */ +async function courtToClient(page, courtX, courtY) { + const svg = await page.evaluate(([cx, cy]) => { + const t = window.OCFEditor.getCurrentTransform(); + return t.toSvg(cx, cy); + }, [courtX, courtY]); + const box = await page.locator('#court-svg').boundingBox(); + return { x: box.x + svg.x, y: box.y + svg.y }; +} + +// ─── #1 Ball possession ─────────────────────────────────────────────────────── + +test.describe('Regression #1: ball possession button', () => { + test('clicking Ball assigns the ball entity onto the selected player', async ({ page }) => { + await openEditor(page); + await paletteBtn(page, 'Offense').click(); + + // Select the player → the floating action menu appears. + await page.evaluate(() => window.OCFEditor.editorState.select('offense_1')); + const ballBtn = page.locator('#ctx-btn-ball'); + await expect(ballBtn).toBeVisible(); + + await ballBtn.click(); + + const result = await page.evaluate(() => { + const doc = window.OCFEditor.editorState.doc; + const ball = doc.entities.find(e => e.type === 'ball'); + const player = doc.entities.find(e => e.type === 'offense' && e.nr === 1); + return { ball, player }; + }); + + // A ball entity now exists and sits on the player. + expect(result.ball).toBeTruthy(); + expect(result.ball.x).toBeCloseTo(result.player.x, 5); + expect(result.ball.y).toBeCloseTo(result.player.y, 5); + // And the button reflects possession. + await expect(ballBtn).toHaveClass(/active/); + }); + + test('assigning the ball on a later frame writes a frame delta, not the base', async ({ page }) => { + await openEditor(page); + await paletteBtn(page, 'Offense').click(); + + await page.evaluate(() => { + const s = window.OCFEditor.editorState; + s.addFrame(); // currentFrameIndex -> 1 + s.select('offense_1'); + }); + await page.locator('#ctx-btn-ball').click(); + + const res = await page.evaluate(() => { + const doc = window.OCFEditor.editorState.doc; + return { + delta: doc.frames[1].entity_states?.ball || null, + ballExists: doc.entities.some(e => e.type === 'ball'), + frame0HasDelta: !!doc.frames[0].entity_states, + }; + }); + + expect(res.ballExists).toBe(true); + expect(res.delta).toBeTruthy(); // possession recorded as a frame delta + expect(res.frame0HasDelta).toBe(false); // base frame untouched + }); +}); + +// ─── #2 Line selection / editing / deletion ───────────────────────────────────── + +test.describe('Regression #2: lines are selectable and deletable', () => { + async function seedLine(page) { + await page.evaluate(() => { + const s = window.OCFEditor.editorState; + const doc = JSON.parse(JSON.stringify(s.doc)); + doc.frames[0].lines = [ + { type: 'movement', curved: false, coords: [{ x: -4, y: 6 }, { x: 4, y: 6 }] }, + ]; + s.loadDocument(doc); + }); + } + + test('clicking a drawn line selects it and shows its properties', async ({ page }) => { + await openEditor(page); + await seedLine(page); + + const mid = await courtToClient(page, 0, 6); // midpoint of the line + await page.mouse.click(mid.x, mid.y); + + const selected = await page.evaluate(() => window.OCFEditor.editorState.selectedLineIndex); + expect(selected).toBe(0); + await expect(page.locator('#properties .prop-title')).toHaveText(/Line/); + }); + + test('a selected line can be deleted with the Delete key', async ({ page }) => { + await openEditor(page); + await seedLine(page); + + const mid = await courtToClient(page, 0, 6); + await page.mouse.click(mid.x, mid.y); + await expect.poll(() => + page.evaluate(() => window.OCFEditor.editorState.selectedLineIndex), + ).toBe(0); + + await page.keyboard.press('Delete'); + + const remaining = await page.evaluate( + () => window.OCFEditor.editorState.doc.frames[0].lines.length, + ); + expect(remaining).toBe(0); + }); +}); + +// ─── #3 Snap indicator ────────────────────────────────────────────────────────── + +test.describe('Regression #3: snap indicator survives the per-move re-render', () => { + test('snap indicator stays in the DOM across consecutive snapping moves', async ({ page }) => { + await openEditor(page); + await paletteBtn(page, 'Offense').click(); // fiba default: offense_1 at (0, 7) + + // Drive the real InteractionManager handlers with synthetic mouse events so + // the sequence is deterministic. The handler reads `clientX - rect.left` and + // feeds it straight into the viewBox transform, so a viewBox unit maps to + // (rect.left + svgX). We snap onto two named positions in a row. + const result = await page.evaluate(() => { + const svg = document.getElementById('court-svg'); + const t = window.OCFEditor.getCurrentTransform(); + const rect = svg.getBoundingClientRect(); + const fire = (type, c, buttons) => svg.dispatchEvent(new MouseEvent(type, { + bubbles: true, cancelable: true, view: window, + clientX: rect.left + c.x, clientY: rect.top + c.y, button: 0, buttons, + })); + const isConnected = () => { + const c = svg.querySelector('circle[stroke="#00cc66"]'); + return !!c && c.isConnected; + }; + + const start = t.toSvg(0, 7); // the player + const snap1 = t.toSvg(0, 8.2); // free_throw_line (fiba) + const snap2 = t.toSvg(0, 5.68); // top_of_the_key (fiba) + + fire('mousedown', start, 1); + fire('mousemove', snap1, 1); // first snap → indicator created + appended + const afterFirst = isConnected(); + fire('mousemove', snap2, 1); // second snap → re-render wipes innerHTML + const afterSecond = isConnected(); + fire('mouseup', snap2, 0); + + const player = window.OCFEditor.editorState.doc.entities.find(e => e.type === 'offense'); + return { afterFirst, afterSecond, movedTo: player.y }; + }); + + // Sanity: the drag actually snapped the player onto the named position. + expect(result.movedTo).toBeCloseTo(5.68, 5); + expect(result.afterFirst).toBe(true); + // The regression: before the fix the indicator was detached by the + // per-move svg.innerHTML reset and never re-appended. + expect(result.afterSecond).toBe(true); + }); +}); + +// ─── #4 Right-click ───────────────────────────────────────────────────────────── + +test.describe('Regression #4: right-click selects instead of deleting', () => { + test('right-clicking a player keeps it and selects it', async ({ page }) => { + await openEditor(page); + await paletteBtn(page, 'Offense').click(); + + const before = await page.evaluate( + () => window.OCFEditor.editorState.doc.entities.length, + ); + + const at = await courtToClient(page, 0, 7); // the offense default position + await page.mouse.click(at.x, at.y, { button: 'right' }); + + const after = await page.evaluate(() => ({ + count: window.OCFEditor.editorState.doc.entities.length, + selected: window.OCFEditor.editorState.selectedEntityKey, + })); + + expect(after.count).toBe(before); // not deleted + expect(after.selected).toBe('offense_1'); // selected instead + }); +}); + +// ─── Numbering cap ────────────────────────────────────────────────────────────── + +test.describe('Regression: offense/defense numbering capped at 9', () => { + test('the 10th offense slot is unavailable (null) and cones are unaffected', async ({ page }) => { + await openEditor(page); + const result = await page.evaluate(() => { + const s = window.OCFEditor.editorState; + for (let i = 1; i <= 9; i++) { + s.addEntity({ type: 'offense', nr: s.nextEntityNr('offense'), x: 0, y: i }); + } + return { + count: s.doc.entities.length, + next: s.nextEntityNr('offense'), + cone: s.nextEntityNr('cone'), + }; + }); + expect(result.count).toBe(9); + expect(result.next).toBeNull(); + expect(result.cone).toBe(1); + }); +});