Undo feature for Markups module

[slicerbot]

This issue was created automatically from an original Mantis Issue. Further discussion may take place here.

The feature is implemented but not enabled in official Slicer releases. There are also these issues found:

• [ ] Points disappear (source): Try creating a pointlist with 3-4 points. Delete the node, recover, and repeat the operation a few times (undo/redo), notice that one by one the points start to disappear from the list.
• [ ] Points remain visible in viewer 1 (source): I also noticed a case where the node was removed from the scene by right click delete scene and yet the points remained visible in the 3D viewer. I can’t remember the sequence this was generated from. (source)
• [ ] Points remain visible in viewer 2 (source):
  • create a Fiducial node with 3 points; delete the node
  • Undo
  • Undo again: the third point is removed in the Markups module, but still persists in a slice view, being non-interactive. The first and second points become non-interactive also.
  • Redo - the third point reappears in the Markups module, and all three points become interactive in the slice view.

[lassoan]

Slicer has a global undo/redo mechanism, which can be enabled quite easily (see this branch: https://github.com/Slicer/Slicer/compare/main...lassoan:Slicer:attic/enable-global-undo?expand=1).

However, there is one limitation that should be resolved before it gets enabled. Currently, all nodes are copied when an undo state is saved. The correct behavior would be this: if a copy already exists in the undo stack and it has the same MTime as the current node then the node should not be copied but instead its the pointer should between the two new and previous undo states.

Additional smaller issues:

• View updates should not trigger SaveStateForUndo, as it could slow down view interactions. We can figure out other methods (scene views, etc.) for the rare cases when views are so valuable that we want to be able to restore a previous view.
• Slicer and 3D views are not always updating after undo/redo (need to interact with the view to trigger a forced re-rendering).

[fusentasticus]

Please do put priority = high ! Thanks.

[lassoan]

@fusentasticus If a feature or fix is critical for you then we can help finding developers (Kitware or other Slicer commercial partners) who can be contracted to work on it.

[fusentasticus]

Poor man's solution is "Right Mouse Click → Clone" every minute or so… This could be scripted in a few lines of asynchronous Python, too, I guess. Ugly, but would go a long way.

I hope somebody will take up the larger challenge as @lassoan recommends.

[lassoan]

Why do you need to undo your markups moves so frequently?

[fusentasticus]

Good question! Answer: because even after much practice, it seems that half my mouse actions still end up doing the wrong thing!!

Tumble/orbit in 3D is the main culprit. That's a consequence of the legacy design of 3D Slicer it appears. I just found the discussion in the forum .

The overloading of mouse use has been mitigated by toggle modes. Unfortunately, those fixes by themselves are also at odds with good UI practices (caps-lock key=bad, shift=good, etc) and create frustration because of markers that move when they weren't meant too.

To get a marker back into submission after user error often requires reestablishing the camera, a 6-DOF operation, which can be tricky in complicated structures. I've also managed to loose all my points at once (Interaction Handles).

The 3D guided placement of markups is otherwise a joy to use. I can tell how much care has been put into this code as I've seen it improve!

[lassoan]

Would it help if we made locking more accessible? For example, we could easily add lock/unlock (all or just the clicked control point) option in the right-click menu.

[fusentasticus]

My general experience is that all that locking acrobatics in 3D Slicer is already a legacy issue made necessary by unfortunate UI design choices. For example, each volume rendering produces a new ROI whose visibility, and even lock status, is unaffected by why you're actually doing later, such as looking at other volumes or doing markups...

So as a user I have to manage my editing intent by flipping locks all the time, in separate modules (not even by a simple right click menu) instead of being assisted by my choice of active module. If I don't flip these locks correctly then something bad will happen or I have to go to another module to unlock something I'd like to work on but that I had to lock earlier.

Between the 'undo' and the 'tumble problem' (as per the previous reference), I'd probably vote for addressing 'tumble' first, in all honesty. Along the lines of CAD and interactive mesh programs as mentioned in that thread. And I'd ditch support for anything but mice with three buttons -- they come wireless for like $10 these days, but I'm getting off track.

So back to your question: no, locks are bad, and I'd like to see that they weren't needed at all in 3D Slicer even if they were more accessible as you suggest.

[lassoan]

For example, each volume rendering produces a new ROI whose visibility, and even lock status, is unaffected by why you're actually doing later, such as looking at other volumes or doing markups...

I don't understand this sentence.

So as a user I have to manage my editing intent by flipping locks all the time, in separate modules (not even by a simple right click menu) instead of being assisted by my choice of active module. If I don't flip these locks correctly then something bad will happen or I have to go to another module to unlock something I'd like to work on but that I had to lock earlier.

You have access to all common setting at a single place - in Data module. Visibility settings (2D/3D, visibility, transparency), transforms, etc. We can very easily add any other function that you miss. We are retiring annotations (ruler, ROI) as we reimplement them in markups, and you can right-click on markups to access functions directly in the viewer (and if you miss any functions from the right-click menu, it is very easy to add; you can even add any custom functions by adding a few lines of Python code to your .slicerrc.py file).

And I'd ditch support for anything but mice with three buttons

This is not an option. For example, I only work with a touchpad when I use a laptop. In many situations (e.g., when not working at a proper desk), then there is not even space for a mouse.

no, locks are bad, and I'd like to see that they weren't needed at all

Slicer is used for many things. Locks are very useful and for certain use cases they are essential.

We'll make GUI -> widget event mapping user configurable (what keyboard/mouse action does what), which should help in separating view rotation from markups manipulation. Making it configurable from Python script should be quite easy already. Would this help you (until undo/redo can be re-enabled for markups)?

[lassoan]

Actually, you can already change/add new keyboard mapping, for example to make the view rotate by right-click-and-drag, you can run this code in the Python console (and add it to .slicerrc.py to make it persistent):

threeDViewWidget = slicer.app.layoutManager().threeDWidget(0)
cameraDisplayableManager = threeDViewWidget.threeDView().displayableManagerByClassName('vtkMRMLCameraDisplayableManager')
cameraWidget = cameraDisplayableManager.GetCameraWidget()
# Remove old mapping from right-click-and-drag
cameraWidget.SetEventTranslationClickAndDrag(cameraWidget.WidgetStateIdle, vtk.vtkCommand.RightButtonPressEvent, vtk.vtkEvent.NoModifier,
    cameraWidget.WidgetStateRotate, vtk.vtkWidgetEvent.NoEvent, vtk.vtkWidgetEvent.NoEvent)
# Make right-click-and-drag rotate the view
cameraWidget.SetEventTranslationClickAndDrag(cameraWidget.WidgetStateIdle, vtk.vtkCommand.RightButtonPressEvent, vtk.vtkEvent.NoModifier,
    cameraWidget.WidgetStateRotate, cameraWidget.WidgetEventRotateStart, cameraWidget.WidgetEventRotateEnd)

[fusentasticus]

For example, each volume rendering produces a new ROI whose visibility, and even lock status, is unaffected by why you're actually doing later, such as looking at other volumes or doing markups...

I don't understand this sentence. I was just trying to say that as you're working markups, volume renderings, mpr's, .... the slice and 3d views of the scene are strewn with things that can change if you click the wrong place. And everything is always exposed, not modulated by the choice of what module is currently active. That's not necessarily bad unless combined with both (a) no undo and (b) merging two unrelated highly used functions unto one mouse button. It was great that (b) was addressed for slice views when window/level was uncoupled from selecting/moving-stuff, and it'll need to be done for the 3D viewer as well I think.

You have access to all common setting at a single place - in Data module. Visibility settings (2D/3D, visibility, transparency), transforms, etc. We can very easily add any other function that you miss. We are retiring annotations (ruler, ROI) as we reimplement them in markups, and you can right-click on markups to access functions directly in the viewer (and if you miss any functions from the right-click menu, it is very easy to add; you can even add any custom functions by adding a few lines of Python code to your .slicerrc.py file).

That's great news. Yes, I'm rooting for the liberation of the right button in all four windows so that I can easily lock/unlock or toggle other attributes of something without changing active module.

We'll make GUI -> widget event mapping user configurable (what keyboard/mouse action does what), which should help in separating view rotation from markups manipulation. Making it configurable from Python script should be quite easy already. Would this help you (until undo/redo can be re-enabled for markups)?

It'll be awesome! For example, many CAD programs manage to both have 3D rotate (tumble) and right menu on the right mouse button. "Paint 3D" (Windows 10) is one example. These choices are controversial, sure, but making it configurable according to one's CAD habits would be fantastically useful.

[fusentasticus]

nd-drag, you can run this code in the Python console (and add it to .slicerrc.py to make it persistent):

This saved my day! I had overlooked Customize_keyboard_shortcuts.

That little snippet makes zoom-translate-orbit-drag fluent (that's three camera operations and then select-and-drag for the point).

Placing new points in this scheme would require that right-clicking doesn't abort place-new-point-mode, but that's a minor thing.

Thanks for drilling down to where the shoe was pinching the most.

[lassoan]

The topic has come up recently:

• https://discourse.slicer.org/t/new-markups-functionality-retains-unset-control-point-positions/20140/11
• https://discourse.slicer.org/t/rfe-add-ability-to-safely-disengage-unintentionally-dragged-handle/20484

Undo/redo can be enabled for markups using this code snippet:

# Enable undo for the scene

slicer.mrmlScene.SetUndoOn()

# Enable undo for markups fiducial nodes

defaultMarkupsNode = slicer.mrmlScene.GetDefaultNodeByClass("vtkMRMLMarkupsFiducialNode")
if not defaultMarkupsNode:
    defaultMarkupsNode = slicer.vtkMRMLMarkupsFiducialNode()
    slicer.mrmlScene.AddDefaultNode(defaultMarkupsNode)

defaultMarkupsNode.UndoEnabledOn()

# Add standard keyboard shortcuts for scene undo/redo

redoKeyBindings = qt.QKeySequence.keyBindings(qt.QKeySequence.Redo)
for redoBinding in redoKeyBindings:
    redoShortcut = qt.QShortcut(slicer.util.mainWindow())
    redoShortcut.setKey(redoBinding)
    redoShortcut.connect("activated()", slicer.mrmlScene.Redo)

undoKeyBindings = qt.QKeySequence.keyBindings(qt.QKeySequence.Undo)
for undoBinding in undoKeyBindings:
    undoShortcut = qt.QShortcut(slicer.util.mainWindow())
    undoShortcut.setKey(undoBinding)
    undoShortcut.connect("activated()", slicer.mrmlScene.Undo)

It is not enabled by default because historically it was attempted to be enabled for all nodes by default, which was too complicated. Since undo was disabled, developers did not pay attention to properly calling markupsNode->SaveStateForUndo() before all user actions, all those need to be added (without that, an undo reverts multiple user interactions), may be duplicated at a few places (requiring hitting undo/redo shortcuts multiple times to get a visible change), and of course some display update issues or other small bugs may be uncovered. Without these fixes, undo/redo will not feel very robust, but these should be all fairly easy to address.