Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

What does this implement/fix?

The goal of this PR is to add a Ipyvolume-based 3d backend to the visualization capabilitites of MNE and inside a jupyter notebook in particular.

Additional information

It is a work in progress, some features are still missing or incomplete as of now.

@larsoner
Copy link
Member

larsoner commented Jun 6, 2019

  1. This:
    t = mne.viz.plot_alignment(info, trans, subject=subject, dig=True,
                              meg=['helmet', 'sensors'], subjects_dir=subjects_dir,
                              surfaces=['head', 'inner_skull', 'outer_skull'])
    
    Only shows one surface:
    Screenshot from 2019-06-06 09-55-14
    Perhaps depth testing needs to be disabled?
  2. The surface above looks weird. Perhaps alpha blending and/or backface culling needs to be enabled?
  3. The surface is not smooth shaded, it's flat shaded. If this is still WIP at the ipyvolume/pythreejs end, then put this in the Notes section as suggested above about **Backend-specific notes**.
  4. Adding a source space:
    src = subjects_dir + '/sample/bem/sample-oct-6-src.fif'
    t = mne.viz.plot_alignment(info, trans, subject=subject, dig=True,
                              meg=['helmet', 'sensors'], subjects_dir=subjects_dir,
                              surfaces=['head', 'inner_skull', 'outer_skull'], src=src)
    
    Does not plot anything, and does not emit an error in Python, but the Chrome console says:
    Screenshot from 2019-06-06 09-58-21

@GuillaumeFavelier
Copy link
Contributor Author

Thanks for the suggesting 1. and 2., I'll try those flags out right now.
3. is normally already written in the 3d backends overview in the section 'Smooth Shading' normally.
Thanks for reporting 4. I'll investigate what's going on here too

@lgtm-com
Copy link

lgtm-com bot commented Jun 6, 2019

This pull request introduces 1 alert when merging 3ce92e0 into c2b9176 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jun 7, 2019

Thanks for the refactoring @massich !

The recent modifications should fix the scenario 4. you had @larsoner (the quiver3d() function with the cylinder glyph was not supported yet).
Now solving 1. and 2. is a matter of finding a good set of parameters to obtain decent transparency. I'm working on it.

@GuillaumeFavelier GuillaumeFavelier changed the title [MRG] Add Ipyvolume 3d backend [WIP] Add Ipyvolume 3d backend Jun 7, 2019
@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2019

This pull request introduces 4 alerts when merging 442f8b3 into f4b66b5 - view on LGTM.com

new alerts:

  • 3 for Unused local variable
  • 1 for Unreachable code

@larsoner
Copy link
Member

larsoner commented Jun 8, 2019

@GuillaumeFavelier the rendering does not need to be perfect, just need to be able to see the three surfaces. Over in IPyvolume you show that even though it's not 100% correct you can see three overlaid meshes. Is that the case here now, too?

@GuillaumeFavelier
Copy link
Contributor Author

With the latest version, even with backface_culling and my tests on depthTest and depthWrite I get something close to this (we still can't see the brain surface inside without zooming):

image

The most satifying resut I got was inverting the processing order during depth test but this is a workaround that works only for this example:

image

My next test is about the rendering order which could be another promising way to achieve a decent result.

@GuillaumeFavelier GuillaumeFavelier self-assigned this Jun 17, 2019
@GuillaumeFavelier GuillaumeFavelier changed the title [WIP] Add Ipyvolume 3d backend WIP: Add Ipyvolume 3d backend Jun 17, 2019
@larsoner
Copy link
Member

larsoner commented Jul 9, 2019

@GuillaumeFavelier what do you think we should do here? Abandon for now due to alpha limitations?

@agramfort
Copy link
Member

agramfort commented Jul 9, 2019 via email

@larsoner
Copy link
Member

larsoner commented Jul 9, 2019

Okay I'll close to clear up a bit of backlog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants