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

New Toolbar #4875

Merged
merged 40 commits into from Oct 22, 2020
Merged

New Toolbar #4875

merged 40 commits into from Oct 22, 2020

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Oct 15, 2020

image

This PR:

  • changes the tool text labels to tool icons
  • adds dedicated tool modes for the fill-tool and the cell picker
  • adds overwrite-mode buttons (to switch between overwrite-all vs overwrite-only-active)
  • adds dynamic updating of the toolbar when pressing modifiers
    • e.g., when pressing shift, the active tool is changed to the cell-picker tool
    • e.g., when being in the move (skeleton) tool, pressing a modifier will open a tooltip which explains the current behavior
  • ctrl+right click won't center the active node anymore (ctrl+right click is used to create a new node without activating it; in my opinion this should not re-center the current node which is why I changed/fixed this)
  • middle-click-drag moves the plane (similar to alt+mousemove) (fixes middle-click plus drag for move during brush mode? #4823)
  • fixed antd border styling when a switch-button was hovered or activated (some borders were missing before)
  • during testing how often the new toolbar re-renders, I noticed that the SaveButton rerendered on every position change. Therefore, I improved the SaveButton component so that it doesn't re-render that often:
    • the current timestamp and the lastSavedTimestamp is computed/accessed ad hoc instead of writing it into the state
    • the progress percentage (which is null most of the time) is passed as a prop instead of passing the progressInfo which changes on every movement
      • maybe we should consider to not persist each movement as an updateTracing action. the save_saga will compact this later, but before that the store is updated with a very high frequency. it could be more performant to throttle this as early as possible. just some food for thought...

URL of deployed dev instance (used for testing):

Steps to test:

  • test all tools and play around with the modifiers to see whether everything behaves as expected

Issues:


…sFraction and by accessing the oldest unsaved timestamp ad-hoc)
@philippotto philippotto self-assigned this Oct 19, 2020
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.

Wow, this PR is so beautiful, I'd like to print it out and put in on a poster 🤩
It's great how you decoupled the overwrite behavior from the active tool and made the code much easier in many places. Also the structured use of hooks in the volume_actions_view makes it very easy to reason about the code in what is actually a fairly complex component.
Everything is so much more discoverable now!

I only noted a couple of clarifying questions and suggestions. During testing everything worked exactly as described, except for these two small issues:

  • Small UI glitch (blue vertical bar) when hovering over the disabled volume tool buttons when merger mode is active (see screenshot).
    hover-glitch-disabled
  • If a cell is active (blue, for example) and then a mapping is activated which changes the cell id (and color to red for example), the dot in the "Create new cell ID" button remains blue and the tooltip contains the old unmapped cell ID. When annotating, the new cell ID and red color is correctly used.

CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
@@ -22,6 +23,9 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- webknossos.org only: Accounts associated with new organizations can now be created even when a datastore is unreachable. The necessary folders are created lazily when needed. [#4846](https://github.com/scalableminds/webknossos/pull/4846)
- When downloading a volume tracing, buckets containing a single 0 byte, that were created to restore older versions, are now skipped. [#4851](https://github.com/scalableminds/webknossos/pull/4851)
- Task information CSV now contains additional column `creationInfo`, containing the original NML filename for tasks based on existing NMLs. [#4866](https://github.com/scalableminds/webknossos/pull/4866)
- Improved the toolbar to make the different webKnossos tools easier to use. For example, the fill-tool and the cell-picker have a dedicated button in volume annotations now. [#4875](https://github.com/scalableminds/webknossos/pull/4875)
- Dragging with the middle mouse button changes the active position (even when another tool is selected). [#4875](https://github.com/scalableminds/webknossos/pull/4875)
- The default overwrite-behavior in volume annotating changed so that erasing with the brush- or trace-tool always erases all voxels (regardless of their segment id). Before that, only the current segment id was overwritten by default. As before, this behavior can be toggled by pressing CTRL. Alternatively, one can now also switch the mode in the toolbar. [#4875](https://github.com/scalableminds/webknossos/pull/4875)
Copy link
Member

Choose a reason for hiding this comment

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

As this is a breaking change (line 28), we should make sure to communicate it properly during release (and highlight volume undo again, maybe :))

@philippotto
Copy link
Member Author

Thank you for your kind feedback :) I think, I covered the crucial aspects now.

Regarding the "breaking change", I'm not 100% sure whether it's necessary to communicate this. I'm afraid it will be difficult to explain/grasp and the potential risk impact is not very high (as there is the undo-feature). We could communicate it to the lab managers, but I'd assume that they won't propagate the new information to all users, as it's not super important in the end (?). What do you think?

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.

Very nice, everything worked well during my renewed testing round 👍 LGTM

Regarding the "breaking change", I'm not 100% sure whether it's necessary to communicate this. I'm afraid it will be difficult to explain/grasp and the potential risk impact is not very high (as there is the undo-feature). We could communicate it to the lab managers, but I'd assume that they won't propagate the new information to all users, as it's not super important in the end (?). What do you think?

I don't have a strong opinion on this. You're right that with the volume undo working, no real harm can be done 👍

@philippotto philippotto merged commit aa959cd into master Oct 22, 2020
@philippotto philippotto deleted the new-toolbar branch October 22, 2020 13:31
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.

middle-click plus drag for move during brush mode? Rework skeleton and volume toolbar
2 participants