When completing the following steps, the undo/redo buttons are enabled despite no visible changes being made to the segmentation in the scene.
Create a new segmentation
Select the erase tool
Erase in the scene (undo is enabled)
Click the undo button (redo is enabled)
Although this makes sense, because editor effects are being applied, it is a little jarring for the user because the segment labelmap is empty the whole time. I feel like the expected behavior would be for the undo/redo button to remain disabled if there is no visible change to the segmentation. Is there a current workaround for this?
Operating system: Windows 11
Slicer version: 5.8.1
Some undo-able changes may not be easily noticeable, but as far as I can see, everything works as expected.
I could not follow the steps you described: I could not select the “Erase” effect without loading an image and creating a segment; but then by the time I got to selecting the “Erase” tool, the undo button was already enabled. So, I performed these steps instead:
Start Slicer
Go to Sample Data module
Load MRHead sample data
Go to Segment Editor module
Add Segment => Undo button has become enabled (clicking it removes the segment) - good
Select “Erase” effect
Click-and-drag in a slice view => Undo button is still enabled (clicking it reverts the segment’s state change from “Not started” to “In progress”; clicking again removes the segment) - good
Everything worked as expected.
I’ve also tried loading a scene that has a segmentation, selected the erase tool, but when I erased an empty region then no undo state was added (undo did not get enabled). So, again, everything worked well.
Add Segment (undo becomes enabled related to the adding of the segment)
Select “Erase” effect
Single left-click in 4 places on the volume. First click changes the segment state from “Not started” to “In progress” although the labelmap has not changed and is still empty.
Click undo multiple times and observe that you have to click undo “4” times before observing the segment going back to the “Not started” state and then clicking undo a 5th reflecting the removal of the segment.
So the clicking of the effect like Erase at a time when the binary labelmap is seemingly not changing before or after the event is resulting in a recorded node state in the undo/redo chain.
I see, thank you for the clarification. Clicking undo button reverts the last user action - the last editing operation that the user initiated, regardless if the action was effective (resulted in change) or not.
Your proposal is to make clicking undo revert the last effective user action. While this could seem more intuitive, it would lead to undeterministic behavior, as the user often has no way of knowing if an action was effective or not.
Example: I apply smoothing on the segmentation. Seemingly nothing changes. I want to redo the segmentation with a stronger smoothing factor. Should I click undo before changing the smoothing factor and applying smoothing again?
If clicking undo revert the last user user action (current): Answer is easy, yes, click undo. It takes me back to the state before the last smoothing operation, regardless if my last smoothing attempt actually changed something or not.
If clicking undo reverts the last effective user action (proposed): It is impossible to tell what to do. I cannot determine if the last operation actually changed something in the segmentation somewhere. I may be able to see changes in the current slice, but how would I know if there has been some subtle change somewhere else?
So, overall, I think keeping the current behavior is better, as it is more deterministic.
Indeed an effect can cause a change elsewhere that is not visible to the user which could still be confusing. Though it would seem that the number of these instances could be reduced if say the binary labelmap is not changed by a user action, then it shouldn’t be added into the undo/redo stack as a modified event. This could reduce unnecessary memory usage if you have a large segmentation and you do an action that does not result in the binary labelmap changing at all while still utilizing the undo stack.
Would such an action be computationally expensive to check if the labelmap was changed during the last user action when deciding whether to add a node state to the undo history?
Segmentation history only saves a new copy of the data object if it is modified. If you save new undo states without segment changes then the memory and computation cost is near zero.
The check is somewhat expensive, but not prohibitive (we do it anyway when we have segments in multiple layers and overwriting of other segments is enabled). The main issue is user experience. How would you tell users that their action did not end up changing anything? Why would you complicate their life by telling them this (most of the time) irrelevant information? Who would explain them that they need this information so that they know what happens when they click undo?
We can find better ways to make the undo/redo behavior more intuitive while keeping it deterministic. For example, in “saveStateForUndo” we could store the name of the editor effect (and the effect could provide an optional extra description). We could use this to make the undo/redo button tooltip more informative (e.g., instead of “Undo last editing operation” the tooltip would be something like “Undo Paint”, “Undo Erase”, “Undo Scissors - Erase inside”). We could also add a dropdown menu to the undo/redo buttons for bulk undo/redo (although by default we save just 10 states, which are not that hard to navigate):
I attempted to change the terminology or segment color to new states and also to the same selection to observe the undo/redo states and I observed:
Slicer 5.8.1:
Add Segment
Double click color column in table to bring up terminology dialog and change “Tissue” selection to “Artery”.
Double click color column in table to bring up terminology dialog and change “Artery” selection to “Body fat”.
Double click color column in table to bring up terminology dialog and select “Body Fat” again.
Press “Undo” and observe that it goes to immediately removing the segment where it didn’t appear to remember anything about the terminology selections in the undo stack.
Is there a reason that terminology selection is not an undo state of the segment? And that re-selecting the same terminology (producing a no change event) didn’t also produce an undo state if no change binary labelmap events create an undo event? This is to better understand what modifications to the segment are saved and if the same event selection is made for a given property (even though no change), does it count as an undo state.
It appears that double-clicking the name column and choosing a terminology saves an undo state for the segment name, but doesn’t actually undo to go back to the old terminology.
Good catch. Terminology selection should store an undo state for consistent behavior. Implementation might be a little tricky, because the segment list should emit the segmentAboutToBeModified just after the terminology popup is closed with “Select”, but before the terminology/color is updated in the segmentation, and if multiple properties are changed at once (segment name, color, terminology) then it would be nice to add only a single undo state for that - but it should be all doable. Please submit an issue for this, as I don’t know when someone will have time to implement it.
