WIP,ENH: new coreg gui features (part 3)

[GuillaumeFavelier]

This PR is based on #10085 and adds the following:

• add support for the scaling parameters

It's part of #8833

Can only be merged after #10085

[GuillaumeFavelier]

I added another checkbox for skip_fiducials and now it's ready for review from my end @agramfort, @larsoner

[larsoner]

From our chat today, I think the plan is:

1. move the MRI scaling params to the left panel (keeping separate fit fid / fit icp buttons)
2. have save surrogate MRI subject button on the left
3. have the save -trans.fif button on the right
4. remove options for what to include/exclude when saving the surrogate subject, and instead make sure the saved subject is as much a copy of the old one as possible (copying labels, copying annot, scaling fiducials, copying bem, recomputing -bem-sol.fif, modifying and saving -src.fif if and only if the original subject had these files)

Others feel free to chime in if it seems like I missed something

[drammock]

Others feel free to chime in if it seems like I missed something

One thing was to not have a default subj name filled in when saving anatomy; the user must enter a subject name (otherwise the save button will be greyed out). Another idea was to have translation and rotation be in a 2-column arrangement; right now the input boxes are wider than they need to be and there's a lot of empty space between input boxes and their labels.

I think the rest was mostly about naming. Not sure which of these was ultimately agreed upon, but ideas were:

1. the new UI group in the left column could be called "MRI scaling"
2. "save scaled anatomy"
3. "fit fiducials with scaling" / "fit ICP with scaling" / "reset scaling"

One thing that didn't come up was where the fitting options end up after all this moving around of UI items... the options affect both the (now left-column) fit-with-anatomy-scaling and also affect the right-column fit-with-translation-and-rotation-only procedure. Moving MRI scaling to the left makes it less obvious that it will be affected by those options.

[GuillaumeFavelier]

The PR is not ready yet but here are the three main directions as mockups:

version 1 (MRI Scaling on the left):

version 2 (Translation (t) and Rotation (r)):

version 3 (MRI Scaling on the right):

[GuillaumeFavelier]

After refining a little bit:

MRI Scaling on the left:

MRI Scaling on the right:

[larsoner]

They seem pretty similar to me. If I had to choose I'd go with MRI scaling on the right because it matches more what people who use this are accustomed to. But no strong feeling, so just +0.5 on "MRI right" for me

[agramfort]

scaling params on the right works for me.

…

Message ID: ***@***.***>

[GuillaumeFavelier]

Notes from the offline meeting:

• rename "Transform" groupbox into "HEAD <> MRI Transform"
• add a prompt check name already exists "overwrite or not"
• multiple threads should block window exit
• add a checkbox to set enable/disable transparency (alpha=0.95)

[larsoner]

add a checkbox to set enable/disable transparency (alpha=0.95)

FWIW I think more people (me and @drammock) voted for a slider or double entry box than just a checkbox (@hoechenberger )

[larsoner]

Okay I found a few issues, but I think they can all be tackled in a follow-up PR, other than the issue of not actually computing the BEM solutions which I fixed in a113b48. As it is now, it allows me to scale + save a usable subject, so I'll go ahead and merge once this last commit comes back green -- thanks in advance @GuillaumeFavelier !

Remaining issues below, please add these and the points you made above to some issue to keep track @GuillaumeFavelier

Non-scaling issues (3)

Okay first I did:

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

then checked "Lock fiducials". The plot is wrong (trans is not used):

Clicking up on the X translation (which makes a tiny change) it actually updates:

Next, after changing the X and clicking "Fit ICP" repeatedly (after waiting for it to finish), I get an error in the terminal (probably because only one ICP iteration is done, and no change is made):

Traceback (most recent call last):
  File "/home/larsoner/python/mne-python/mne/gui/_coreg.py", line 948, in _fit_icp
    self._fits_icp()
  File "/home/larsoner/python/mne-python/mne/gui/_coreg.py", line 979, in _fits_icp
    f"{self._current_icp_iterations} iterations.")
AttributeError: 'CoregistrationUI' object has no attribute '_current_icp_iterations'. Did you mean: '_set_icp_n_iterations'?

After clicking "Fit ICP with scaling" or "Fit ICP", the name of the button should become "Stop ICP with scaling" and "Stop ICP" respectively, and if you click it, the button should be disabled, ICP should stop, and then the button should have its label changed back and be reenabled. This is better UX and also prevents problems with accidentally double-clicking "Fit ICP" in the current UI (if you double-click, you will just get one iteration of ICP, rather than two sets of ICP trying to run at the same time, or whatever weirdness happens currently).

Scaling issues (5)

Taking the same command above, do "lock fiducials", change to uniform scaling, and "Fit fiducials with scaling", then "Fit ICP with scaling", the head does a weird "flash" -- this is aesthetic, probably because of a remove/add/scale cycle with an update=True where it shouldn't exist (at the add step?), but we should fix it.

When in uniform scaling mode, the X should be enabled, but Y and Z disabled for user input. When X is updated, Y and Z should be updated to the same value. (In other words, there should be only one input value you can change, and when you do, it should change all three.)

Changing 3-axis mode to 1-axis mode does not actually change the head to 1-axis mode (it should use all the same value for x/y/z, probably the average of the three?); same with changing to None mode (it shouldn't scale).

The x/y/z double box increment is 100 (!), 1 is a more reasonable value.

Negative values (and ideally not even zero) should not be allowed in the x/y/z boxes. I would just set a minimum of 1., it seems reasonable enough. (If someday someone needs to scale to less than 1% we can change this.)