ENH: Implement deactivate() for fast marching

[ThomasKierski]

This PR adds an implementation of deactivate() to the Fast Marching segment editor effect to address the issue where the "preview" label map was not cleaned up when the user switches to another segment editor effect or module. This presented some confusing possibilities, such as the one shown in the video below where a user could edit the preview label map.

This PR

fm_pr.mp4

Current main branch

fm_issue_720p.mp4

[jamesobutler]

cc: @lassoan @Sunderlandkyl for review thoughts here. This is something our group observed and thought should change based on our development to expose this effect into our Slicer custom app.

[lassoan]

This change looks good to me. I'm just wondering how this behaves if the user saves the scene while the preview is active. When the user restsrts Slicer and opens the saved scene, data that was temporarily saved in variables are lost, which may lead to data loss or inconsistent state.

[jamesobutler]

Do you know offhand what happens when a user saves the scene while Fill Between Slices preview is active? Is that preview restored? I believe Fast Marching is unique in that its "preview" is directly modifying the actual segment rather than a temp segment object. I would assume saving the scene will save the segmentation node at its current preview or if the effect is deactivated prior to save then yes it would lose the preview's changes.

[lassoan]

Auto-complete effects are working well (we had to be careful saving everything into the scene to make sure scene saving and loading dors not mess up things). Threshold preview is quite robust, too.

[ThomasKierski]

@jamesobutler is correct. Since the Fast Marching preview label map is stored in the active segment, the preview is written to disk on save. This PR doesn't do anything to modify that behavior.

[lassoan]

Yes, I agree, this change does not affect the incorrect behavior (original segment is lost) when the scene is saved and reloaded. The fix for that would probably require to create a temporary segmentation to preview the result, so the current fix might not be necessary or it may work somewhat differently.

@ThomasKierski
If you have the time to address the scene saving issue then I would wait for those changes.
If you don't plan to fix that issue for now then let me know and then this PR can be just merged as is. It is an improvement, as it makes the effect's behavior a bit easier to understand for the user (the hidden saved state of the edited segment can be confusing for the user - the shorter this saved state is maintained, the better).

[ThomasKierski]

@lassoan I took a first pass at solving the scene saving issues you mentioned in your last comment and issued a draft PR #64 where we can discuss how to proceed on that front

[jamesobutler]

@lassoan Considering #64 has been posted to improve the effect's overall usage of preview functionality, I suggest we proceed with integration with this initial step improvement

[lassoan]

OK, this is an improvement for sure and we'll work on the final solution (that is more robust and probably allows faster updates, even in 3D), so let's merge this.

I'm just curious, why was it necessary to make a copy of self.originalSelectedSegmentLabelmap before using it to set the segment?

[ThomasKierski]

@lassoan it may have been something peculiar to my environment, but I had an odd edge case pop up where an observer on the active segmentation node / segmentation was causing recursion due to self.originalSelectedSegmentLabelmap never getting set to None. I can't remember the exact steps I took that caused this behavior, but I kept that change in out of caution

[lassoan]

Tabs for the explanation. Such information needs to be added a comments to the code, because without that the next time someone touches that part of the code will remove the unexplained, seemingly unnecessary steps. Since this code will not stay there for long (as you are already working on a new solution) it is OK to leave it there as is.