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: Link cameras in _LinkViewer #7962

Merged
merged 14 commits into from
Jul 23, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jul 6, 2020

This PR adds parameters to link_brains() to decide what to link: time controller and/or camera controls.

ToDo

  • Fix camera update delay
  • Update testing in test_3d.py
  • Rename the time_viewer used as reference into leader
  • Check stc time infos
  • Link time course canvas

It's an item of #7162
Related to #7959

@GuillaumeFavelier GuillaumeFavelier self-assigned this Jul 6, 2020
@GuillaumeFavelier GuillaumeFavelier changed the title Link cameras in _LinkViewer WIP: Link cameras in _LinkViewer Jul 6, 2020
@agramfort
Copy link
Member

@bloyl can you give it a try?

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jul 6, 2020
86 tasks
mne/viz/_3d.py Outdated Show resolved Hide resolved
mne/viz/_brain/_timeviewer.py Outdated Show resolved Hide resolved
@bloyl
Copy link
Contributor

bloyl commented Jul 20, 2020

Sorry for the delay in testing this.

This is pretty cool and does what I'd like.

A couple of things I noticed.

  1. The camera doesn't force an update on the other renderer so things get laggy. I assume this is what you mean by Fix camera update delay in the description. but thought I'd mention it.
  2. The time linking only works on the slider. if you click in the time course plot only the one brain updates.
  3. You are currently allowed to time link brains that have different time axis. but since the linking is on time index this causes things to get out of sync.

My suggestion for point 3 would be use the actual time as opposed to the index to do the linking. Additionally I would add a time='auto' option as the default. this would use time=True if the linked brain objects all have the time array and false if they don't, supporting both linked multiple evoked STCs and unlinked stcs (say evoked and a PSD) with the default. Additonally if a user had evoked stcs with different time axis that were actually comparable (maybe historic data or something) they could force time=True to still have the linking.

@GuillaumeFavelier
Copy link
Contributor Author

The camera doesn't force an update on the other renderer so things get laggy.

The cameras are linked but the event "the camera has been modified" is not captured simply because I don't know how to do that. And since #7517, the plotter is not updated automatically. I see 2 ways to fix this. Either capture the vtk event (interactor, camera...) or allow automatic update again.

The time linking only works on the slider

Nice catch! I'll take care of this :)

You are currently allowed to time link brains that have different time axis.

Hm, this is not intended... I have to check the time axes somehow. @larsoner, @agramfort any idea on how to deal with this?

@agramfort
Copy link
Member

agramfort commented Jul 21, 2020 via email

@GuillaumeFavelier
Copy link
Contributor Author

  1. and 3) should be fixed by now. I'll investigate 1) and see if I can capture the right VTK event to update the plotters. Otherwise, I'll have to re-enable automatic update to mitigate.

@GuillaumeFavelier
Copy link
Contributor Author

It turns out, the camera ModifiedEvent does the trick! I invite you to try it out @bloyl :)

output

Reference: http://vtk.1045678.n5.nabble.com/Vtk-Camera-Postion-Event-td5727757.html

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Link cameras in _LinkViewer MRG: Link cameras in _LinkViewer Jul 21, 2020
Comment on lines 1207 to 1213
# check time infos
brain_times = [brain._times for brain in brains]
if not all(len(x) == len(brain_times[0]) for x in brain_times):
time = False
if not all(all(np.equal(x, brain_times[0])) for x in brain_times):
time = False

Copy link
Contributor

Choose a reason for hiding this comment

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

This second check throws ValueError: operands could not be broadcast if the brain_times are of different lengths and more than one.

why not only do the check if you have to (time == True)

        if time and not all(len(x) == len(brain_times[0]) for x in brain_times):
            time = False
        if time and not all(all(np.equal(x, brain_times[0])) for x in brain_times):
            time = False

to test just link a plot of an evoked stc with a cropped copy of the same stc

@bloyl
Copy link
Contributor

bloyl commented Jul 21, 2020

Other than my comment above this is working for me.

Additional question, how did you make the 4 panel figure on the right?

@GuillaumeFavelier
Copy link
Contributor Author

how did you make the 4 panel figure on the right?

I use the following parameters: hemi='split', views=['lat', 'med']

@GuillaumeFavelier
Copy link
Contributor Author

Feel free to have a look or even test it @agramfort, @larsoner

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@larsoner larsoner merged commit 6e5beb6 into mne-tools:master Jul 23, 2020
@GuillaumeFavelier GuillaumeFavelier deleted the linkviewer_camera branch July 23, 2020 12:27
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