Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make merger mode work for all skeleton modifications #4669

Merged
merged 10 commits into from
Jun 25, 2020

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Jun 16, 2020

Merger mode only worked for basic node/tree creation/deletion cases (see #4655 for an explanation), but didn't work for undo/redo or when merging/splitting trees.

By switching from overwriting specific redux actions to acting on the result of our tree differ, all of these cases are now handled. The resulting code is even easier as the merger mode only needs to react to two diffing actions: createNode and deleteNode. Complex redux actions like mergeTree, result in low-level diffing actions:

  • deleteNode actions for each node of the tree that was deleted as part of the merge
  • createNode actions for all of the same nodes, but in the tree that "survived" the merge
  • a deleteTree action that merger mode doesn't care about, similar an updateTree action which only modifies tree properties
    Similar for undo/redo/tree merging/splitting. This is actually an advantage of the "primitive" low-level diffing we do.

I fixed a couple of other bugs while I was at it (not all of them have issues):

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open a tracing with a segmentation and activate merger mode. Execute all kinds of tree-modifying actions - the merger mode mapping should remain consistent. (Undo, Redo, Node Deletion, Tree Split, Tree Merge, ...)
  • Activate another mapping, then activate merger mode and set nodes. No progress notifications should appear.
  • Deactivate Merger Mode. The mapping should no longer be applied.
  • Activate a mapping through the API: webknossos.apiReady(3).then(api => api.data.setMapping("segmentation", {1: 2, 3: 4})). The ID Mapping toggle in the segmentation tab should become activated.

Issues:


@hotzenklotz
Copy link
Member

@daniel-wer I just gave it a quick test and seems to work very well :-) Nice!

@MichaelBuessemeyer
Copy link
Contributor

I'll try to review this when I got time. Currently, #4663 has a higher priority

Copy link
Member

@hotzenklotz hotzenklotz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks go to me! ready to ship :shipit:

frontend/javascripts/oxalis/merger_mode.js Outdated Show resolved Hide resolved
@daniel-wer
Copy link
Member Author

I have noticed one more bug that I would like to fix. If the user sets a node outside of a segment, the node is removed automatically while displaying a toast with an explanation. However, this leads to an infinite loop when using undo. If you press undo afterwards, the just automatically removed node is created again, but immediately automatically removed again by the merger mode script. However, by removing the node a new entry is added to the undo stack, so we're in the beginning again...
The solution should be to avoid that the node is ever set at all by registering an overwrite for the CREATE_NODE action and dropping the action if the node is set outside a segment.

@bulldozer-boy bulldozer-boy bot merged commit e3a8241 into master Jun 25, 2020
@bulldozer-boy bulldozer-boy bot deleted the better-merger-mode branch June 25, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants