From 19de950cd42108b2d6cd7d22c724629e01cbc7f3 Mon Sep 17 00:00:00 2001 From: P S Kesavan Date: Sun, 6 Sep 2026 07:29:36 +0530 Subject: [PATCH] refactor(ui): one marking surface, invoked two ways MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There were two complete implementations, and the question "why are there two tabs of embryos" is what surfaced it. `static/js/marking.js` — 452 lines with its own canvas, its own hit-test, its own marker list — lived in the Embryos tab behind a "Marking" subtab, and `websocket.js` switched the operator into it when the agent sent a `marking_image` frame. `operate.js` had the other, on the bottom-camera pane, for an operator marking unprompted. So every improvement to marking landed on exactly one of them. The zoom, the display range, #105's hit-test radius and #126's roster aliasing were all on the Operate side and none of it existed on the agent's side. Whether an operator got the corrected behaviour depended on who had asked them to mark. The agent's request lands on the Operate pane. The pushed image is ADAPTED into the payload shape a live camera frame already has — `shape`, `downsample`, `stage_position`, `jpeg_b64` — so `frameOf`, `stageOf`, `drawMarkers`, `onCanvasClick` and the corrected hit-test all apply with no special cases. That is the point: the agent's request inherits zoom, geometry and the display range because it is the same surface, not because anything was reimplemented. Initial markers arrive in pixels and are converted to stage µm once, at the boundary, because this pane keeps markers in stage coordinates so they stay attached to the sample under zoom and stage motion. The answer converts back. `pixel_size_um` is a session parameter rather than the rig default, threaded through the geometry by `pxBase()` — a session declaring a different scale would otherwise place every marker at 0.65 µm/px. It is a websocket request/response, not an HTTP call: the agent blocks on `session["complete"]` in `routes/websocket.py` and reads a role per marker out of the answer. So `marking_done` goes back with the session id, pixel coordinates and a role each, `marking_redetect` still works, and the session panel carries a per-marker role toggle — registered embryos get their roles in the Acquisition roster, but these are not registered yet. A reference is also tagged on the glyph, since it must be distinguishable on the image and not only in the panel. The session UI is present only while the agent is waiting. marking.js, the `embryos-marking` markup, the subtab bar (one subtab left is chrome for a choice that no longer exists), and 222 lines of CSS that became unreachable — 31 rules, removed rather than left to rot. The agent's frame lands on Devices → Operate → Bottom cam with the image shown, the session panel seated, and two initial markers carrying the roles they arrived with. Cycling a role, asking for a re-detect and pressing Done all answer over the websocket, and pixel coordinates round-trip exactly: (100,80) and (220,140) in, the same out. And the claim that motivated it — zoomed to 1.75x, clicking the frame centre put a marker back at exactly (200,150), the frame centre, with the display histogram available on the pushed image too. Neither was possible on the old surface. Co-Authored-By: Claude Opus 5 (1M context) --- gently/ui/web/static/css/main.css | 222 ----------- gently/ui/web/static/css/operate.css | 48 +++ gently/ui/web/static/js/marking.js | 452 ---------------------- gently/ui/web/static/js/operate.js | 173 ++++++++- gently/ui/web/static/js/panels/marking.js | 42 +- gently/ui/web/static/js/websocket.js | 18 +- gently/ui/web/templates/index.html | 47 +-- tests/test_one_marking_surface.py | 123 ++++++ 8 files changed, 393 insertions(+), 732 deletions(-) delete mode 100644 gently/ui/web/static/js/marking.js create mode 100644 tests/test_one_marking_surface.py diff --git a/gently/ui/web/static/css/main.css b/gently/ui/web/static/css/main.css index fdc9c821..798025d7 100644 --- a/gently/ui/web/static/css/main.css +++ b/gently/ui/web/static/css/main.css @@ -3860,39 +3860,11 @@ kbd { background: var(--bg-card); } -/* ======================================== - EMBRYOS TAB - Subtab Bar - ======================================== */ - -.embryos-subtab-bar { - display: none; /* Merged into embryo rail */ -} - -.embryos-subtab { - padding: 0.6rem 1.2rem; - cursor: pointer; - color: var(--text-muted); - font-size: 0.85rem; - font-weight: 500; - border-bottom: 2px solid transparent; - transition: color 0.2s, border-color 0.2s; - user-select: none; -} - -.embryos-subtab:hover { - color: var(--text); -} - .embryos-subtab.active { color: var(--accent); border-bottom-color: var(--accent); } -.embryos-subtab .marking-badge { - font-size: 0.7rem; - margin-left: 0.3rem; -} - /* ======================================== EMBRYOS TAB - Marking ======================================== */ @@ -3904,41 +3876,6 @@ kbd { min-height: 0; } -.marking-placeholder { - flex: 1; - display: flex; - flex-direction: column; - align-items: center; - justify-content: center; - gap: 0.5rem; - color: var(--text-muted); -} - -.marking-placeholder-icon { - font-size: 2.5rem; - opacity: 0.4; -} - -.marking-placeholder-text { - font-size: 1.1rem; - font-weight: 500; -} - -.marking-placeholder-hint { - font-size: 0.8rem; - max-width: 360px; - text-align: center; - line-height: 1.5; -} - -.marking-active { - flex: 1; - display: flex; - gap: 1rem; - padding: 1rem 1.5rem; - min-height: 0; -} - .marking-viewer { flex: 1; display: flex; @@ -3957,124 +3894,6 @@ kbd { margin-bottom: 0.5rem; } -.marking-instructions { - font-size: 0.85rem; - color: var(--text-muted); -} - -.marking-actions { - display: flex; - gap: 0.5rem; -} - -.marking-action-btn { - padding: 0.35rem 0.8rem; - border: 1px solid var(--border); - border-radius: 6px; - background: var(--bg-card); - color: var(--text); - cursor: pointer; - font-size: 0.8rem; - transition: background 0.15s; -} - -.marking-action-btn:hover:not(:disabled) { - background: var(--bg-hover); -} - -.marking-action-btn:disabled { - opacity: 0.4; - cursor: default; -} - -.marking-done-btn { - background: var(--accent); - color: #fff; - border-color: var(--accent); -} - -.marking-done-btn:hover:not(:disabled) { - filter: brightness(1.1); - background: var(--accent); -} - -.marking-image-container { - flex: 1 1 auto; - position: relative; - background: #111; - border-radius: 8px; - overflow: hidden; - display: flex; - align-items: center; - justify-content: center; - /* min-height was 300px which prevented flex-shrink when the parent - (#embryos-marking → .marking-container → .marking-active → - .marking-viewer) lacked a bounded height. Use 0 + a reasonable - basis so the container shrinks correctly inside its flex chain. */ - min-height: 0; -} - -.marking-image-container img { - max-width: 100%; - max-height: 100%; - object-fit: contain; -} - -.marking-image-container canvas { - position: absolute; - top: 50%; - left: 50%; - transform: translate(-50%, -50%); - cursor: crosshair; - pointer-events: auto; -} - -/* Marking sidebar */ -.marking-sidebar { - width: 220px; - flex-shrink: 0; - display: flex; - flex-direction: column; - background: var(--bg-card); - border: 1px solid var(--border); - border-radius: 8px; - overflow: hidden; -} - -.marking-sidebar-header { - padding: 0.6rem 0.75rem; - font-size: 0.85rem; - font-weight: 600; - border-bottom: 1px solid var(--border); - color: var(--text); -} - -.marking-list { - flex: 1; - overflow-y: auto; - padding: 0.5rem; -} - -.marking-list-empty { - text-align: center; - color: var(--text-muted); - font-size: 0.8rem; - padding: 1.5rem 0.5rem; -} - -.marking-list-item { - display: flex; - align-items: center; - gap: 0.5rem; - padding: 0.4rem 0.5rem; - border-radius: 6px; - font-size: 0.8rem; -} - -.marking-list-item:hover { - background: var(--bg-hover); -} - .marking-number { width: 22px; height: 22px; @@ -4096,47 +3915,6 @@ kbd { font-size: 0.75rem; } -.marking-remove-btn { - border: none; - background: none; - color: var(--text-muted); - cursor: pointer; - font-size: 1rem; - line-height: 1; - padding: 0 0.2rem; - opacity: 0; - transition: opacity 0.15s; -} - -.marking-list-item:hover .marking-remove-btn { - opacity: 1; -} - -.marking-remove-btn:hover { - color: #ff6b6b; -} - -/* Role chip on each marker — click cycles test ↔ calibration ↔ unassigned. - Inline styles set the role-specific colors (magenta/cyan/grey); this - gives the chip its shape, cursor, and hover affordance. */ -.marking-role-chip { - padding: 2px 8px; - border-radius: 10px; - font-size: 0.7rem; - font-weight: 600; - letter-spacing: 0.02em; - cursor: pointer; - user-select: none; - transition: filter 0.15s, transform 0.1s; - flex-shrink: 0; -} -.marking-role-chip:hover { - filter: brightness(1.2) saturate(1.1); -} -.marking-role-chip:active { - transform: scale(0.96); -} - /* Source tag (sam / manual / existing) — quieter than the role chip. */ .marking-source { font-size: 0.65rem; diff --git a/gently/ui/web/static/css/operate.css b/gently/ui/web/static/css/operate.css index b79975c2..3b2d8f60 100644 --- a/gently/ui/web/static/css/operate.css +++ b/gently/ui/web/static/css/operate.css @@ -1093,3 +1093,51 @@ opacity: 0.45; cursor: default; } + +/* ── Agent-initiated marking session ───────────────────────────────────── + Present only while the agent is waiting on an answer, so it seats and + retires with the session rather than standing there empty. */ +.mk-session { + margin-top: 8px; + padding-top: 8px; + border-top: 1px solid var(--op-rule); + grid-column: 1 / -1; +} + +.mk-shead { + font-size: 0.62rem; + font-weight: 600; + letter-spacing: 0.10em; + text-transform: uppercase; + color: var(--accent); + margin-bottom: 6px; +} + +/* One row per pending marker: the contract carries a role for each, so each + needs to be settable before the answer goes back. */ +.mk-prow { + display: flex; + align-items: center; + gap: 8px; + padding: 2px 0; +} + +.mk-pnum { + flex: 0 0 18px; + font-family: 'SF Mono', Monaco, 'Cascadia Code', monospace; + font-size: 0.72rem; + color: var(--op-ink); +} + +.mk-psrc { + flex: 1 1 auto; + font-size: 0.64rem; + color: var(--op-ink-dim); +} + +.mk-prole.is-reference { + border-color: var(--accent); + color: var(--accent); +} + +.mk-session .mk-acts { margin-top: 8px; } diff --git a/gently/ui/web/static/js/marking.js b/gently/ui/web/static/js/marking.js deleted file mode 100644 index 1d21b19f..00000000 --- a/gently/ui/web/static/js/marking.js +++ /dev/null @@ -1,452 +0,0 @@ -/** - * Map view — embryo detection, marking, role assignment. - * - * Single spatial GUI: SAM-detected markers arrive pre-placed and editable; - * user can add/remove markers, toggle each between Test (magenta) and - * Calibration (cyan), and click "Re-detect" to recapture + re-run SAM. - * Replaces the deprecated napari-based marker. - * - * WS message types: - * incoming marking_image — image + initial markers + default role - * outgoing embryo_marked — single new marker added (clicked) - * outgoing marking_update — full marker list (after edit/role-cycle) - * outgoing marking_done — finalize + commit roles - * outgoing marking_redetect — recapture + re-run SAM, replace markers - */ - -const ROLE_CYCLE = ['test', 'calibration', 'unassigned']; -const ROLE_COLORS = { - test: '#ff66cc', // magenta — biological subject (precious) - calibration: '#00cccc', // cyan — reference / staging anchor - unassigned: '#888888', // grey — not yet classified -}; -const ROLE_LABEL = { - test: 'Test', - calibration: 'Cal', - unassigned: '—', -}; - -const MarkingManager = { - // State - active: false, - markers: [], // [{ number, pixelX, pixelY, role, source, embryo_id?, confidence?, timestamp }] - imageWidth: 0, - imageHeight: 0, - sessionId: null, - defaultRole: 'test', - - // Coordinate / overlay state - // Capture-time stage position of the marking image, NOT the live stage. - // The image is a still; its centre is this position by construction, and - // the server converts markers with the same `initial_stage_position`. - // Do not subscribe these to SharedState 'stageXY' — a live value would - // slide the coverslip overlay off a static image. - stageXUm: 0, - stageYUm: 0, - umPerPixel: 0.65, // effective um per pixel on sample - coverslip: null, // {center_um: [x,y], size_mm: [w,h]} from /api/devices/coverslip - - // DOM refs (set in init) - canvas: null, - img: null, - container: null, - listEl: null, - - init() { - this.canvas = document.getElementById('marking-canvas'); - this.img = document.getElementById('marking-image'); - this.container = document.getElementById('marking-image-container'); - this.listEl = document.getElementById('marking-list'); - - if (!this.canvas || !this.img) return; - - this.canvas.addEventListener('click', (e) => this._onCanvasClick(e)); - - this.img.addEventListener('load', () => { - this._syncCanvasSize(); - this._redraw(); - }); - - new ResizeObserver(() => { - this._syncCanvasSize(); - this._redraw(); - }).observe(this.container); - - // Load coverslip outline once (small static metadata) - fetch('/api/devices/coverslip') - .then(r => r.ok ? r.json() : null) - .then(d => { if (d && d.coverslip) this.coverslip = d.coverslip; }) - .catch(() => { /* coverslip overlay is optional */ }); - }, - - // Called when server sends a marking_image message - handleMarkingImage(data) { - this.sessionId = data.session_id; - this.imageWidth = data.width; - this.imageHeight = data.height; - this.defaultRole = data.default_role || 'test'; - this.stageXUm = data.stage_x_um != null ? data.stage_x_um : 0; - this.stageYUm = data.stage_y_um != null ? data.stage_y_um : 0; - this.umPerPixel = data.pixel_size_um || 0.65; - - // Auto-switch to the marking subtab so the user doesn't have to - // hunt for it when a session starts. - try { this.switchSubtab('marking'); } catch (_) { /* tabs not ready */ } - - // Hydrate any initial markers (e.g. SAM detections) — already - // normalized server-side to {number, pixelX, pixelY, role, source, ...}. - this.markers = (data.initial_markers || []).map(m => ({ - number: m.number, - pixelX: m.pixelX, - pixelY: m.pixelY, - role: m.role || this.defaultRole, - source: m.source || 'sam', - embryo_id: m.embryo_id || null, - confidence: m.confidence != null ? m.confidence : null, - timestamp: m.timestamp || new Date().toISOString(), - })); - this.active = true; - - this.img.src = 'data:image/png;base64,' + data.image_b64; - this.img.style.display = 'block'; - - const placeholder = document.getElementById('marking-placeholder'); - const activeEl = document.getElementById('marking-active'); - if (placeholder) placeholder.style.display = 'none'; - if (activeEl) activeEl.style.display = 'flex'; - - const instructions = document.getElementById('marking-instructions'); - if (instructions) { - const n = this.markers.length; - instructions.textContent = n > 0 - ? `${n} marker(s) loaded. Click to add, click a role chip to cycle Test/Calibration, press Done when finished.` - : 'Click on each embryo center. Click a role chip to switch Test/Calibration. Press Done when finished.'; - } - - // Re-enable action buttons in case a previous session disabled them. - document.querySelectorAll('.marking-actions .marking-action-btn').forEach(btn => btn.disabled = false); - - this._renderList(); - }, - - // Bench hook: a CSS transform on an ancestor fires neither resize nor - // ResizeObserver, so the bench calls this after unfolding or rescaling. - refresh() { - this._syncCanvasSize(); - this._redraw(); - }, - - _syncCanvasSize() { - if (!this.img || !this.canvas || !this.imageWidth || !this.imageHeight) return; - - // Layout size, not getBoundingClientRect(): the rect carries every - // ancestor transform, which would oversize the backing store and the - // CSS box by the bench scale. - const containerStyle = getComputedStyle(this.container); - const availW = parseFloat(containerStyle.width); - const availH = parseFloat(containerStyle.height); - // Zero/NaN means folded or hidden — keep the current size instead of - // collapsing the canvas; refresh() restores it on unfold. - if (!(availW > 0) || !(availH > 0)) return; - - const imgAspect = this.imageWidth / this.imageHeight; - const containerAspect = availW / availH; - - let renderW, renderH; - if (imgAspect > containerAspect) { - renderW = availW; - renderH = availW / imgAspect; - } else { - renderH = availH; - renderW = availH * imgAspect; - } - - this.canvas.width = renderW; - this.canvas.height = renderH; - this.canvas.style.width = renderW + 'px'; - this.canvas.style.height = renderH + 'px'; - }, - - _onCanvasClick(e) { - if (!this.active) return; - - const rect = this.canvas.getBoundingClientRect(); - if (!rect.width || !rect.height) return; - const canvasX = e.clientX - rect.left; - const canvasY = e.clientY - rect.top; - - // Scale from the measured rect, not the backing store: the rect is - // transform-inclusive and clientX/clientY are too, so mixing the two - // would ship stage coordinates off by the bench scale. - const scaleX = this.imageWidth / rect.width; - const scaleY = this.imageHeight / rect.height; - const pixelX = canvasX * scaleX; - const pixelY = canvasY * scaleY; - - const marker = { - number: this.markers.length + 1, - pixelX: Math.round(pixelX * 10) / 10, - pixelY: Math.round(pixelY * 10) / 10, - role: this.defaultRole, - source: 'manual', - embryo_id: null, - confidence: null, - timestamp: new Date().toISOString(), - }; - this.markers.push(marker); - - this._send('embryo_marked', { marker }); - - this._redraw(); - this._renderList(); - }, - - _redraw() { - if (!this.canvas) return; - const ctx = this.canvas.getContext('2d'); - const w = this.canvas.width; - const h = this.canvas.height; - ctx.clearRect(0, 0, w, h); - - if (!this.active) return; - - const scaleX = w / this.imageWidth; - const scaleY = h / this.imageHeight; - - // Coverslip outline (drawn first so markers sit on top). - // The coverslip is much larger than the FOV; we draw it relative to - // the current stage position so the user sees where in the slide - // they're looking. - if (this.coverslip && this.umPerPixel > 0) { - const csCx = (this.coverslip.center_um && this.coverslip.center_um[0]) || 0; - const csCy = (this.coverslip.center_um && this.coverslip.center_um[1]) || 0; - const csW = ((this.coverslip.size_mm && this.coverslip.size_mm[0]) || 0) * 1000; - const csH = ((this.coverslip.size_mm && this.coverslip.size_mm[1]) || 0) * 1000; - if (csW > 0 && csH > 0) { - // Coverslip extents in stage µm - const x0um = csCx - csW / 2; - const y0um = csCy - csH / 2; - const x1um = csCx + csW / 2; - const y1um = csCy + csH / 2; - // Map each corner to pixel coords (image center IS stage_*Um) - const imgCx = this.imageWidth / 2; - const imgCy = this.imageHeight / 2; - const toPxX = (xum) => (imgCx + (xum - this.stageXUm) / this.umPerPixel) * scaleX; - const toPxY = (yum) => (imgCy + (yum - this.stageYUm) / this.umPerPixel) * scaleY; - const left = toPxX(x0um); - const top = toPxY(y0um); - const right = toPxX(x1um); - const bottom = toPxY(y1um); - ctx.save(); - ctx.strokeStyle = 'rgba(255, 220, 0, 0.55)'; - ctx.setLineDash([6, 6]); - ctx.lineWidth = 1.5; - ctx.strokeRect(left, top, right - left, bottom - top); - ctx.setLineDash([]); - ctx.restore(); - } - } - - // Stage-center crosshair (current XY) — image center IS the current - // stage position by construction. Subtle so it doesn't fight markers. - { - const cx = (this.imageWidth / 2) * scaleX; - const cy = (this.imageHeight / 2) * scaleY; - const r = Math.max(6, Math.min(w, h) * 0.008); - ctx.save(); - ctx.strokeStyle = 'rgba(255, 255, 255, 0.4)'; - ctx.lineWidth = 1; - ctx.beginPath(); - ctx.moveTo(cx - r, cy); ctx.lineTo(cx + r, cy); - ctx.moveTo(cx, cy - r); ctx.lineTo(cx, cy + r); - ctx.stroke(); - ctx.restore(); - } - - if (this.markers.length === 0) return; - - for (const m of this.markers) { - const x = m.pixelX * scaleX; - const y = m.pixelY * scaleY; - const size = Math.max(10, Math.min(w, h) * 0.015); - const color = ROLE_COLORS[m.role] || ROLE_COLORS.test; - - // Crosshair (role-colored) - ctx.strokeStyle = color; - ctx.lineWidth = 2; - ctx.beginPath(); - ctx.moveTo(x - size, y); ctx.lineTo(x + size, y); - ctx.moveTo(x, y - size); ctx.lineTo(x, y + size); - ctx.stroke(); - - // Circle (role-colored) - ctx.beginPath(); - ctx.arc(x, y, size * 2, 0, Math.PI * 2); - ctx.stroke(); - - // Number label above - ctx.font = `bold ${Math.max(12, size)}px sans-serif`; - ctx.fillStyle = color; - ctx.textAlign = 'center'; - ctx.fillText(m.number, x, y - size * 2.5); - - // Role short label below - ctx.font = `${Math.max(10, size * 0.85)}px sans-serif`; - ctx.fillText(ROLE_LABEL[m.role] || m.role, x, y + size * 3); - } - }, - - _renderList() { - if (!this.listEl) return; - - if (this.markers.length === 0) { - this.listEl.innerHTML = '
No embryos marked yet
'; - this._updateCount(0); - return; - } - - this.listEl.innerHTML = this.markers.map(m => { - const color = ROLE_COLORS[m.role] || ROLE_COLORS.test; - const label = ROLE_LABEL[m.role] || m.role; - const src = m.source ? `${m.source}` : ''; - return `
- ${m.number} - - ${src} - (${m.pixelX}, ${m.pixelY}) - -
`; - }).join(''); - - this._updateCount(this.markers.length); - }, - - _updateCount(n) { - const countEl = document.getElementById('marking-count'); - if (countEl) { - countEl.textContent = n; - countEl.style.display = n > 0 ? '' : 'none'; - } - }, - - cycleRole(number) { - if (!this.active) return; - const m = this.markers.find(x => x.number === number); - if (!m) return; - - const idx = ROLE_CYCLE.indexOf(m.role); - const next = ROLE_CYCLE[(idx + 1) % ROLE_CYCLE.length]; - m.role = next; - - this._send('marking_update', { markers: this.markers }); - this._redraw(); - this._renderList(); - }, - - removeMarker(number) { - this.markers = this.markers.filter(m => m.number !== number); - // Renumber so labels stay 1..N - this.markers.forEach((m, i) => m.number = i + 1); - - this._send('marking_update', { markers: this.markers }); - - this._redraw(); - this._renderList(); - }, - - clearAll() { - if (!this.active) return; - if (this.markers.length > 0 && !confirm('Clear all marked embryos?')) return; - - this.markers = []; - this._send('marking_update', { markers: [] }); - - this._redraw(); - this._renderList(); - }, - - redetect() { - if (!this.active) return; - if (this.markers.length > 0 && !confirm('Recapture image and re-run SAM detection? Current markers will be replaced.')) return; - - this._send('marking_redetect', {}); - - const instructions = document.getElementById('marking-instructions'); - if (instructions) { - instructions.textContent = 'Recapturing and re-running detection…'; - } - }, - - done() { - if (!this.active) return; - - if (this.markers.length === 0) { - if (!confirm('No embryos marked. Finish anyway?')) return; - } - - this._send('marking_done', { markers: this.markers }); - - this.active = false; - const counts = this.markers.reduce((acc, m) => { - acc[m.role] = (acc[m.role] || 0) + 1; - return acc; - }, {}); - const summary = Object.entries(counts) - .map(([r, n]) => `${n} ${r}`) - .join(', '); - const instructions = document.getElementById('marking-instructions'); - if (instructions) { - instructions.textContent = `Marking complete — ${this.markers.length} embryo(s)${summary ? ': ' + summary : ''}.`; - } - - document.querySelectorAll('.marking-actions .marking-action-btn').forEach(btn => btn.disabled = true); - - // Auto-switch back to monitoring after the user sees the - // "marking complete" confirmation. Without this the marker - // window stays put even after the agent has started a - // timelapse, and the user has to manually switch tabs. - setTimeout(() => { - try { this.switchSubtab('monitoring'); } catch (_) { /* tabs may be gone */ } - // Reset placeholder/active visibility so the next session - // starts fresh on this tab. - const placeholder = document.getElementById('marking-placeholder'); - const activeEl = document.getElementById('marking-active'); - if (placeholder) placeholder.style.display = ''; - if (activeEl) activeEl.style.display = 'none'; - this.markers = []; - this._updateCount(0); - this._redraw(); - this._renderList(); - }, 1500); - }, - - _send(type, payload) { - if (!state.ws || state.ws.readyState !== WebSocket.OPEN) return; - state.ws.send(JSON.stringify({ - type, - session_id: this.sessionId, - ...payload, - })); - }, - - // Switch between monitoring and marking subtabs - switchSubtab(subtab) { - document.querySelectorAll('.embryos-subtab').forEach(t => t.classList.remove('active')); - const tab = document.querySelector(`.embryos-subtab[data-subtab="${subtab}"]`); - if (tab) tab.classList.add('active'); - - const monitoring = document.getElementById('embryos-monitoring'); - const marking = document.getElementById('embryos-marking'); - if (monitoring) monitoring.style.display = subtab === 'monitoring' ? '' : 'none'; - if (marking) marking.style.display = subtab === 'marking' ? '' : 'none'; - } -}; - -if (document.readyState === 'loading') { - document.addEventListener('DOMContentLoaded', () => MarkingManager.init()); -} else { - MarkingManager.init(); -} diff --git a/gently/ui/web/static/js/operate.js b/gently/ui/web/static/js/operate.js index c8634002..e09a9960 100644 --- a/gently/ui/web/static/js/operate.js +++ b/gently/ui/web/static/js/operate.js @@ -100,6 +100,29 @@ const OperateManager = (function () { // has always done — a plain click must never silently narrow a timelapse // from every subject to one, so the narrowing is a thing you say. let _targetScope = 'all'; + + // ── AGENT-INITIATED MARKING ───────────────────────────────────────────── + // The agent can ask the operator to mark embryos on an image it pushes + // (server.start_marking_session -> a `marking_image` websocket frame, and + // it waits for `marking_done` keyed by session_id). + // + // That request used to land on a SECOND marking implementation — + // static/js/marking.js, 452 lines with its own canvas, its own hit-test + // and its own list, in the Embryos tab. So every fix to marking landed on + // one of two surfaces: the zoom, the display range, #105's hit-test and + // #126's aliasing all existed here and none of them there. Whether an + // operator got the corrected behaviour depended on who had asked them to + // mark. + // + // One surface now, two invocations. The pushed image is adapted into the + // frame shape this pane already understands, so all of that geometry + // applies unchanged. + let _markSession = null; + + /** The µm/px this session declares, or the rig default. */ + function pxBase() { + return _markSession ? _markSession.pixelSizeUm : undefined; + } // Read back from the Light panel's device read, never remembered here. const ledIsOpen = () => (SharedState.get('light') || {}).led === 'Open'; let _galvo = 0.0, _piezo = 50.0; @@ -610,7 +633,7 @@ const OperateManager = (function () { function markerToCanvas(m, r) { const f = frameOf(_lastBottom), cap = stageOf(_lastBottom); if (!f || !cap || !M) return null; - const px = M.stageToFrame(m.stageX, m.stageY, f, cap); + const px = M.stageToFrame(m.stageX, m.stageY, f, cap, pxBase()); if (!px) return null; return { cx: r.x + (px[0] / r.fw) * r.w, cy: r.y + (px[1] / r.fh) * r.h, px }; } @@ -623,7 +646,7 @@ const OperateManager = (function () { _embryos.forEach(emb => { const xy = resolveXY(emb); if (!xy) return; - const px = M.stageToFrame(xy.x, xy.y, f, cap); + const px = M.stageToFrame(xy.x, xy.y, f, cap, pxBase()); if (!px) return; out.push({ emb, cx: r.x + (px[0] / r.fw) * r.w, cy: r.y + (px[1] / r.fh) * r.h }); }); @@ -672,7 +695,11 @@ const OperateManager = (function () { ctx.stroke(); ctx.fillStyle = colour; ctx.font = '600 11px Inter Tight, sans-serif'; - ctx.fillText(String(i + 1), cx + 13, cy - 8); + // During an agent-initiated session each marker carries a role that + // goes back in the answer, so a reference must be distinguishable + // on the image and not only in the panel. + const tag = (_markSession && m.role === 'calibration') ? `${i + 1}·ref` : String(i + 1); + ctx.fillText(tag, cx + 13, cy - 8); ctx.restore(); }); } @@ -716,7 +743,7 @@ const OperateManager = (function () { if (!cap) { toastFail('Stage position unknown — wait for the readout, then mark'); return; } const fx = ((cxv - r.x) / r.w) * r.fw, fy = ((cyv - r.y) / r.h) * r.fh; - const s = M && M.frameToStage(fx, fy, f, cap); + const s = M && M.frameToStage(fx, fy, f, cap, pxBase()); if (!s) { toastFail('Cannot place a marker without a stage position'); return; } _markers.push({ stageX: s[0], stageY: s[1], source: 'manual' }); drawMarkers(); renderMarkCount(); @@ -758,6 +785,16 @@ const OperateManager = (function () { detecting: _detecting, startedAt: _detectStartedAt, note: _markNote, + // Present only while the agent is waiting on an answer. The panel + // renders the pending markers and their roles from this, because + // the contract carries a role per marker. + session: _markSession ? { + pending: _markers.map((m, i) => ({ + index: i, + role: m.role || _markSession.defaultRole, + source: m.source || 'manual', + })), + } : null, }); } @@ -834,7 +871,7 @@ const OperateManager = (function () { cands.forEach(c => { let sx = c.stage_x_um, sy = c.stage_y_um; if ((sx == null || sy == null) && f && cap && M && c.pixel_x != null && c.pixel_y != null) { - const s = M.frameToStage(c.pixel_x / f.downsample, c.pixel_y / f.downsample, f, cap); + const s = M.frameToStage(c.pixel_x / f.downsample, c.pixel_y / f.downsample, f, cap, pxBase()); if (s) { sx = s[0]; sy = s[1]; } } if (sx == null || sy == null) return; @@ -869,7 +906,7 @@ const OperateManager = (function () { // the localiser that will replace SAM. Pixel coords are projected // from stage space against the frame being submitted. const markers = _markers.map(m => { - const px = (f && M) ? M.stageToFrame(m.stageX, m.stageY, f, cap) : null; + const px = (f && M) ? M.stageToFrame(m.stageX, m.stageY, f, cap, pxBase()) : null; return { stage_x_um: m.stageX, stage_y_um: m.stageY, pixel_x: px ? px[0] : undefined, pixel_y: px ? px[1] : undefined, @@ -1491,6 +1528,124 @@ const OperateManager = (function () { el.textContent = bits.join(' · '); } + /** + * The agent is asking the operator to mark embryos on an image it captured. + * + * Adapts the pushed image into the same payload shape a live bottom-camera + * frame has, so `frameOf`/`stageOf`/`drawMarkers`/`onCanvasClick` and the + * corrected hit-test all work on it with no special cases. The one thing + * that differs is the scale: the session declares its own `pixel_size_um`, + * which `pxBase()` threads into the geometry. + */ + function onMarkingImage(d) { + if (!d || !d.image_b64) return; + + _markSession = { + sessionId: d.session_id, + defaultRole: d.default_role || 'test', + pixelSizeUm: d.pixel_size_um || undefined, + }; + + // A pushed still, in the shape a live frame arrives in. `mime` matters: + // start_marking_session sends PNG, the camera stream sends JPEG. + _lastBottom = { + t: Date.now(), + shape: [d.height, d.width], + downsample: 1, + stage_position: [d.stage_x_um != null ? d.stage_x_um : 0, + d.stage_y_um != null ? d.stage_y_um : 0], + mime: 'image/png', + jpeg_b64: d.image_b64, + }; + + const f = frameOf(_lastBottom), cap = stageOf(_lastBottom); + // Initial markers arrive in PIXEL coordinates; this pane keeps markers + // in stage µm so they stay attached to the sample under zoom and stage + // motion. Convert once, here, at the boundary. + _markers = (d.initial_markers || []).map(m => { + const st = (f && cap && M) ? M.frameToStage(m.pixelX, m.pixelY, f, cap, pxBase()) : null; + return st ? { + stageX: st[0], stageY: st[1], + source: m.source || 'sam', + role: m.role || _markSession.defaultRole, + embryo_id: m.embryo_id || null, + confidence: m.confidence != null ? m.confidence : null, + } : null; + }).filter(Boolean); + + // Bring the operator to the surface rather than expecting them to find + // it. The old implementation did this too, and it is right: the agent + // is blocked waiting on an answer. + if (typeof switchTab === 'function') switchTab('devices'); + showPane('bottom'); + setImg('op-img-bottom', 'op-ph-bottom', _lastBottom); + drawMarkers(); + renderMarkCount(); + setDetectNote(_markers.length + ? `${_markers.length} detected — the agent is waiting. Adjust them, set roles, then Done.` + : 'The agent is waiting for you to mark the embryos. Click each one, then Done.'); + } + + /** Answer the agent. Markers go back in pixel coordinates, with roles. */ + function finishMarkingSession() { + if (!_markSession) return; + const f = frameOf(_lastBottom), cap = stageOf(_lastBottom); + const markers = _markers.map((m, i) => { + const px = (f && cap && M) ? M.stageToFrame(m.stageX, m.stageY, f, cap, pxBase()) : null; + return px ? { + number: i + 1, + pixelX: Math.round(px[0]), + pixelY: Math.round(px[1]), + role: m.role || _markSession.defaultRole, + source: m.source || 'manual', + embryo_id: m.embryo_id || null, + confidence: m.confidence != null ? m.confidence : null, + timestamp: new Date().toISOString(), + } : null; + }).filter(Boolean); + + const sent = sendWs({ type: 'marking_done', session_id: _markSession.sessionId, markers }); + if (!sent) { + toastFail('Not connected — the agent did not get your marks'); + return; + } + const n = markers.length; + _markSession = null; + _markers = []; + drawMarkers(); + renderMarkCount(); + setDetectNote(`Marking complete — ${n} embryo${n === 1 ? '' : 's'} sent to the agent.`); + publishMarking(); + } + + /** Ask the agent to recapture and re-detect. */ + function redetectMarkingSession() { + if (!_markSession) return; + if (!sendWs({ type: 'marking_redetect', session_id: _markSession.sessionId })) { + toastFail('Not connected — could not ask for a re-detect'); + return; + } + setDetectNote('Asked the agent to recapture and re-detect…'); + } + + /** Cycle one pending marker between subject and reference. */ + function cycleMarkerRole(index) { + const m = _markers[index]; + if (!m || !_markSession) return; + m.role = m.role === 'calibration' ? 'test' : 'calibration'; + drawMarkers(); + publishMarking(); + } + + // The marking contract is a websocket request/response keyed on + // session_id, not an HTTP call — the agent is blocked on `complete`. + function sendWs(payload) { + const ws = (typeof state !== 'undefined' && state) ? state.ws : null; + if (!ws || ws.readyState !== WebSocket.OPEN) return false; + ws.send(JSON.stringify(payload)); + return true; + } + // ══ EVENTS ══════════════════════════════════════════════════════════════ function onBottomFrame(p) { // Bail when hidden, or a hidden Operate keeps base64-decoding every @@ -1643,6 +1798,8 @@ const OperateManager = (function () { // Server-pushed stills (focus montages today). websocket.js emits // this for every image; onPushedImage takes the kinds this pane owns. ClientEventBus.on('IMAGE_RECEIVED', onPushedImage); + // The agent asking for marks. One surface, two invocations. + ClientEventBus.on('MARKING_IMAGE', onMarkingImage); ClientEventBus.on('DEVICE_STATE_UPDATE', p => { const pos = p && p.positions; if (!pos) return; @@ -1722,6 +1879,10 @@ const OperateManager = (function () { detect: () => runDetect(), register: () => confirmMarks(), clear: () => clearMarks(), + // Session verbs — only meaningful while the agent is waiting. + done: () => finishMarkingSession(), + redetect: () => redetectMarkingSession(), + cycleRole: i => cycleMarkerRole(Number(i)), }, }; })(); diff --git a/gently/ui/web/static/js/panels/marking.js b/gently/ui/web/static/js/panels/marking.js index eee06838..db347446 100644 --- a/gently/ui/web/static/js/panels/marking.js +++ b/gently/ui/web/static/js/panels/marking.js @@ -111,12 +111,51 @@ const MarkingPanel = (() => { >Clear + ${session(s)} ${s.note ? `

