Conversation
This was referenced Sep 20, 2026
mvdoc
added this pull request to stack #744
September 20, 2026 01:08
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Subject mixing, unsafe output names, asynchronous readiness races, and group-name collisions can cause incorrect or missing rendering.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (7)
What changed in this PR
Adds WebGL rendering and controls for Tractogram streamlines.
Changes:
- Packages and serves tractogram buffers for live and static viewers.
- Adds Three.js geometry, visibility, opacity, and bundle controls.
- Adds documentation and transport/browser tests.
| File | Description |
|---|---|
docs/dataset.rst |
Documents viewer controls. |
cortex/webgl/view.py |
Serves and exports tract buffers. |
cortex/webgl/template.html |
Loads tractogram UI resources. |
cortex/webgl/static.html |
Initializes static tracts. |
cortex/webgl/resources/js/tractogram.js |
Implements rendering and controls. |
cortex/webgl/resources/js/svgoverlay.js |
Excludes tracts from depth override passes. |
cortex/webgl/resources/js/mriview.js |
Integrates tracts into the viewer. |
cortex/webgl/resources/css/mriview.css |
Styles tract controls. |
cortex/webgl/mixer.html |
Initializes live tracts. |
cortex/webgl/data.py |
Builds tract metadata and buffers. |
cortex/tests/test_webgl_tractogram.py |
Tests transport and rendering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Streamlines bypass the mosaic and CTM machinery entirely. Package collects each Tractogram into four little-endian buffers -- points, offsets, colors and group membership -- served by a new TractHandler under /tract/, or written to tracts/*.bin by make_static. Colors arrive finished, since the colormapping happens in Python, so the browser side stays simple. resources/js/tractogram.js builds one THREE.Line in LinePieces mode per tractogram, with a Uint16 or Uint32 index depending on size and a duplicated-vertex fallback where OES_element_index_uint is missing. The controls live in their own #tracts panel under the dataset box rather than in dat.gui, because what a tractogram shows is data: a visibility checkbox and an opacity slider per tractogram, plus a checkbox per bundle when the file has groups. A streamline draws while it belongs to at least one checked group. Three things about Three.js r69 that this had to work around, each of which looked like a rendering bug first: - svgoverlay.js renders a depth pass with scene.overrideMaterial, which the line geometry cannot satisfy; tract objects opt out through userData.skipOverrideMaterial or r69 throws on the missing attributes. - r69 walks the transparent list backwards, so a translucent tractogram needs a large renderDepth to stay behind the surface rather than on top of it. - The material keeps writing depth at every opacity. Without it the streamlines have nothing to depth-test against each other and blend in buffer order, so the bundle last in the geometry paints over those in front and the apparent depth order changes as the opacity slider moves. Streamlines hide as soon as the surface starts to inflate or flatten, since their coordinates only mean anything against the folded surface, and a tractogram alone cannot open a viewer -- Package says so rather than failing obscurely later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dragging tract opacity to 0 did not hide the streamlines: it turned them into silhouettes cut out of the translucent surface. Writing depth at every opacity is what keeps the bundles from reordering themselves as the slider moves, but it also means fragments contributing no colour still hide whatever is behind them. r69's basic shader runs its alphatest discard while gl_FragColor.a is still just `opacity`, before vertex colours are folded in, so a small constant alphaTest drops exactly the fragments that would have been invisible anyway, depth write and all. Being constant, the ALPHATEST define compiles once and opacity changes stay cheap. The bundle checkboxes were in wire order, which for a real TRX is arbitrary: pyAFQ's HCP atlas lists IF0F_R, F_R, F_L, CC_ForcepsMajor and so on. They are now alphabetical, comparing digit runs as numbers so CST_2 precedes CST_10, with the synthetic "(ungrouped)" entry pinned last since it is not a bundle and its leading parenthesis would otherwise float it to the top. groupNames() returns the same order, so the panel and the API agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mvdoc
force-pushed
the
tractography/3-webgl
branch
from
September 20, 2026 01:46
218d429 to
72c0836
Compare
`_served_metadata` located the dataset payload by the `dataset.fromJSON(`
marker and JSON-decoded from there. Adding the tract panel changed
mixer.html and static.html to assign the payload first, so that
`viewer.addTracts(metadata.tracts)` has something to read:
metadata = {{data}};
dataviews = dataset.fromJSON(metadata);
That call now takes an identifier rather than a JSON literal, so the
decode landed on `metadata);` and raised, failing the four tests that read
the served metadata:
json.decoder.JSONDecodeError: Expecting value: line 15603 column 30
The served page was always correct -- the viewer parses it fine -- so the
marker moves to the assignment. Line 4 of each template declares
`metadata` without an `=`, so the assignment is the page's first
`metadata = `.
Verified against a real served page: a viewer started with
open_browser=False reproduces the failure at exactly the line and column
CI reports, and returns the payload once the marker moves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zgY3aPBUWUhHBpisvF6Kw
A tractogram's name is a dataset key, i.e. an arbitrary string, and it was
reaching both the buffer urls and the file names of `make_static` verbatim:
`Dataset(**{"../../escape": tract})` wrote outside the output directory, and
any name holding a `/` failed outright. Buffers now travel under a transport
id (`cortex.webgl.data._tract_id`) that is safe as a single url segment and
as a file name; ordinary names pass through unchanged, so the urls stay
readable, and the metadata is still keyed by the name the user gave.
The viewer keys its tractograms by name in a plain object too, where a
tractogram called "constructor" or "toString" looked like one that was
already loaded -- addTracts would "replace" it and rmTracts would then trip
over the inherited value. Every lookup goes through `Viewer.hasTract` now,
and the per-group maps, whose keys are bundle names, are prototype-free.
Streamlines are meaningless against another subject's brain, but in a
multi-subject dataset they stayed on screen when the active dataview (and
with it the surface) switched subjects. A tractogram now shows only while a
dataview of its own subject is active, lines and panel entry alike; and
`Package` refuses at the outset a tractogram whose subject has no dataview
at all, rather than booting a viewer that silently omits it.
Tractograms load outside `viewer.loaded` on purpose, so that a slow
streamline download cannot hold up an otherwise usable viewer -- which left
`getImage`/`save_3d_views` free to screenshot a scene whose streamlines were
still in flight. `headless_viewer` waits for `viewer.tractsState()` on top of
`viewer.loaded`, and reports a failed download rather than waiting it out.
The panel's controls were bare inputs with no accessible name, and the
collapse control was a glyph in a <span>: unreachable by keyboard and
unannounced by a screen reader. They carry their own labels now, the
collapse and all/none controls are real buttons, and each bundle row is a
<label> wrapping its checkbox.
Testing: the index strategy is split into `mriview.indexModeFor`, so the
uint16/uint32/duplicate choice can be checked with plain arguments, and
`forceIndexMode` pins it, so the duplicated-vertex fallback is built and
compared segment by segment against the indexed geometry it replaces instead
of going untested for want of a 65536-point fixture. The headless test drops
its own polling loop, which the new wait makes unnecessary.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011GHv7KSxts6xnv1eQrgouT
`self.brains` and `self.images` are keyed by a BrainData's name, which is a hash of its array, so two distinct dataviews holding the same data are one brain on the wire. They were two entries in `uniques` all the same (BrainData compares by identity), which had `reorder` rewrite `self.images[name]` a second time on top of the npy blob its first pass had just written there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011GHv7KSxts6xnv1eQrgouT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack
Merge bottom-up; each PR is based on the one above it.
Streamlines bypass the mosaic and CTM machinery entirely.
Packagecollectseach
Tractograminto four little-endian buffers — points, offsets, coloursand group membership — served by a new
TractHandlerunder/tract/, orwritten to
tracts/*.binbymake_static. Colours arrive finished, since thecolormapping happens in Python, so the browser side stays simple.
resources/js/tractogram.jsbuilds oneTHREE.LineinLinePiecesmode pertractogram, with a Uint16 or Uint32 index depending on size and a
duplicated-vertex fallback where
OES_element_index_uintis missing. Thecontrols live in their own
#tractspanel under the dataset box rather than indat.gui, because what a tractogram shows is data: a visibility checkbox and an
opacity slider per tractogram, plus a checkbox per bundle when the file has
groups. A streamline draws while it belongs to at least one checked group.
Three.js r69 gotchas
Each of these looked like a rendering bug first, and each is commented where it
bites:
svgoverlay.jsrenders a depth pass withscene.overrideMaterial, which theline geometry cannot satisfy. Tract objects opt out through
userData.skipOverrideMaterial, or r69 throws on the missing attributes.large
renderDepthto stay behind the surface rather than on top of it.have nothing to depth-test against each other and blend in buffer order, so
the bundle last in the geometry paints over those in front of it and the
apparent depth order changes as the opacity slider moves.
Streamlines hide as soon as the surface starts to inflate or flatten, since
their coordinates only mean anything against the folded surface, and a
tractogram alone cannot open a viewer —
Packagesays so rather than failingobscurely later.
Testing
cortex/tests/test_webgl_tractogram.py: the wire format (buffer sizes, dtypes,group slices), that tractograms stay out of every existing JS path, that
make_staticwrites the four.binfiles, and a headless leg asserting thegeometry is built, per-bundle toggles change the drawn segment count, and the
material keeps writing depth after an opacity change.
Verified by hand in the browser as well, live and from a
make_staticexportserved over plain HTTP.
🤖 Generated with Claude Code