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

Improve skeleton proofreading and fix some shortcuts on MacOS #7678

Merged
merged 31 commits into from
Mar 27, 2024

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Mar 7, 2024

  • Enables the existing (legacy) shortcuts for deleting/creating edges for orthogonal and arbitrary mode. Now, they are not restricted to legacy mode anymore.
  • Improves the shortcut indicator in the status bar.
  • Allows creating trees and deleting nodes in arbitrary mode with the keyboard.
  • Treats the Cmd (meta) key in MacOS as a synonym to ctrl everywhere.
  • Shows the ⌥ symbol instead of "alt" and Cmd instead of Ctrl in the status bar and context menu on MacOS.

URL of deployed dev instance (used for testing):

Steps to test:

  • ortho mode:
    • create a tree with some nodes
    • delete an edge by holding ctrl+shift while clicking on the neighbor of the active node (the statusbar should indicate that action while pressing the modifiers)
    • merge the edge again by holding ctrl+alt (on some linux setups, shift also needs to be pressed to avoid that the default handler for moving a window is triggered)
    • the above should also work in legacy mode
    • test that node selection still works correctly (legacy mode should require shift for that)
  • ortho mode
    • c should create a new tree
    • delete should delete the active node
    • clicking on a node should select it (sometimes the node is then not precisely visible, I don't really know how we could circumvent this)
    • as above, try out creating and deleting edges
  • on mac:

Issues:


(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto self-assigned this Mar 7, 2024
@philippotto philippotto marked this pull request as ready for review March 8, 2024 08:54
@philippotto philippotto changed the title Improve skeleton proofreading Improve skeleton proofreading and fix some shortcuts on MacOS Mar 8, 2024
Copy link
Member

@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.

Nice improvements! Code looks great 👍

Notes from testing:

  • Ctrl + Leftclick to delete meshes doesn't work (I suggested the fix during my review, I just want to remember to retest this)
  • From the internal feature request that spawned this PR, number (1) refers to easy skeleton merging and splitting using keyboard shortcuts similar to proofreading. Would it be possible to reuse the shortcuts used in proofreading mode or would this be a breaking change?
  • The keyboard shortcut overview is no longer accurate. For example, the arbitrary mode section doesn't show that nodes can be deleted or edges can be removed/created.
  • In flight/oblique mode, the active node is not highlighted making it hard to know what's going to happen if delete is pressed or an edge is created.
  • Someone with a Mac should test the Mac-related fixes in this PR

frontend/javascripts/oxalis/controller/td_controller.tsx Outdated Show resolved Hide resolved
Comment on lines 70 to 73
// camera.far = state.userConfiguration.clippingDistanceArbitrary;
camera.near = ARBITRARY_CAM_DISTANCE - state.userConfiguration.clippingDistanceArbitrary;
} else {
camera.far = state.userConfiguration.clippingDistance;
Copy link
Member

Choose a reason for hiding this comment

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

Was this a bug before? If so, the comment can be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

I dug into this for a bit, but I'm still not really sure what is going on. The far/near clipping still doesn't seem to work as expected when selecting nodes. This is also a problem in orthogonal mode (not new). It seemed like regardless of what I did, I could always select nodes even when they weren't visible and I did stuff like setting near=far=0.
I noticed that frustumCulled is set to false for the skeleton geometry (this is relatively new), which seemed like a likely culprit. However, even setting that to true, didn't make a difference.

Unless you have another idea, I'd ignore this for now :S

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your further investigations!

It seemed like regardless of what I did, I could always select nodes even when they weren't visible and I did stuff like setting near=far=0.

One more thing that just came to my mind is that I think we enlarge the nodes in the picking shader a bit to make it easier to click them - maybe this is done in z as well 🤔

However, we don't need to investigate this further as part of this PR. Did you find an existing issue or could you create one so we remember to have another look at this in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

One more thing that just came to my mind is that I think we enlarge the nodes in the picking shader a bit to make it easier to click them - maybe this is done in z as well 🤔

Interesting idea! However, here it is written that gl_PointSize "contains size of rasterized points, in pixels". So, I would assume that it only impacts the size in 2D as rasterization is explicitly mentioned.

Did you find an existing issue or could you create one so we remember to have another look at this in the future.

I just created #7725 :)

frontend/javascripts/oxalis/view/statusbar.tsx Outdated Show resolved Hide resolved
@philippotto
Copy link
Member Author

From the internal feature request that spawned this PR, number (1) refers to easy skeleton merging and splitting using keyboard shortcuts similar to proofreading. Would it be possible to reuse the shortcuts used in proofreading mode or would this be a breaking change?

I think, this won't be possible without breaking changes. CTRL + Leftclick is used for moving nodes in classic mode and Shift + leftclick is used for selecting nodes. Regardless of classic mode, there is ctrl + leftclick to create a node without focusing it. in that case, one would click somewhere where no existing node is, but I think, it's still a collision with the split operation.
I'd defer this change to another point when we decide to improve the shortcuts. For now, this PR should empower users to do the skeleton editing without the context menu.

@philippotto
Copy link
Member Author

@valentin-pinkau could you test the two mac-related items from the test plan? :)

on mac:
test that #7652 is fixed
test that #7584 is fixed

@valentin-pinkau
Copy link
Member

@valentin-pinkau could you test the two mac-related items from the test plan? :)

Yes, both issues are fixed!

test that https://github.com/scalableminds/webknossos/issues/7652 is fixed
test that https://github.com/scalableminds/webknossos/issues/7584 is fixed

However, it would be great to show the mac user to use CMD click to split two segments.
Screenshot 2024-03-21 at 10 59 32

@philippotto
Copy link
Member Author

However, it would be great to show the mac user to use CMD click to split two segments.

Good point, done!

Copy link
Member

@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.

Mostly LGTM, but two things to consider :)

  • The current active tool seems to remain active when switching to flight or oblique mode. So when the brush is active before switching, the mouse pointer is wrong or when the proofreading tool is active before, skeletons cannot be modified. This is, although the tools are not visible and cannot be switched, there. This doesn't necessarily need to be fixed in this PR as the bug likely existed before, but users might hit it more frequently now that skeleton modifications are allowed in flight mode.
  • The casing of Shift and Ctrl in the context menu shortcuts is mixed now. Would be nice to align that.
    image

Copy link
Member

@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.

LGTM, so many bug fixes and improvements in one PR - great work 🌈

@philippotto philippotto enabled auto-merge (squash) March 27, 2024 10:16
@philippotto philippotto merged commit 7f75576 into master Mar 27, 2024
2 checks passed
@philippotto philippotto deleted the improve-skeleton-proofreading branch March 27, 2024 10:32
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.

Zoom hotkey hint could be improved on macOS Ctrl+Click proofreading shortcut does not work on MacBook
3 participants