|
| 1 | +# SurfAnnotate — agent notes |
| 2 | + |
| 3 | +## Architecture |
| 4 | + |
| 5 | +``` |
| 6 | +src/ |
| 7 | + main.js UI wiring, interaction, export. The only DOM-aware file. |
| 8 | + surface/ Pure geometry and algorithms — no DOM, no NiiVue, all unit-tested |
| 9 | + adjacency.js CSR 1-ring vertex graph from (vertices, triangles) |
| 10 | + pathfinder.js A* shortest path along mesh edges; chain building and validation |
| 11 | + edgeAnchor.js Distance-to-cut field; extends a border out to an open edge |
| 12 | + exclude.js Cuts a completed ROI out of the graph so its rim is an edge |
| 13 | + parcellation.js Resolves ordered ROI definitions into disjoint regions |
| 14 | + fill.js Flood fill inside a closed boundary, seeded or automatic |
| 15 | + roiSession.js Drawing state: clicks, trace, fill, landmarks |
| 16 | + vertexLookup.js Uniform-grid nearest-vertex search |
| 17 | + hatch.js Stripe and halo masks for fill rendering |
| 18 | + niivue/ Every NiiVue call lives here |
| 19 | + meshAdapter.js Loading, picking, layers, overlays |
| 20 | + colormaps.js Colour maps NiiVue does not ship |
| 21 | + io/ File writers/readers, pure and unit-tested |
| 22 | + freesurferLabel.js, gifti.js, points.js, naming.js, classify.js, geometryOffset.js |
| 23 | +``` |
| 24 | + |
| 25 | +The split matters: `surface/` and `io/` run under plain `node --test` with no browser, |
| 26 | +which is why the algorithm suite is fast and deterministic. Only `main.js` and |
| 27 | +`niivue/` need a WebGL context. |
| 28 | + |
| 29 | +## Key conventions |
| 30 | + |
| 31 | +- **All NiiVue *mesh* access goes through `src/niivue/meshAdapter.js`.** Construction |
| 32 | + and the canvas (`new Niivue`, `attachToCanvas`, `setSliceType`, `drawScene`, |
| 33 | + `removeMesh`) stay in `main.js`, and colormaps in `niivue/colormaps.js`; it is the |
| 34 | + mesh, layer and picking surface that is centralised, and that is what the 1.0.0-rc |
| 35 | + rewrite changes. 1.0.0-rc.x is a |
| 36 | + ground-up rewrite (`pts`/`tris` → `positions`/`indices`, camelCase layer fields, no |
| 37 | + `indexNearestXYZmm`), so keeping the surface area in one file makes that migration a |
| 38 | + single-file change. Pin stays at **0.69.0** — npm `latest`, and byte-identical mesh |
| 39 | + code to the 0.68.x the rest of this monorepo uses. |
| 40 | +- **The favicons in `public/` are generated — run `node icon/render.mjs`, do not edit |
| 41 | + the PNGs.** `icon/surfannotate.svg` is the master and is deliberately *not* shipped: |
| 42 | + ~46 kB gzipped against 1.9 kB for the 32px PNG a tab actually uses, and the rasterised |
| 43 | + versions are indistinguishable at every size a browser asks for. Its viewBox is already |
| 44 | + square and tight to the *painted* bounds — measured by rasterising and finding the alpha |
| 45 | + box, because `getBBox()` ignores stroke width and would clip the brain outline. Note |
| 46 | + that `drawImage(img, 0, 0, w, h)` stretches rather than fitting, which is why the |
| 47 | + viewBox has to be square before rendering. |
| 48 | +- **`index.html` opens on a start page, not the app.** `#startPage` is a fixed-position |
| 49 | + section over `#app`, hidden by `#enterAppButton` — the same shape calmar uses. The app |
| 50 | + is behind it the whole time, so the canvas is already sized and nothing needs |
| 51 | + re-laying out. Every e2e test dismisses it in `beforeEach`. |
| 52 | +- **Loads are serialised through `enqueueLoad`.** A file input fires `change` when the |
| 53 | + files are set, not when the async handler finishes, so two quick picks — or a pick |
| 54 | + during a drop — started overlapping `loadSurface` calls that interleaved on |
| 55 | + `state.surfaces` and the active-surface mirrors. |
| 56 | +- **`setInputFiles` does not wait for the load.** In e2e, always follow it with a wait on |
| 57 | + the status text or the surface-list count before touching `window.__surfannotate`. |
| 58 | + Getting this wrong shows up as a rare `session is null`, one test per full run. |
| 59 | +- **`state.surfaces` is the list; `state.mesh`/`geometry`/`session`/... are mirrors of |
| 60 | + whichever entry is active,** written by `activateSurface`. The exceptions are |
| 61 | + deliberate: `state.graph`/`finder`/`excluded` belong to **`bindSession`**, because |
| 62 | + they track the *cut* graph rather than the surface's own, and the overlay mirrors |
| 63 | + belong to the overlay handlers. Assigning `entry.graph` in `activateSurface` puts the |
| 64 | + uncut graph back over bindSession's work and forces a second full rebuild. |
| 65 | +- **Every guard in `fill.js` is a fraction of the WALKABLE surface, not `graph.V`.** |
| 66 | + `excludeVertices` keeps a completed ROI's vertices and their indices and only strips |
| 67 | + their edges, so `graph.V` stops being the size of the surface a flood can reach the |
| 68 | + moment any ROI is saved. Measured against it, the >half-the-surface swap fires |
| 69 | + spuriously — handing back the exterior as the ROI, with `error: null` — and the 40% |
| 70 | + escape guard goes blind as the parcellation fills up. |
| 71 | +- **Exactly one surface is visible at a time.** Not a UI preference: the depth picker |
| 72 | + returns a position, never an identity, so a click over two overlapping meshes could |
| 73 | + not be attributed to either. Multiple simultaneous surfaces would silently break |
| 74 | + vertex picking. |
| 75 | +- **ROI sessions are keyed by topology (`vertexCount:triangleHash`), not by file.** One |
| 76 | + subject's white/pial/inflated share a session so border points survive a switch; |
| 77 | + `RoiSession.rebind` moves it and deliberately discards the traced chain and fill, |
| 78 | + which are geometry-dependent. Deleting a surface only drops the session once the last |
| 79 | + surface with that topology is gone. |
| 80 | +- **"Use a completed ROI as an edge" is one graph operation, not a special case.** |
| 81 | + `exclude.js` isolates the ROI's vertices, which makes its rim an open edge; every |
| 82 | + other layer — pathfinder, fill, `closeOnEdge` — then behaves as it already did for a |
| 83 | + flat patch. Vertices keep their indices (labels and clicks refer to them), and |
| 84 | + `isIsolated` is what keeps them out of paths and fills. Resist adding a barrier |
| 85 | + parameter to the algorithms: the graph is the barrier. |
| 86 | +- **An ROI is a definition, not a mask.** `state.rois` holds border points, closure |
| 87 | + mode, region index and an anchor; `mask`/`chain`/`error` on them are *outputs* of |
| 88 | + `recomputeParcellation` and are overwritten wholesale. Never edit a mask in place — |
| 89 | + the next recompute discards it. |
| 90 | +- **Order is meaning.** Each ROI is resolved with the ROIs above it cut away, so |
| 91 | + earlier ROIs win every overlap and editing one re-derives all the ones below it. |
| 92 | + This is what makes a moved shared boundary move both sides. |
| 93 | +- **`restoreEdited` clears the session too.** The ROI is authoritative again once it |
| 94 | + is back on the list, and a leftover copy of its clicks means a later Save appends it |
| 95 | + a second time under a new id and colour — the duplicate then resolves as |
| 96 | + unresolvable, because the original already owns the territory. |
| 97 | +- **Reopening keeps the ROI's position** (`state.editIndex`). That is what makes it |
| 98 | + work at all: an ROI's border points routinely lie *inside* the ROI drawn next to it, |
| 99 | + because the fill excludes the border row, so V2 claims the row V1 was clicked along. |
| 100 | + Editing V1 in place means only the ROIs above it constrain, and V2 is below. |
| 101 | +- **The anchor is how an ROI is recognised after its neighbours move.** Component size |
| 102 | + ordering alone flips as ROIs grow and shrink; `anchorVertex` picks the vertex furthest |
| 103 | + from the border by hop count, which is the last one a neighbour would take. The border is recomputed |
| 104 | + from the clicks, not restored from the saved chain, for the same reason the clicks are |
| 105 | + authoritative everywhere else. |
| 106 | +- **The clicked vertices are the only authoritative ROI state.** The traced chain and |
| 107 | + the filled mask are always derived and are discarded whenever the clicks change. |
| 108 | + freeview does the opposite and that is what makes its undo impossible. |
| 109 | +- **Flood fill must only ever walk the 1-ring graph.** Augmenting it (unfolded 2-ring |
| 110 | + edges, k-ring neighbourhoods) adds edges that cross faces, so the fill hops the |
| 111 | + barrier and swallows the hemisphere. Validate the chain before filling. |
| 112 | +- **Exports must undo the loader's translation.** NiiVue adds the volume centre |
| 113 | + to every vertex on load — `cras` from a FreeSurfer footer, `VolGeomC_R/A/S` from |
| 114 | + GIfTI — turning tkreg RAS into scanner RAS so meshes line up with volumes. A |
| 115 | + `.label` header declares `vox2ras=TkReg` and FreeSurfer's |
| 116 | + `labelGetSurfaceRasCoords` takes it verbatim, so the shift has to come back off: |
| 117 | + `io/geometryOffset.js` recomputes it from the same bytes and the writers subtract |
| 118 | + it. It mirrors NiiVue's quirks deliberately — `cras` is applied even when the |
| 119 | + footer says `valid = 0`, and GIfTI values are read only from CDATA — because a |
| 120 | + correction that does not match what was applied is worse than none. |
| 121 | + `showCoordinateSource` distinguishes the two cases, because the advice differs: an |
| 122 | + inflated or spherical surface shares the native vertex indexing, so switching to a |
| 123 | + loaded `lh.white` carries the ROIs over; a flat patch is a *cut* with its own |
| 124 | + numbering and fewer vertices, so sending the user to a whole hemisphere would hide |
| 125 | + their work rather than fix anything. |
| 126 | + Drawing on `lh.inflated` or a flat patch still writes *that* surface's coordinates |
| 127 | + — freeview substitutes the white surface (`SurfaceLabel.cpp:408`), this app warns |
| 128 | + instead. `showCoordinateSource` names the surface in the export panel and flags a |
| 129 | + non-anatomical one, using `naming.surfaceKind` plus a planarity check on the |
| 130 | + geometry, which catches a flat patch whatever it is called. Substituting a |
| 131 | + same-topology anatomical surface automatically is still open. |
| 132 | +- **Exports are named `<hemisphere>.<roi>`, never after the source surface.** See |
| 133 | + `io/naming.js`. An ROI drawn on `lh.sphere.reg` is valid on any surface sharing that |
| 134 | + vertex indexing, so `lh.sphere.reg.surf.V1.label` would misrepresent it. |
| 135 | +- **Never trust a fill that covers more than 40% of the surface** — that is a gap in |
| 136 | + the boundary, not a large ROI. Refuse and tell the user. The one exception is an |
| 137 | + edge closure (`closure === 'edge'`): there the barrier has already been *proved* to |
| 138 | + separate the graph by counting components, so a leak is not possible and the guard |
| 139 | + would only block a border that legitimately halves a patch. |
| 140 | +- **A closed border is not the only way to enclose a region.** On a cut surface the open |
| 141 | + edge is itself an impassable barrier to a 1-ring flood fill, so a border running from |
| 142 | + the cut to the cut encloses a region with no loop at all. That is what `closeOnEdge` |
| 143 | + builds, and it is why flat patches do not need dozens of clicks along the rim. |
| 144 | + It does *not* follow that any edge-to-edge line separates the surface — one joining |
| 145 | + two distinct cuts turns an annulus into a disk without dividing it — so the component |
| 146 | + count is checked, never assumed. |
| 147 | +- **`#controls` must stay `flex-wrap: nowrap`.** The shared `.nd-imaging-controls` class |
| 148 | + sits on the same element and sets `flex-wrap: wrap` for its own row layout. With the |
| 149 | + column direction `styles.css` applies, anything taller than the panel wraps into a |
| 150 | + second column to the *right* of a 320px panel — invisible and unreachable, and silent, |
| 151 | + because wrapping absorbs the overflow so `overflow-y` never scrolls. Growing the tool |
| 152 | + section by ~120px made every annotation button vanish the moment a cut surface was |
| 153 | + loaded. Covered by an e2e test that asserts one column. |
| 154 | +- **Toggling `[hidden]` needs `display: none !important`** (in `styles.css`). Any author |
| 155 | + `display` rule outranks the UA stylesheet's `[hidden]`, so an element with both stays |
| 156 | + stubbornly visible. This shipped once as a drop hint permanently covering the canvas. |
| 157 | + |
| 158 | +## NiiVue 0.69 traps, all found the hard way |
| 159 | + |
| 160 | +- `NVMesh.loadLayer` is **static**; calling it on an instance throws silently. Use |
| 161 | + `NVMeshLoaders.readLayer(...)` and push the result onto `mesh.layers`. |
| 162 | +- Overlays default to the **full data range**, and `readCURV` min-max normalises *and |
| 163 | + inverts* FreeSurfer curvature. Values cluster mid-range, so a 0–1 window renders flat |
| 164 | + grey and looks like a failed load. We set a 2nd–98th percentile window. |
| 165 | +- **There is no vertex picking.** `onLocationChange` gives mm only; the picking shader |
| 166 | + packs depth, not identity. `indexNearestXYZmm` is a ~3 ms linear scan — 163x slower |
| 167 | + than the uniform grid in `vertexLookup.js`. |
| 168 | +- **There is no "the ray missed" signal.** `depthPicker` early-returns and leaves the |
| 169 | + crosshair untouched, so an unchanged crosshair is ambiguous. `pickWorldMm` disambiguates |
| 170 | + on screen position. |
| 171 | +- **`dragAndDropEnabled: false` is not enough.** `dropListener` calls |
| 172 | + `stopPropagation()`/`preventDefault()` *before* consulting that flag, so drop handlers |
| 173 | + must be **capture-phase** on an ancestor to see the event at all. |
| 174 | +- `opts.loadingText` defaults to `"loading ..."` and is painted over an empty canvas. |
| 175 | +- Geometry is `mesh.pts` / `mesh.tris`. **`mesh.vertexCount` is `pts.length`**, i.e. |
| 176 | + three times the vertex count. |
| 177 | +- Avoid the `Uint8Array` packed-RGBA layer path — it renders nothing in 0.69.0. Use |
| 178 | + `Float32Array` values plus `colormapLabel`. |
| 179 | +- **A hand-built mesh layer must set `nFrame4D: 1`.** `NVMeshLayerDefaults` leaves it 0, |
| 180 | + and NiiVue computes the frame as `min(max(frame4D, 0), nFrame4D - 1)` — which is -1, so |
| 181 | + it reads `values[j - vertexCount]`, gets `undefined`, and every colour lookup lands on |
| 182 | + NaN. The whole surface renders black, not the layer. |
| 183 | +- **`readLayer` has no case for a FreeSurfer `.label`.** The extension falls through to |
| 184 | + its curvature reader, which cannot parse ASCII and returns a layer with zero values. |
| 185 | + `io/freesurferLabel.labelToValues` expands it and `attachValueLayer` builds the layer. |
| 186 | +- `mesh.updateMesh(gl)` costs ~24 ms on a 163k-vertex mesh because it regenerates |
| 187 | + normals for unchanged geometry. Fine per interaction, too slow per frame. |
| 188 | + |
| 189 | +## Test surface |
| 190 | + |
| 191 | +| Command | Covers | |
| 192 | +| --- | --- | |
| 193 | +| `pnpm --filter surfannotate test` | `surface/` and `io/` — adjacency, A*, chain validation, fill (including escape and figure-eight cases), hatching, vertex lookup vs brute force, ROI session contract, every file writer | |
| 194 | +| `pnpm --filter surfannotate lint` | `node --check` over every JS file | |
| 195 | +| `pnpm --filter surfannotate test:e2e` | Real Chromium with SwiftShader: shell mount, WebGL2, surface load and index, picking, draw→close→fill→export, drag-and-drop, click-vs-drag, overlay window, marker lifecycle, colour map and range, ROI naming, edge closure on a flat patch | |
| 196 | + |
| 197 | +`test/fixtures/lh.flat.surf.gii` is a synthetic flat patch — a disk with one open edge, |
| 198 | +like `mris_flatten` output but a few kB. Regenerate with |
| 199 | +`node test/fixtures/make-flat-patch.mjs`. Its faces are wound to point along -x on |
| 200 | +purpose: a sheet is one-sided, NiiVue does not cull back faces but does shade them by |
| 201 | +the flipped normal, so from the wrong side the patch renders near-black on a dark |
| 202 | +background and looks like a failed load. -x is where the default render view looks from. |
| 203 | + |
| 204 | +**When adding an e2e test, verify it fails without the fix.** Two drag-and-drop tests |
| 205 | +here passed against broken code — one dispatched events on the wrong element, and the |
| 206 | +other used a synthetic `DragEvent` whose `clientX/Y` of 0 made NiiVue's `eventInBounds` |
| 207 | +bail before the `stopPropagation` that caused the bug. |
0 commit comments