Add cortex.webgl.show_multi: multi-brain viewer with yoke toggle#622
Add cortex.webgl.show_multi: multi-brain viewer with yoke toggle#622mvdoc wants to merge 2 commits into
Conversation
Introduces a new entry point that hosts N independent mriview.Viewer
instances in a jsplot.GridFigure and adds a JS YokeController to
synchronize camera + surface mix across panels via a toggle button.
API:
- cortex.webgl.show_multi(views, layout=(rows, cols), yoke=False, ...)
- New multi.html template, new multiview.js (YokeController + UI).
- show() left untouched.
Per-viewer isolation:
- view.py builds one Package per panel, prefixes data layer names with
"panel{i}__" so the global /data/{name} route stays unique, and unions
subjects into a single ctms dict served once per page.
- Each Viewer owns its own Surface per subject (Three.js objects can
only have one parent, so a shared Surface would get reparented out of
the first viewer's scene). SurfDelegate prefers viewer.subjects.
- Each Viewer owns its own colormaps registry; window.colormaps is
re-aliased to the active viewer's dict at addData / setData.
- DOM IDs are stamped per-instance via an idPrefix and looked up via
viewer.$id(name); CSS rules switch to attribute-suffix selectors
([id$='X']) so they match prefixed and unprefixed IDs.
Bug fixes uncovered along the way:
- mriview.Surface.apply: SVG overlay's "update" handler resolves
this.loaded more than once per Surface; in multi-viewer that left
jQuery's resolve-Callbacks memory in a state where new .done() adds
silently became no-ops while state() still reported "resolved", so
per-frame apply() couldn't swap meshes from MeshBasicMaterial to the
data ShaderMaterial. Run synchronously when already resolved.
- sliceplane.js: replace bare "viewer" global ref with this.viewer.
- mriview.js setData: use per-viewer subjects registry when computing
the dataset.loaded defers (was indexing the empty global subjects).
Picker is disabled in multi-viewer mode (per-Surface pick stubbed in
multi.html) — the picker shares the viewer's WebGLRenderer to draw
vertex IDs into offscreen render targets, but in multi-viewer the
renderer's cached render-target/texture-unit state gets corrupted on
the way back, so the next regular render samples the picker's encoding
targets and shows a high-frequency speckled brain. Out of scope for
MVP; see plan.
Tests:
- test_show_multi_smoke: 2 viewers, 2 canvases, no JS errors.
- test_show_multi_yoke_lockstep: yoke ON syncs azimuth + mix across
panels.
- test_show_multi_unyoked_independent: yoke OFF, panels independent.
- All existing single-viewer datatype tests continue to pass.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-viewer feature to the WebGL brain viewer, allowing multiple independent viewers to be displayed side-by-side with optional camera and surface-mix synchronization. The implementation includes a new show_multi Python function, a multi.html template, and significant JavaScript refactoring to support scoped DOM lookups and independent texture registries via ID prefixing. Review feedback points out isolation issues in setColorOptions due to global jQuery selectors, potential race conditions caused by reassigning the global window.colormaps object, and the use of the deprecated global event object in an event handler.
| $('#colorlegend').removeClass('colorlegend-3d') | ||
| $cl.removeClass('colorlegend-2d') | ||
| $cl.removeClass('colorlegend-3d') | ||
| setColorOptions(get1dColormaps()) |
There was a problem hiding this comment.
The setColorOptions function (called here and at line 413) uses global jQuery selectors like $('.colorlegend-select'). In a multi-viewer setup, this will cause all viewers to update the dropdown of the first viewer on the page, breaking isolation. setColorOptions should be refactored to use scoped selectors (e.g., $(viewer.object).find('.colorlegend-select')).
| // (dataset.js, figure.js select2) hit our textures during this viewer's | ||
| // data-loading and UI handlers. Each viewer re-aliases at addData and | ||
| // any UI-driven setColormap entry-point (see _activateColormaps). | ||
| window.colormaps = this.colormaps; |
There was a problem hiding this comment.
Reassigning the global window.colormaps to a per-viewer dictionary is a side effect that can lead to race conditions or unexpected behavior, especially if multiple viewers are active or if other components rely on a stable global registry. While the PR description mentions this is for legacy support, it makes the global state fragile. Consider refactoring dependent code to accept a colormap registry explicitly instead of relying on a mutable global.
| $text.on('click', enterFunction); | ||
| $input.off(); | ||
| $input.on('blur', leaveFunction); | ||
| $input.on('keyup', function (e) { if (event.keyCode === 13) leaveFunction() }); |
There was a problem hiding this comment.
The event handler uses the deprecated global event object instead of the passed argument e. This can lead to issues in environments where window.event is not available or behaves differently.
| $input.on('keyup', function (e) { if (event.keyCode === 13) leaveFunction() }); | |
| $input.on('keyup', function (e) { if (e.keyCode === 13) leaveFunction() }); |
…ution write,
fix typo'd `event` global
- get1dColormaps / get2dColormaps now read from `viewer.colormaps`
directly instead of the re-aliased `window.colormaps` shim — when we
have a viewer in scope, prefer the explicit per-instance registry.
- setColorOptions scopes its option-add/remove to
`$(viewer.object).find('.colorlegend-select')`. The previous
unscoped query targeted the FIRST .colorlegend-select in the
document, so a 1d/2d swap on viewer 1 reached into viewer 0's
dropdown.
- Resolve the colorbar img src via `viewer.colormaps[cmapName]`
instead of the bare-global lookup, same reasoning.
- Drop the `colormaps[pid] = tex` write in the Surface ctor's cmap
catalog loop. Nothing reads colormaps by prefixed id; the write
lands in whichever dict `window.colormaps` happens to point at,
polluting one viewer's registry with the other's THREE.Texture
instances. Per-viewer `viewerCmaps[name] = tex` already covers the
legitimate use case.
- Document the `window.colormaps` re-aliasing more thoroughly so the
legacy-bridge intent is explicit.
- Fix `event.keyCode` → `e.keyCode` (deprecated global event ref) in
the colorbar input keyup handler.
1 participant
Summary
Adds a new entry point
cortex.webgl.show_multi(views, layout=(rows, cols), yoke=False, ...)that hosts N independentmriview.Viewerinstances side-by-side in ajsplot.GridFigure, with a JS-side controller that synchronizes camera + surface mix across panels via a toggle button. The existingshow()is untouched.This is a draft — see Known limitations below.
What's new
cortex.webgl.show_multi(views, layout=(r, c), yoke=False, ...)— accepts a list ofVolume/Vertex/Dataviewitems, one per panel; supports per-viewer different data and different subjects.cortex/webgl/multi.html.cortex/webgl/resources/js/multiview.js—YokeControllerplus the toggle button helper.Per-viewer isolation
The single-viewer pipeline assumed exactly one
Viewer/Surface/Rendererper page. To make N independent viewers coexist on one page:view.py): build onePackageper panel, namespace data layer names withpanel{i}__so/data/{name}stays globally unique, union subjects across panels into a singlectmsblob served once + browser-cached.mriview_htmlwith a per-instance prefix (p0_,p1_, …). Internal lookups go through a scopedviewer.\$id(name)helper, and CSS rules switched to attribute-suffix selectors ([id\$='X']) so they match both prefixed and unprefixed IDs (single-viewer mode keeps prefix'').Surfaceper subject (Three.js objects can have only one parent — a shared Surface gets reparented out of the first viewer's scene).SurfDelegate.updateprefersviewer.subjectsover the globalsubjectsdict; CTM data is still shared via HTTP cache.window.colormapsis re-aliased to the active viewer's dict ataddData/setDataentry points so legacy unscoped lookups (dataset.jsetc.) still resolve.Yoke
mriview.YokeControllerlistens for\"change\"on each viewer'sLandscapeControls(fired only on user input). When ON, it broadcasts{azimuth, altitude, radius, target, mix}to the others, appliessetMixfirst (becausesetMixrecomputes az/alt/target from the per-viewer_foldedazimuth/_flatazimuthinterpolation) and then overrides camera state with the source's exact values. A_propagatingguard breaks the feedback loop. The button is appended to theGridFigurechrome and flipsyoke.enabled.Bugs uncovered + fixed along the way
Surface.applysilently no-ops in multi-viewer mode: the SVG overlay's\"update\"handler callsthis.loaded.resolve()more than once per Surface; in multi-viewer that leaves jQuery's resolve-Callbacks memory in a state where new.done(fn)adds become no-ops whilestate()still reports\"resolved\". Net effect: the per-frameapply()fromdrawViewnever swapped the mesh material to the dataShaderMaterial, so meshes stayed on the defaultMeshBasicMaterialand the brains rendered as flat solid colors. Fix: run synchronously when already resolved.sliceplane.js: bareviewerglobal ref (line 14) broke multi-viewer construction; replaced withthis.viewer.mriview.jssetData: the dataset-loaded\$.when(...)defer loop indexedsubjects[subj], which is the empty global registry in multi mode; now uses the per-viewerviewer.subjectsregistry.Tests
test_show_multi_smoke— 2 viewers, 2 canvases, no JS pageerrors.test_show_multi_yoke_lockstep— yoke ON: dispatchingchangeon viewer 0 syncs azimuth + mix on viewer 1.test_show_multi_unyoked_independent— yoke OFF, panels stay independent.Known limitations / draft scope
pick()stubbed to no-op inmulti.html). The picker shares the Viewer'sWebGLRendererto draw vertex IDs into offscreen render targets; in multi-viewer the renderer's cached render-target / texture-unit state gets corrupted on the way back, so the next regular render samples the picker's encoding targets and shows a high-frequency speckled brain. Diagnosing this further is the most likely next pull.htmlembed.pyis untouched (single-viewer export still works).Test plan
show_multion S1, yoke OFF → panels rotate independently.show_multiwith 4 distinct datasets on S1.