Skip to content

FIX: Brain lights#8972

Merged
larsoner merged 2 commits intomne-tools:mainfrom
GuillaumeFavelier:brain/light
Mar 1, 2021
Merged

FIX: Brain lights#8972
larsoner merged 2 commits intomne-tools:mainfrom
GuillaumeFavelier:brain/light

Conversation

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Mar 1, 2021

This PR fixes the 3 light system:

PR master
image image

The patch is just shared to all renderers. Such a function is already implemented on pyvista master so the function update_lighting() will be replaced by enable_3_lights() in the future.

@GuillaumeFavelier GuillaumeFavelier self-assigned this Mar 1, 2021
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 1, 2021

Perhaps a separate bug, it looks like the lighting is asymmetric:

  1. the back/posterior of the left hemisphere and the front/anterior of the right hemisphere are illuminated, and
  2. the front/anterior of the left hemisphere and the back/posterior of the right hemisphere are dark

Are the lights set up to illuminate equivalently the left and right? Are they attached to the camera, or to the world?

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 1, 2021

Are the lights set up to illuminate equivalently the left and right?

I think we use the _orient_lights() function for that:

mne-python/mne/viz/_brain/_brain.py

Lines 528 to 529 in 2c485d5

if hemi == 'rh' and hasattr(self._renderer, "_orient_lights"):
self._renderer._orient_lights()

mne-python/mne/viz/backends/_pyvista.py

Lines 265 to 270 in 2c485d5

def _orient_lights(self):
lights = list(self.plotter.renderer.GetLights())
lights.pop(0) # unused headlight
lights[0].SetPosition(_to_pos(45.0, -45.0))
lights[1].SetPosition(_to_pos(-30.0, 60.0))
lights[2].SetPosition(_to_pos(-30.0, -60.0))

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 1, 2021

Are they attached to the camera, or to the world?

Light 0 is a Headlight, Light 1 and Light 2 are CameraLights.

And according to VTK doc:

A Headlight is always located at the camera and is pointed at the camera's focal point. The renderer is free to modify the position and focal point of the camera at any time.

A CameraLight is also attached to the camera, but is not necessarily located at the camera's position. CameraLights are defined in a coordinate space where the camera is located at (0, 0, 1), looking towards (0, 0, 0) at a distance of 1, with up being (0, 1, 0). CameraLight uses the transform matrix to establish this space.

Reference: https://vtk.org/doc/nightly/html/classvtkLight.html#a1e0175706ef066ca837726425bdcc6d7

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 1, 2021

Okay here it was on your commit:

Screenshot from 2021-03-01 12-43-03

And here it is on mine that I just pushed -- the lighting is uniform left/right which I think is important (otherwise you end up with the asymmetry I mention above):

Screenshot from 2021-03-01 13-00-01

Okay for you @GuillaumeFavelier ?

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 1, 2021

Yes, all good 👌

@larsoner larsoner merged commit 04d3100 into mne-tools:main Mar 1, 2021
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 1, 2021

Thanks @GuillaumeFavelier !

larsoner added a commit to marsipu/mne-python that referenced this pull request Mar 1, 2021
@larsoner
Merge remote-tracking branch 'upstream/main' into neuromag122_adjacency
8eec554 
* upstream/main:
  MAINT: Skip matplotlib pre for now (mne-tools#8973)
  FIX: Brain lights (mne-tools#8972)
  MNT: Migrate VTK Widgets (mne-tools#8862)
  Fix (mne-tools#8971)
  Fix indexing dipoles read from a bdip file (mne-tools#8963)
larsoner added a commit to adam2392/mne-python that referenced this pull request Mar 1, 2021
@larsoner
Merge remote-tracking branch 'upstream/main' into epochstfr
bbafc83 
* upstream/main:
  MAINT: Skip matplotlib pre for now (mne-tools#8973)
  FIX: Brain lights (mne-tools#8972)
  MNT: Migrate VTK Widgets (mne-tools#8862)
  Fix (mne-tools#8971)
larsoner added a commit to bloyl/mne-python that referenced this pull request Mar 1, 2021
@larsoner
Merge remote-tracking branch 'upstream/main' into chpi_ordering
c050561 
* upstream/main:
  MAINT: Skip matplotlib pre for now (mne-tools#8973)
  FIX: Brain lights (mne-tools#8972)
  MNT: Migrate VTK Widgets (mne-tools#8862)
  Fix (mne-tools#8971)
  Fix indexing dipoles read from a bdip file (mne-tools#8963)
@GuillaumeFavelier GuillaumeFavelier deleted the brain/light branch March 3, 2021 10:32
@larsoner larsoner mentioned this pull request Oct 26, 2021
Merged
41 tasks
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

Assignees

@GuillaumeFavelier GuillaumeFavelier

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GuillaumeFavelier @larsoner @agramfort