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

Add possibility to move nodes #4743

Merged
merged 24 commits into from
Aug 11, 2020
Merged

Add possibility to move nodes #4743

merged 24 commits into from
Aug 11, 2020

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Jul 30, 2020

Add the possibility to move nodes by adding an action/reducer and by making the WebGL buffer updater (skeleton.js) update node and edge positions.
The active node can be moved using "Ctrl + Arrow Keys" or by using "Ctrl + LeftMouseDrag".

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a tracing and move nodes using the mentioned keyboard and mouse controls.

    • Does it work in each viewport?
    • Is the node position persisted after a reload?
    • Does it work for branchpoints?
  • Testing the merger mode:

    • open a skeleton tracing with a segmentation layer
    • enable the merger mode and create two nodes that merge to segments
    • Now move one of the nodes around. The mapping should update as expected.

Issues:

  • This will allow users to easily correct ground truth skeleton data. Previously one needed to remove a node, add a new node and reconnect the split trees.

@daniel-wer
Copy link
Member Author

daniel-wer commented Jul 30, 2020

  • Use Ctrl + MouseDrag instead of Shift + MouseDrag, because Shift + Click selects a node and the potential to mistakenly move nodes feels rather high
  • Undo buffer should not be completely filled with updateNode actions, take latest instead

Copy link
Member Author

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

@MichaelBuessemeyer Just a small note regarding your last two commits. Also, please add the new shortcuts to the shortcut overview page :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome stuff!! Works very well 👍

Moving a node can be a tad laggy, but I think this is alright for this feature. However, I noticed two things which should probably be improved before merging:

  • moving does not work in a hybrid tracing (have a look at getPlaneMouseControls in plane_controller.js)
  • the move-shortcuts don't repeat when being pressed down. If I want to move a node by 20 pixels, I have to hit the shortcut 20 times?

@MichaelBuessemeyer
Copy link
Contributor

Also, please add the new shortcuts to the shortcut overview page :)

good catch 👍

@MichaelBuessemeyer
Copy link
Contributor

@philippotto I applied all of your feedback except one thing (see my comment). Could you please have a look at this PR again?

@normanrz
Copy link
Member

normanrz commented Aug 5, 2020

  • Btw moving nodes does not seem to work with merger mode yet.

@MichaelBuessemeyer
Copy link
Contributor

@philippotto I just adapted the merger mode to work correctly when nodes are moved. Could you please check this PR again? I added a section on how to test the merger mode to the description. The description is rather easy 😄

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome! Works very well. I left some smaller changes, but apart from these, the PR should be good to merge :)

CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/merger_mode.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/merger_mode.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/merger_mode.js Outdated Show resolved Hide resolved
@MichaelBuessemeyer MichaelBuessemeyer merged commit fc458f9 into master Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants