Skip to content

Rework _Brain show #7580

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.

Sign up for GitHub

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

Jump to bottom
Merged
larsoner merged 3 commits into from
Apr 8, 2020
Merged

Rework _Brain show #7580

larsoner merged 3 commits into from
Apr 8, 2020

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Apr 8, 2020

TL:DR: _TimeViewer is displayed only once, when the UI is ready.

The default plotter of the pyvista backend is interactive as soon as it opens that's why one can notice every piece of the interface being added when time_viewer=True resulting in the jerkiness mentioned by @hoechenberger in #7572 (comment) and @jasmainak in #7247 (comment).

This PR fixes this behaviour by showing the _TimeViewer only when when it's ready (once every GUI element is set up). To achieve that I have to make _Brain aware that time_viewer is set. So I introduced the show parameter.

This way, the behaviour of _Brain is more consistent with all the basic _3d.py primitives and the show parameter is now connected to the rest of the viz pipeline.

@GuillaumeFavelier GuillaumeFavelier self-assigned this Apr 8, 2020
@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 8, 2020

What do you think of this change @agramfort, @larsoner ? Does it fix the 'jerkiness' for you @hoechenberger ?

@codecov
Copy link

codecov bot commented Apr 8, 2020 •
edited
Loading

Codecov Report

Merging #7580 into master will decrease coverage by 0.16%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #7580      +/-   ##
==========================================
- Coverage   90.20%   90.04%   -0.17%     
==========================================
  Files         452      452              
  Lines       82104    82951     +847     
  Branches    13109    13113       +4     
==========================================
+ Hits        74064    74690     +626     
- Misses       5206     5407     +201     
- Partials     2834     2854      +20

@larsoner
Copy link
Member

larsoner commented Apr 8, 2020

@GuillaumeFavelier it fixes the jerkiness of the controls. Is it possible to also fix the jerkiness of the bottom matplotlib plot when show_traces=True? In theory you could create that window and embed the matplotlib and (complete) pyvista views, then show it, right?

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 8, 2020

could create that window and embed the matplotlib and (complete) pyvista views, then show it

should be possible indeed, I'll take a look.

@hoechenberger
Copy link
Member

hoechenberger commented Apr 8, 2020

Just tried it out with @GuillaumeFavelier together via Zoom on macOS. Works great, if one ignores the "traces" window issue @larsoner mentioned. If this could be fixed, I'd be happy. :)

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 8, 2020

I switched my desktop window manager from i3 to KDE but I can't reproduce the jumpy behaviour with show_traces=True. For me, the window appears complete. I tried PyQt5 5.11.3 and 5.14.1.

So I'll try by showing the matplotlib canvas hopefully to update the layout calculation before the final show() request.

@larsoner
Copy link
Member

larsoner commented Apr 8, 2020

The latest commit appears to have fixed all jerkiness for me, thanks @GuillaumeFavelier !

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 8, 2020

The latest commit appears to have fixed all jerkiness for me

Nice! And what about you @hoechenberger ?

@hoechenberger
Copy link
Member

hoechenberger commented Apr 8, 2020 •
edited
Loading

There is only a minor jerkiness left here, which occurs when show_traces=True: at a very late stage of window composition, the status bar pops in (or maybe it's the window being resized so the status bar becomes visible, cannot tell). This doesn't happen when show_traces=False. Aside from that, it's great!

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 8, 2020

Oh the status bar! I should enable it once and for all xD

@agramfort
Copy link
Member

agramfort commented Apr 8, 2020 via email

@hoechenberger
Copy link
Member

hoechenberger commented Apr 8, 2020 •
edited
Loading

Sorry it's the exact opposite of what I wrote above: status bar pops in when show_traces=False.

In both cases, show_traces=False and show_traces=True, the window is created with a status bar eventually. It just seems that when show_traces=False, the bar is either created later or, as I said, becomes visible due to a resize.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 8, 2020

What I mean is that the status bar is not supposed to pop in any way. This is not intended.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 8, 2020

I don't see the point to enable it in this PR without anything to populate it right now. It was done in #7257 to get info about the calculation but got reverted.

Is it okay for you if I work on the status bar in another PR and work towards fixing the jerkiness at that time?

@hoechenberger
Copy link
Member

hoechenberger commented Apr 8, 2020

What I mean is that the status bar is not supposed to pop in any way. This is not intended.

Maybe it's a macOS & Qt thing then. BUUUT I have some ideas how to use the status bar ;))

@hoechenberger
Copy link
Member

hoechenberger commented Apr 8, 2020

@GuillaumeFavelier Only now I see your previous comment. Yes, that's totally alright, do what you need to do :)

@larsoner larsoner merged commit f3eb193 into mne-tools:master Apr 8, 2020
@larsoner
Copy link
Member

larsoner commented Apr 8, 2020

Thanks @GuillaumeFavelier

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Apr 9, 2020
Merged
larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 10, 2020
@larsoner
Merge remote-tracking branch 'upstream/master' into cola
002bd0a 
* upstream/master: (1522 commits)
  FIX: Show bug
  MRG, FIX: Datetime call in gdf 2.x age calculation (mne-tools#7581)
  DOC: Simplify Darwin installation (mne-tools#7584)
  MRG, ENH: Allow picking without preload (mne-tools#7507)
  DOC: Document anonymization better (mne-tools#7587)
  Rework _Brain show (mne-tools#7580)
  DOC: Fixes in tutorial (mne-tools#7579)
  ENH: muscle artifact detection (mne-tools#7407)
  MRG: Remove toolbars in PyVista plotter (mne-tools#7572)
  WIP: Deregister plotter from the figure list in close() (mne-tools#7573)
  MRG: Fix mouse wheel event in _TimeViewer (mne-tools#7563)
  FIX: Fix toggle all (mne-tools#7567)
  MRG, FIX: parallel n_jobs check (mne-tools#7566)
  Rename artifact detection to movement detection (mne-tools#7569)
  ENH: Update spelling check [ci skip] (mne-tools#7565)
  MRG, ENH: Dont require preload for raw resample (mne-tools#7508)
  MRG: Add interpolation for NIRS signals (mne-tools#7428)
  WIP: Add temporal derivative distribution repair algorithm (mne-tools#7556)
  DOC: fix link in docstr [skip ci] (mne-tools#7562)
  ENH: Custom figure title when plotting Dipole locations (mne-tools#7558)
  ...
@GuillaumeFavelier GuillaumeFavelier deleted the brain_show branch June 11, 2020 09:43
larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 25, 2023
@larsoner
Merge remote-tracking branch 'upstream/master' into cola
04dabfd 
* upstream/master: (1522 commits)
  FIX: Show bug
  MRG, FIX: Datetime call in gdf 2.x age calculation (mne-tools#7581)
  DOC: Simplify Darwin installation (mne-tools#7584)
  MRG, ENH: Allow picking without preload (mne-tools#7507)
  DOC: Document anonymization better (mne-tools#7587)
  Rework _Brain show (mne-tools#7580)
  DOC: Fixes in tutorial (mne-tools#7579)
  ENH: muscle artifact detection (mne-tools#7407)
  MRG: Remove toolbars in PyVista plotter (mne-tools#7572)
  WIP: Deregister plotter from the figure list in close() (mne-tools#7573)
  MRG: Fix mouse wheel event in _TimeViewer (mne-tools#7563)
  FIX: Fix toggle all (mne-tools#7567)
  MRG, FIX: parallel n_jobs check (mne-tools#7566)
  Rename artifact detection to movement detection (mne-tools#7569)
  ENH: Update spelling check [ci skip] (mne-tools#7565)
  MRG, ENH: Dont require preload for raw resample (mne-tools#7508)
  MRG: Add interpolation for NIRS signals (mne-tools#7428)
  WIP: Add temporal derivative distribution repair algorithm (mne-tools#7556)
  DOC: fix link in docstr [skip ci] (mne-tools#7562)
  ENH: Custom figure title when plotting Dipole locations (mne-tools#7558)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner approved these changes

Assignees

@GuillaumeFavelier GuillaumeFavelier

Labels

VIZ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@GuillaumeFavelier @larsoner @hoechenberger @agramfort