Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jan 19, 2022

This PR :

  • renames the MRI groupbox from "Transform" to "HEAD <> MRI Transform"
  • brings an immediate solution to ICP "spamming" by disabling all the fit buttons
  • fixes the range of the x/y/z spin boxes
  • correcty updates the plot when the trans parameter is set
  • fix the x/y/z scaling spin boxes increment
  • improve the UX in scale mode "uniform"

Related to #10117 and #8833

@GuillaumeFavelier GuillaumeFavelier self-assigned this Jan 19, 2022
@larsoner
Copy link
Member

adds a slider for the head surface opacity (it effectively replaces the previous head_transparency and its automatic opacity management)

I don't think we should get rid of some of the opacity management. I think that whenever the fiducials are unlocked, we should go to a fully-opaque state. (It's okay to let the user change it from this, but I doubt they will.) Once they lock the fiducials, it should go back to its previous opacity (or to the default opacity if the UI launched in edit-fiducials mode).

fixes the range of the x/y/z combo boxes

Make sure you fix the step, too -- I'd go for 1 mm, 1 degree, and 1 percent for the translation, rotation, and scaling, respectively.

Let me know when I should test!

@GuillaumeFavelier
Copy link
Contributor Author

How is it now @larsoner ?

@larsoner
Copy link
Member

correcty updates the plot when the trans parameter is set

The plot, yes:

mne coreg -s sample -d subjects -f MEG/sample/sample_audvis_raw.fif --trans MEG/sample/sample_audvis_raw-trans.fif

The rX/rY/rZ, no -- they should not all be 1. Did you fix the range of these so that they are always >= 1? I think these actually in practice go from something like -180 to 180, but please do check the range. It could even be -360 to 360 degrees, not sure...

The translations similarly seem fixed to >= 1, when they should be allowed to range at least -100 to 100 (mm), maybe even -500 to 500 just in case someone's MRI has a very weird affine.

The range of 1 to 1000 percent for scaling seems reasonable, though.

@larsoner
Copy link
Member

In fixing a test I broke flake, so please fix this while fixing the above stuff:

Error: ./mne/gui/tests/test_coreg_gui.py:178:80: E501 line too long (81 > 79 characters)

@hoechenberger
Copy link
Member

hoechenberger commented Jan 20, 2022

  • The slider behaves in an odd fashion: there appears to be some kind of a huge step in the beginning, before transparency can be adjusted smoothly (see video below)
  • I thought we had discussed not to add a slider (or any transparency adjustments) if we figure that we can live with a fixed opacity level of 95% – am I mistaken here?
Screen.Recording.2022-01-20.at.08.45.30.mov

@larsoner
Copy link
Member

am I mistaken here?

I think you voted against a slider but @drammock and I voted for a slider

@GuillaumeFavelier GuillaumeFavelier changed the title FIX/ENH: some coreg features and fixes WIP,FIX,ENH: some coreg features and fixes Jan 20, 2022
@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jan 20, 2022

The slider behaves in an odd fashion

Hm... I believe this is how vtk handles transparency. I have the same on my linux. I would like to have a smoother transition too

@larsoner
Copy link
Member

@GuillaumeFavelier should I test again? Did you test locally a bit to make sure these things worked properly?

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jan 20, 2022

Make sure you fix the step, too -- I'd go for 1 mm, 1 degree, and 1 percent for the translation, rotation, and scaling, respectively.

I did not check this yet.

I still have to deprecate head_transparency in favor of head_opacity (after #10222)


I also found a bug with mark_inside when using scale mode "uniform", all the hsp become black as if they are detected inside the head surface:

image

@hoechenberger
Copy link
Member

@GuillaumeFavelier @larsoner i will also test tonight (currently out for a walk) and can report back!

@GuillaumeFavelier GuillaumeFavelier changed the title WIP,FIX,ENH: some coreg features and fixes FIX,ENH: some coreg features and fixes Jan 26, 2022
@GuillaumeFavelier
Copy link
Contributor Author

After extracting some code from it, now this PR:

  • renames the MRI groupbox from "Transform" to "HEAD <> MRI Transform"
  • brings an immediate solution to ICP "spamming" by disabling all the fit buttons
  • fixes the range of the x/y/z spin boxes
  • correcty updates the plot when the trans parameter is set
  • fix the x/y/z scaling spin boxes increment
  • improve the UX in scale mode "uniform"

@GuillaumeFavelier GuillaumeFavelier changed the title FIX,ENH: some coreg features and fixes ENH: some coreg features and fixes Jan 26, 2022
@GuillaumeFavelier GuillaumeFavelier changed the title ENH: some coreg features and fixes WIP,ENH: some coreg features and fixes Jan 27, 2022
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 27, 2022
Partially extraced from mne-tools#10242 and mne-tools#10224

- status bar is now stored in `self._widgets`
- `_forward_widget_command()` has a docstring and new functionality
  (extracted from mne-tools#10224)
GuillaumeFavelier pushed a commit that referenced this pull request Jan 27, 2022
Partially extraced from #10242 and #10224

- status bar is now stored in `self._widgets`
- `_forward_widget_command()` has a docstring and new functionality
  (extracted from #10224)
@GuillaumeFavelier GuillaumeFavelier changed the title WIP,ENH: some coreg features and fixes ENH: some coreg features and fixes Jan 28, 2022
@GuillaumeFavelier
Copy link
Contributor Author

I fixed the conflicts, can you test it @hoechenberger ?

@GuillaumeFavelier
Copy link
Contributor Author

62ae738 also fixes:

Aligning using ICP
Start     median distance:  17.40 mm
  ICP  1  median distance:  12.46 mm
Traceback (most recent call last):
  File "<decorator-gen-563>", line 24, in _redraw
  File "/Users/hoechenberger/Development/mne-python/mne/gui/_coreg.py", line 692, in _redraw
    for key in self._redraws_pending:
RuntimeError: Set changed size during iteration

@GuillaumeFavelier
Copy link
Contributor Author

This is ready for review @larsoner, @hoechenberger

@GuillaumeFavelier
Copy link
Contributor Author

@hoechenberger as per your offline review, I added 4ec8b39

@GuillaumeFavelier GuillaumeFavelier changed the title ENH: some coreg features and fixes WIP,ENH: some coreg features and fixes Jan 31, 2022
@GuillaumeFavelier GuillaumeFavelier marked this pull request as draft January 31, 2022 09:42
This was referenced Jan 31, 2022
@GuillaumeFavelier
Copy link
Contributor Author

Closing in favor of #10283 and #10276

@GuillaumeFavelier GuillaumeFavelier deleted the enh/coreg branch February 1, 2022 16:48
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