Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MRG: Reset the viz settings in _TimeViewer #8125

Merged
merged 9 commits into from
Aug 20, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Aug 17, 2020

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)

@larsoner
Copy link
Member

Could we use a 2D interactor instead like

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

@GuillaumeFavelier
Copy link
Contributor Author

Could we use a 2D interactor

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

@GuillaumeFavelier
Copy link
Contributor Author

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
Contributor Author

I prefer 1 personally.

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

@hoechenberger
Copy link
Member

I think 1 could be a nice choice :)

@agramfort
Copy link
Member

agramfort commented Aug 18, 2020 via email

@GuillaumeFavelier
Copy link
Contributor Author

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
if interaction == "rubber_band":
for renderer in self.plotter.renderers:
renderer.enable_parallel_projection()
style = vtk.vtkInteractorStyleRubberBand2D()
Copy link
Member

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
Member

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
Contributor Author

@GuillaumeFavelier GuillaumeFavelier 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?

Sure, will do.

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

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
Copy link
Contributor Author

@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
Member

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
Contributor Author

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
Member

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
Member

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.

Copy link
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
Member

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants