Skip to content

MRG: Reset the viz settings in _TimeViewer#8125

Merged
larsoner merged 9 commits intomne-tools:masterfrom
GuillaumeFavelier:viz_init
Aug 20, 2020
Merged

MRG: Reset the viz settings in _TimeViewer#8125
larsoner merged 9 commits intomne-tools:masterfrom
GuillaumeFavelier:viz_init

Conversation

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Aug 17, 2020 •
edited
Loading

This PR reworks the interactor in _Brain when using flatmaps and adds a button to reset the initial time index and camera settings "Reset".

  • Use vtkInteractorStyleRubberBand2D with parallel projection
  • Find a suitable icon
  • Build the qt icon resources
  • Fix conflicts
  • try/except or hasattr for "RubberBand2D"
  • Check the MNE logo

It's an item of #7162
Related to #8082 (comment)

@GuillaumeFavelier GuillaumeFavelier self-assigned this Aug 17, 2020
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Aug 17, 2020

Could we use a 2D interactor instead like

https://vtk.org/doc/nightly/html/classvtkInteractorStyleRubberBand2D.html

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Aug 18, 2020
Closed
86 tasks
@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Aug 18, 2020

Could we use a 2D interactor

Yes indeed! In my experiments it works assuming that the camera uses parallel projection :)

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Aug 18, 2020

These are the results for "restore" on material.io:

1 2 3 4 5
image image image image image

4 is already used for "Restore Scale". I put it in case we would swap for something else.

Alternatively:

cancel flip repeat
image image image

I searched "reset" and "init" without success.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Aug 18, 2020

I prefer 1 personally.

Any opinion @agramfort , @larsoner, @hoechenberger, @drammock ?

@hoechenberger
Copy link
Copy Markdown
Member

hoechenberger commented Aug 18, 2020

I think 1 could be a nice choice :)

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Aug 18, 2020 via email

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Aug 18, 2020

I'll go ahead with 1 then

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Reset the viz settings in _TimeViewer MRG: Reset the viz settings in _TimeViewer Aug 18, 2020
larsoner
larsoner reviewed Aug 18, 2020
View reviewed changes
mne/viz/backends/_pyvista.py Outdated
if interaction == "rubber_band":
for renderer in self.plotter.renderers:
renderer.enable_parallel_projection()
style = vtk.vtkInteractorStyleRubberBand2D()
Copy link
Copy Markdown
Member

@larsoner larsoner Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you open a PR to pyvista to add this as a mode? Then we can try/except or hasattr here

Copy link
Copy Markdown
Member

@larsoner larsoner Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For example, this mode will suffer from the "click-and-drag-not-captured" bug, because the fix for this is in PyVista subclassing the VTK interactor style and dealing with button presses.)

Copy link
Copy Markdown
Contributor Author

@GuillaumeFavelier GuillaumeFavelier Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you open a PR to pyvista to add this as a mode?

Sure, will do.

I use middle-click to drag with this one. I was not aware of this bug.

Copy link
Copy Markdown
Member

@larsoner larsoner Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyvista/pyvista#116

Copy link
Copy Markdown
Member

@larsoner larsoner Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It's possible we also need to capture middle clicks, in which case hopefully from pyvista/pyvista#854 it's clear what you'd need to do to capture those, too)

@GuillaumeFavelier GuillaumeFavelier changed the title MRG: Reset the viz settings in _TimeViewer WIP: Reset the viz settings in _TimeViewer Aug 18, 2020
@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Aug 19, 2020
Closed
@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Aug 19, 2020

@larsoner I opened pyvista/pyvista#872

Also, could you check the that mne logo is exactly like you want? I had the following message while building it:

findfont: Font family ['sans-serif'] not found. Falling back to DejaVu Sans.

Maybe that was not a good idea but I tried multiple packages for my linux distribution without any success.

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Aug 19, 2020

Also, could you check the that mne logo is exactly like you want? I had the following message while building it:

The MNE logo in the resources is a PNG. I'm not sure why this would happen.

But FWIW you can get the Primetime font listed in the logo generation script for free here

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Reset the viz settings in _TimeViewer MRG: Reset the viz settings in _TimeViewer Aug 20, 2020
@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Aug 20, 2020

The MNE logo in the resources is a PNG. I'm not sure why this would happen.

The logo is not tracked in mne/icons like the other icons and I built the Qt resources to integrate the new "Reset" icon. To fix the conflicts, I had to regenerate the resources. That's why I wanted to check that everything is okay.

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Aug 20, 2020

But you shouldn't have gotten any font warning from running the icon resource generation from the mne logo because it's not a vector format, but rather bitmap. Did you also need to run something from logo/? Or did you just run the icons/ script? Can you got bisect to verify it's coming from the logo commit?

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Aug 20, 2020

On a system without the Primetime font, I can reproduce with:

$ cd mne/logo
$ python generate_mne_logos.py 
findfont: Font family ['sans-serif'] not found. Falling back to DejaVu Sans.

But you should not have needed to call generate_mne_logos.py unless you were changing the logo. You should just be able to do:

$ cd ../mne/icons
$ python resources.py

which does not emit any warning.

... but then I realized that the logo actually was not saved as part of the git repo! So now I see why you had to do all of this. I'll push a commit that adds the PNG so that people don't have to do it in the future, and also regenerate the resources.

larsoner
larsoner approved these changes Aug 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great for me

@larsoner larsoner added this to the 0.21 milestone Aug 20, 2020
@larsoner larsoner merged commit 0686c14 into mne-tools:master Aug 20, 2020
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Aug 20, 2020

Thanks @GuillaumeFavelier

@GuillaumeFavelier GuillaumeFavelier deleted the viz_init branch August 20, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort approved these changes

@larsoner larsoner larsoner approved these changes

Assignees

@GuillaumeFavelier GuillaumeFavelier

Labels

VIZ

Projects

None yet

Milestone

0.21

Development

Successfully merging this pull request may close these issues.

4 participants

@GuillaumeFavelier @larsoner @hoechenberger @agramfort