MRG: Multiple fixes in PyVista 3d backend

[GuillaumeFavelier]

This PR fixes multiple things with the PyVista 3d backend.

ToDo:

• Fixes the bug with representation in the PyVista 3d backend.
• Send pre-computed normals to the renderer.mesh() function when possible
• Retrieve nn infos from the surface parameter and use them as normals data in renderer.surface()
• Use smooth_shading=True when the normals are not given OR use same parameters as mayavi
• Refactor the mesh functions to avoid code duplication

master	PR

[GuillaumeFavelier]

TL;DR: The issue is that the reprensentation parameter (named style in PyVista) was sent to the private renderer._add_mesh() function instead of renderer.mesh().

_add_mesh is just a helper function that wraps PyVista add_mesh and process Qt events and it's not only used in renderer.mesh() but in renderer.surface() too. This caused every call to renderer.surface() to use the default wireframe representation.

[larsoner]

Looks like the normals are still not fixed, so the abstraction layer is still missing something

[GuillaumeFavelier]

It appears that the normals are most of the time computed by VTK for the mayavi backend too.

mne-python/mne/viz/backends/_pysurfer_mayavi.py

Lines 362 to 367 in 3d3bb9a

	if vtk_normals:
	mesh = mlab.pipeline.poly_data_normals(mesh)
	mesh.filter.compute_cell_normals = False
	mesh.filter.consistency = False
	mesh.filter.non_manifold_traversal = False
	mesh.filter.splitting = False

Even though the abstraction layer supports receiving precomputed normals, they would need to be computed on-the-fly in most cases.

[larsoner]

Ahh I see. So the question is, why can't we use smooth_shading=True in PyVista, but it has always worked fine in Mayavi? Do we need to use the same set of options?

[GuillaumeFavelier]

why can't we use smooth_shading=True in PyVista

I think this is related to an issue on macos during migration to pyvista 0.25/vtk 9 if i'm not mistaken

[GuillaumeFavelier]

Do we need to use the same set of options?

That's a good idea :)

[larsoner]

I think this is related to an issue on macos during migration to pyvista 0.25/vtk 9 if i'm not mistaken

Yes but the question is why do we hit it on PyVista but not Mayavi? Maybe we won't anymore now that we supply the normals in brain. I think it's worth trying to make sure all the options are the same, then setting smooth_shading=True in PyVista (when we don't supply normals) and see if it works. Or if you can't set the options the same in PyVista, do our own VTK-based computation that's equivalent to Mayavi (or fix how PyVista does it in pyvista itself).

[GuillaumeFavelier]

Okay, i'll take care of that in here 👍

[GuillaumeFavelier]

In terms of visual quality, it's certainly better to have smooth shading 🤩

[larsoner]

We now get on Linux the same sort of segfault we've been seeing on macOS:

https://travis-ci.org/github/mne-tools/mne-python/jobs/710809861

[GuillaumeFavelier]

We now get on Linux the same sort of segfault we've been seeing on macOS

Good news that it's consistent then 😅 ?

I'll try the custom VTK filter now.

[GuillaumeFavelier]

I try first with a homemade private function and if it works I'll use PyVista compute_normals() with the good parameters.

[larsoner]

Sounds good. Although if it does turn out to be a parameters issue, we should definitely open a PyVista issue about it as segfaulting with default parameters is not good. Maybe they need to be triaged by VTK version or something -- and maybe there is an underlying VTK bug, again worth whittling down to a minimal example, as the VTK folks have generally been very responsive when I've brought them issues that are easily reproducible.

[GuillaumeFavelier]

For reference, here are the parameters used for normal computation (for smooth shading) by the 3d backends ('x' for True, ' ' for False):

function	mayavi	pyvista
cell_normals
point_normals	x	x
split_vertices
flip_normals
consistent_normals		x
auto_orient_normals
non_manifold_traversal		x

When using those values with PyVista (smooth_shading=True), Travis fails as @larsoner describes in #8041 (comment) (version 7c6b9bf).

When forcing PyVista to use the same parameters as Mayavi, the CIs are happy (version 30af751) therefore I suppose that either consistent_normals or non_manifold_traversal (or both) cause the failures.

I can't reproduce those failures locally so I will push some test commits to clarify the situtation.

Note: default values for PyVista are available in the documentation

[larsoner]

Great! Worth opening an issue about the defaults being unsafe in PyVista?

[GuillaumeFavelier]

I did my best playing with my TTY and Xvfb but although I'm on linux I can't reproduce this locally, I am now sure this does not happen consistently because I restarted #8041 (comment) and the red job became green @larsoner.

2e6f434, 90c42fb and 7868c18 show that it's not because of the parameters of compute_normals().

I did 52655ec to start from scratch because I was too focused on mesh() and surface() and then f5c8910 made me think that the issue is related to sphere() somehow: everything is green when I use the homemade _compute_normals() until I force sphere() to compute normals although it's not necessary (MacOS fails consistently from that though).

Now I tend to think that the segfault happens when the normals are actually recomputed (normals are always computed as long as smooth_shading=True) but once again I need to confirm it. I think a dedicated minimal test can help here.

[larsoner]

I think a dedicated minimal test can help here.

Feel free to add one. And also during long debugging sessions like this you can speed up iterations by changing azure_pipelines.yml and .travis.yml calls to pytest to isolate for example``pytest ... mne/viz/`. Then each run comes back in 15 minutes instead of 30

[larsoner]

Shouldn't this just be False since they're either added or computed above?

[GuillaumeFavelier]

I remove it in the commit just after

[larsoner]

This is the one place we actually don't want smooth shading. Is there any way to disable it on this example? It makes it clear that the mesh is decimated quite a bit. Or maybe we should just use wireframes to make the point? WDYT?

[larsoner]

Yeah I think wireframes could look good for the last plots:

https://mne.tools/dev/auto_tutorials/source-modeling/plot_background_freesurfer_mne.html#surface-decimation

[GuillaumeFavelier]

Using wireframe could be a bit messy. I will let this option then 👍

[GuillaumeFavelier]

I modified mesh() and surface() to be light interfaces over the polydata() function. I also removed _add_polydata_actor() in favor of polydata() too. What do you say @larsoner ?

[larsoner]

Thanks @GuillaumeFavelier !