${escape(s.note)}

` : ''} `; wire(el); }); } + /** + * The agent-initiated session, when there is one. + * + * Present only while the agent is waiting — this is the one part of the + * panel that is a transient condition rather than a standing control, so it + * seats and retires itself (PANELS.md rule 6). + * + * The per-marker role list exists because the contract needs it: + * `marking_done` carries a role per marker and the waiting agent reads + * them, so the operator has to be able to say which of these is a + * reference before answering. Registered embryos get their roles in the + * Acquisition roster; these are not registered yet. + */ + function session(s) { + if (!s.session) return ''; + const rows = (s.session.pending || []).map(m => { + const ref = m.role === 'calibration'; + return `
+ ${m.index + 1} + ${escape(m.source)} + +
`; + }).join(''); + + return `
+
The agent is waiting
+ ${rows || '

Nothing marked yet — click each embryo on the image.

'} +
+ + +
+
`; + } + function escape(t) { return String(t).replace(/[&<>"]/g, c => ({ '&': '&', '<': '<', '>': '>', '"': '"' }[c])); @@ -128,7 +167,8 @@ const MarkingPanel = (() => { const v = verbs(); if (!v) return; const fn = v[b.dataset.act]; - if (typeof fn === 'function') fn(); + // `data-index` is only present on the per-marker role toggles. + if (typeof fn === 'function') fn(b.dataset.index); }; }); } diff --git a/gently/ui/web/static/js/websocket.js b/gently/ui/web/static/js/websocket.js index 8b0fca54..4d94c65d 100644 --- a/gently/ui/web/static/js/websocket.js +++ b/gently/ui/web/static/js/websocket.js @@ -131,14 +131,16 @@ function handleMessage(msg) { } else if (msg.type === 'timelapse_state') { ClientEventBus.emit('TIMELAPSE_STATE', msg.data); } else if (msg.type === 'marking_image') { - // Server is requesting embryo marking - if (typeof MarkingManager !== 'undefined') { - MarkingManager.handleMarkingImage(msg.data); - // Auto-switch to marking subtab - MarkingManager.switchSubtab('marking'); - // Switch to embryos tab if not already there - if (state.tab !== 'embryos') switchTab('embryos'); - } + // The agent is asking the operator to mark embryos on an image it + // captured, and is blocked until `marking_done` comes back. + // + // This used to drive a second marking implementation in the Embryos + // tab (static/js/marking.js) with its own canvas and hit-test, so every + // improvement to marking landed on only one of two surfaces. It goes to + // the Operate bottom-camera pane now — the same surface an operator + // marks on unprompted, which means the agent's request inherits the + // zoom, the display range and the corrected hit-test. + ClientEventBus.emit('MARKING_IMAGE', msg.data); } else if (msg.type === 'open_volume') { // The agent asked us to open the in-browser volume viewer — the // web-native replacement for the old desktop napari window. diff --git a/gently/ui/web/templates/index.html b/gently/ui/web/templates/index.html index 3f9e830f..5394a006 100644 --- a/gently/ui/web/templates/index.html +++ b/gently/ui/web/templates/index.html @@ -310,14 +310,10 @@

Calibration

- -
-
Monitoring
-
- Marking - -
-
+
@@ -372,40 +368,6 @@

Embryo Monitoring

-
@@ -1478,7 +1440,6 @@

Acquired - diff --git a/tests/test_one_marking_surface.py b/tests/test_one_marking_surface.py new file mode 100644 index 00000000..93835ee4 --- /dev/null +++ b/tests/test_one_marking_surface.py @@ -0,0 +1,123 @@ +"""There is one marking surface, invoked two ways. + +There were two complete implementations. `static/js/marking.js` — 452 lines +with its own canvas, its own hit-test, its own marker list — lived in the +Embryos tab behind a "Marking" subtab, and `websocket.js` switched the operator +into it when the agent sent a `marking_image` frame. `operate.js` had the +other, on the bottom-camera pane, for an operator marking unprompted. + +So every improvement to marking landed on exactly one of them. The zoom, the +display range, #105's hit-test radius and #126's roster aliasing were all on +the Operate side; none of it existed on the agent's side. Whether an operator +got the corrected behaviour depended on who had asked them to mark. + +The agent's request now lands on the Operate pane. The pushed image is adapted +into the payload shape a live camera frame already has, so the existing +geometry applies to it unchanged. + +WHAT MUST NOT BREAK + +The contract is a websocket request/response, not an HTTP call — the agent +blocks on `session["complete"]` in `routes/websocket.py` and reads a role per +marker out of the answer. So the reply must carry `marking_done` with the +session id and pixel coordinates, and per-marker roles must remain settable +before it is sent. +""" + +from __future__ import annotations + +from pathlib import Path + +WEB = Path(__file__).resolve().parents[1] / "gently" / "ui" / "web" +JS = WEB / "static" / "js" +INDEX = WEB / "templates" / "index.html" + + +def test_the_second_implementation_is_gone() -> None: + assert not (JS / "marking.js").exists(), ( + "static/js/marking.js is back — a second marking surface means every " + "fix lands on one of two, and which one an operator gets depends on who " + "asked them to mark" + ) + html = INDEX.read_text(encoding="utf-8") + assert "MarkingManager" not in html + assert 'id="embryos-marking"' not in html + # The old path specifically. `panels/marking.js` is the panel and stays — + # the two files having near-identical names was itself part of the mess. + assert "/static/js/marking.js" not in html + + +def test_the_agent_request_reaches_the_operate_surface() -> None: + ws = (JS / "websocket.js").read_text(encoding="utf-8") + assert "MARKING_IMAGE" in ws, "marking_image no longer reaches any surface" + assert "MarkingManager" not in ws + + src = (JS / "operate.js").read_text(encoding="utf-8") + assert "ClientEventBus.on('MARKING_IMAGE'" in src + assert "function onMarkingImage(d)" in src + + +def test_the_pushed_image_is_adapted_not_special_cased() -> None: + """It becomes a frame, so the existing geometry applies to it unchanged. + + That is the whole point: the agent's request inherits the zoom, the + corrected hit-test and the display range because it is the same surface, + not because any of them were reimplemented. + """ + src = (JS / "operate.js").read_text(encoding="utf-8") + body = src[src.index("function onMarkingImage(d)") :] + body = body[: body.index("\n }")] + # The live-frame payload shape: shape/downsample/stage_position/jpeg_b64. + for key in ("shape:", "downsample:", "stage_position:", "jpeg_b64:"): + assert key in body, f"the adapted frame is missing {key}" + # PNG, not JPEG — start_marking_session sends PNG. + assert "image/png" in body + + +def test_the_session_declares_its_own_scale() -> None: + """`pixel_size_um` is a session parameter, not the rig default.""" + src = (JS / "operate.js").read_text(encoding="utf-8") + assert "function pxBase()" in src + assert "pixelSizeUm" in src + # Threaded into every geometry call, or a session with a different scale + # would silently place markers wrong. + assert src.count("pxBase()") >= 5, ( + "pxBase is not threaded through the geometry — a session declaring a " + "different µm/px would place markers at the rig default instead" + ) + + +def test_the_answer_keeps_the_contract() -> None: + src = (JS / "operate.js").read_text(encoding="utf-8") + body = src[src.index("function finishMarkingSession()") :] + body = body[: body.index("\n }")] + assert "'marking_done'" in body + assert "session_id" in body + # Back out to pixels: the server and the agent speak pixel coordinates. + assert "stageToFrame" in body + for field in ("number:", "pixelX:", "pixelY:", "role:", "source:"): + assert field in body, f"the answer is missing {field}" + + +def test_per_marker_roles_stay_settable() -> None: + """`marking_done` carries a role per marker and the agent reads them. + + Registered embryos get roles in the Acquisition roster; these are not + registered yet, so the session needs its own way to say which is a + reference. + """ + src = (JS / "operate.js").read_text(encoding="utf-8") + assert "function cycleMarkerRole(index)" in src + assert "cycleRole:" in src, "the panel cannot reach the role toggle" + + panel = (JS / "panels" / "marking.js").read_text(encoding="utf-8") + assert 'data-act="cycleRole"' in panel + assert "data-index=" in panel + + +def test_the_session_ui_is_present_only_while_the_agent_waits() -> None: + """A standing "the agent is waiting" panel would be a lie most of the time.""" + panel = (JS / "panels" / "marking.js").read_text(encoding="utf-8") + body = panel[panel.index("function session(s)") :] + body = body[: body.index("\n }")] + assert "if (!s.session) return ''" in body