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

FIX: Brain lights #8972

Merged
merged 2 commits into from
Mar 1, 2021
Merged

FIX: Brain lights #8972

merged 2 commits into from
Mar 1, 2021

Conversation

GuillaumeFavelier
Copy link
Contributor

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

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

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

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

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

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

Yes, all good 👌

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

larsoner commented Mar 1, 2021

Thanks @GuillaumeFavelier !

larsoner added a commit to marsipu/mne-python that referenced this pull request Mar 1, 2021
* 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
* 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
* 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
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants