Skip to content

MRG, ENH: Allow disabling FXAA#7877

Merged
larsoner merged 5 commits intomne-tools:masterfrom
larsoner:fxaa
Jun 10, 2020
Merged

MRG, ENH: Allow disabling FXAA#7877
larsoner merged 5 commits intomne-tools:masterfrom
larsoner:fxaa

Conversation

@larsoner
Copy link
Copy Markdown
Member

@larsoner larsoner commented Jun 8, 2020

@bloyl @kingjr can you see if this fixes your OSMESA / remote rendering experience in mne coreg and stc.plot? For the latter you'll also need to pass stc.plot(..., antialias=False) and if you use PySurfer you'll need to be on nipy/PySurfer#295

@GuillaumeFavelier can you take a look and make sure I disabled it in the right place(s)?

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

GuillaumeFavelier commented Jun 8, 2020

Everything looks okay to me @larsoner

@bloyl
Copy link
Copy Markdown
Contributor

bloyl commented Jun 8, 2020

I'll look more closely later today but this looks like it only fixes stc.plot?

Would it be possible to use a global config variable that is respected by the 3d renderers? That would have the benefit of fixing all 3d plotting (plot_alignment, etc.) and also let the same code work in a number of settings.

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jun 8, 2020

Ahh right, we'd need to update every 3D plotting function to take this argument. That's probably not so nice. We could have something like a mne.viz.set_3d_options(antialias=None) -- for now it would just be antialias as the only arg but eventually we might want to add other things (depth peeling?). @GuillaumeFavelier WDYT? Other API ideas? And also maybe we want a new MNE_3D_OPTION_ANTIALIAS so that users that always want this off can just set a bash env var and never worry about it.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

GuillaumeFavelier commented Jun 8, 2020

update every 3D plotting function to take this argument

If it's global, why update every function? It's not like you would like to have specific plots with AA and some without it. I think we could just modify the renderers directly and mne.coreg. It's less painful to maintain too.

I like set_3d_options. Do we have a place to save such settings or we can just modify os.environ?

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jun 8, 2020

I like set_3d_options. Do we have a place to save such settings or we can just modify os.environ?

We shouldn't use os.environ to pass things around unless we need to. I think I know a simple way to implement it, let me give it a shot...

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jun 8, 2020 •
edited
Loading

If it's global, why update every function?

I meant if we followed what I was originally doing we'd need to. With the global approach (implemented now), indeed we don't.

@GuillaumeFavelier can you look at the changes and let me know what you think? @bloyl in principle this should now work with all 3D functions in MNE. You can do either mne.viz.set_3d_options(antialias=False) in a given session, or set MNE_3D_OPTIONS_ANTIALIAS MNE_3D_OPTION_ANTIALIAS=false as an env var.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

GuillaumeFavelier commented Jun 8, 2020

mne/viz/_3d.py is a nice place indeed, thanks for adding this. Also, LGTM

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jun 9, 2020
Merged
11 tasks
@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

GuillaumeFavelier commented Jun 9, 2020

I pushed a commit to fix the import in _pyvista.py

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jun 9, 2020

Okay @bloyl @kingjr let us know if this helps and we'll go ahead and merge

@bloyl
Copy link
Copy Markdown
Contributor

bloyl commented Jun 10, 2020

mne.viz.set_3d_options in code works.
MNE_3D_OPTIONS_ANTIALIAS=false does not. I think this is because of what I'm about to highlight

@bloyl
Copy link
Copy Markdown
Contributor

bloyl commented Jun 10, 2020

Sorry your comment above used OPTIONS but the code uses OPTION
MNE_3D_OPTION_ANTIALIAS=true does work.

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jun 10, 2020

MNE_3D_OPTIONS_ANTIALIAS should not, but MNE_3D_OPTION_ANTIALIAS (no plural on OPTION) should

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Jun 10, 2020

Great! Glad it works. @kingjr feel free to try it as well, but I assume you're going to be in the same boat as @bloyl.

Just realized I should add this to the FAQ about remote rendering, then I'll merge.

@larsoner larsoner merged commit 3fdd502 into mne-tools:master Jun 10, 2020
@larsoner larsoner deleted the fxaa branch June 10, 2020 15:31
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 17, 2020
@larsoner
Merge remote-tracking branch 'upstream/master' into refactor
59db30a 
* upstream/master: (24 commits)
  WIP: Fix Travis (mne-tools#7906)
  WIP: Prototype of notebook viz (screencast) (mne-tools#7758)
  MRG, FIX: Speed up I/O tests, mark some slow (mne-tools#7904)
  Proper attribution for Blender tutorial (mne-tools#7900)
  MAINT: Check usage [ci skip] (mne-tools#7902)
  Allow find_bad_channels_maxwell() to return scores (mne-tools#7845)
  Warn if NIRx directory structure has been modified from original format (mne-tools#7898)
  Pin pvyista to 0.24.3 (mne-tools#7899)
  MRG: Add support for reading and writing sufaces to .obj (mne-tools#7824)
  Fix _auto_topomap_coords docstring. (mne-tools#7895)
  MRG, FIX: Ensure Info H5-writeable (mne-tools#7887)
  Website contents (mne-tools#7889)
  MRG, ENH: Add mri_resolution="sparse" (mne-tools#7888)
  MRG, ENH: Allow disabling FXAA (mne-tools#7877)
  remove "and and" [ci skip] (mne-tools#7882)
  fix evoked nave → inverse guidance (mne-tools#7881)
  ENH: Better error messages (mne-tools#7879)
  FIX : EDF+ Annotation Timestamps missing sub-second accuracy (mne-tools#7875)
  FIX: Fix get_channel_types (mne-tools#7878)
  MRG, BUG: Fix combine evokeds (mne-tools#7869)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

VIZ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@larsoner @GuillaumeFavelier @bloyl