ENH: add a vtk.js backend for MNE's 3D renderer (JupyterLite split 3/5) - #14144
ENH: add a vtk.js backend for MNE's 3D renderer (JupyterLite split 3/5)#14144natinew77-creator wants to merge 30 commits into
Conversation
VTK cannot load in WebAssembly, so the JupyterLite notebooks need a renderer that draws with vtk.js. MNE does its geometry in numpy and only hands the result to a renderer, so replacing that last step leaves the transform maths to MNE.
c20eae7 to
5a823e2
Compare
natinew77-creator
changed the title
|
Maybe you have looked and I am coming late to this... have you thought about adding it as a type of AbstractRenderer and using https://github.com/tkoyama010/pyvista-js ? Maybe it's not too much work to use that in place of our PyVista calls... but if you've tried or looked I could be way off! |
Hi Eric, it already uses pyvista-js. On AbstractRenderer, it sits in doc/ as a string the setup cell appends, to keep browser-only code out of mne/. But it already implements 21 of the 22 abstract methods, so converting it is mostly moving and registering it, not a rewrite. Happy to do that here, or land this as is and convert in a follow-up. Which would you prefer? Thanks! |
|
Yeah if there is some way for it to be a plain renderer and then |
The renderer was a 560-line string literal, which no linter or formatter could see. It now lives in _lite_renderer_cell.py as ordinary Python and the cell is read from there, so ruff covers it like any other file. The code itself is unchanged apart from what the formatter did to it.
Done in eb3c814, it's a plain module now and LITE_RENDERER_CELL is read from it, so ruff and the formatter cover it. |
|
I think it's probably best to move this to the I don't want to add 700 lines with no unit tests, things are bound to break / or be broken... |
Done, moved to mne/viz/backends/_lite.py as a real _AbstractRenderer subclass. That turned up a bug, it reported a _kind that isn't "notebook", sending _coreg.py into _qt_app_exec with no Qt event loop in a browser. pyvista-js is pure Python, so no selenium, 18 pytest cases plus one nbexec test. It ships in the wheel now too, so the setup cell drops from ~24,500 characters to 438. I didn't register it in VALID_3D_BACKENDS, since that pulls in the 17 widget classes _do_widget_tests expects. Happy to if you meant that. |
| def _enable_time_interaction(self, *args, **kwargs): | ||
| # the figures are static here; there is no time slider to wire up | ||
| return None |
There was a problem hiding this comment.
some things like this you might get for free if this inherited from the _TimeInteraction mixin class, and there are other mixin classes that might give you lots of other interactivity for free. See, e.g.,
mne-python/mne/viz/backends/_notebook.py
Lines 1533 to 1543 in 8fa0c10
There was a problem hiding this comment.
It needs _dock_add_slider and the rest of the dock API, and the _Ipy* mixins are unreachable because _notebook.py imports _pyvista at module level, so this raises now instead.
There was a problem hiding this comment.
Ah, that's too bad. @larsoner would it be worth trying to refactor _notebook.py (to avoid module-level pyvista import) to see if those mixins could be reused here? (In a follow up I guess)
There was a problem hiding this comment.
Yeah in a follow-up moving all the ipy* into a _notebook_base.py or similar then importing two places would make sense. It would make _notebook.py very short I think, and make clear what is "ipywidgets vs qt abstraction" and what is actually related to 3D renderering.
There was a problem hiding this comment.
That split looks clean, none of the _Ipy* classes touch _pyvista, so _notebook_base.py would need only ipywidgets and _abstract. Happy to open the follow-up.
| _mne_rend.backend = _LiteBackend() | ||
| # Naming a backend keeps _get_3d_backend() from walking VALID_3D_BACKENDS and | ||
| # importing _qt, which would overwrite the stub above on its way to failing. | ||
| _mne_rend.MNE_3D_BACKEND = "notebook" |
There was a problem hiding this comment.
| _mne_rend.MNE_3D_BACKEND = "notebook" | |
| _mne_rend.MNE_3D_BACKEND = "jupyterlite_notebook" |
There was a problem hiding this comment.
Great, It registers as jupyterlite_notebook now (16d3418), so MNE_3D_BACKEND comes from set_3d_backend instead of being set by hand.
Replaces the _activate monkeypatching with a real "jupyterlite" backend so it works with the pytest renderer fixtures. Also fixes the glyph templates, which drew spheres at twice their size and EEG cylinders off their own axis.
|
Thanks for the quick response @natinew77-creator. I only have phone at the moment but will look again tomorrow morning |
Matches what the review asked for and lines the backend name up with _kind. The capability table goes back to upstream's three columns, with the browser backend described in the notes instead.
It was handing back only the last color group and dropping the rest.
Drops a wrong claim about plot_bem, makes the drawing tests assert geometry rather than actor counts, and stops accepting arguments without saying why they cannot be honoured.
mne-toolsgh-13074 made mne/viz/_3d.py write channel names onto the second value instanced_mesh returns, which broke plot_alignment here three ways: a pyvista-js PolyData has no field_data, several colours came back as a list, and empty positions came back as None. Return the per-instance point cloud instead, always one object with the mapping attached, which is what _PyVistaRenderer returns.
4 participants
Part 3 of the split of #13925. Parts 1 and 2 are #14128 and #14135.
Adds a drawing backend for MNE's 3D renderer that uses vtk.js, since VTK itself cannot load in WebAssembly. MNE's 3D functions do their geometry and coordinate-frame work in numpy and only hand the result to a renderer, so replacing that last step leaves the transform maths with MNE. That matters here, because a subtly wrong head or device transform still produces a plausible-looking picture.
Supported: meshes, surfaces, spheres, tubes and glyphs, which covers the static figures the docs render. Not supported: the interactive
Braintime viewer, which needs dock widgets, and scalar colormaps, which pyvista-js 0.15 does not have.It is a plain string constant, so nothing in the build touches it yet. The setup cell that appends it is #14150.