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

Feature/event dispatch locking changes #836

Conversation

JamesAPetts
Copy link
Member

feat(Event Dispatchers): Can now use non-annotation tools during a multi-part tool's loop.

Added a new flag, isMultiPartTool to tools, and a global state trigger you may set called isMultiPartToolActive. If this is true, only non-annotation tools may be used whilst you are in a multi-part tool's event loop.

re #833

James Petts added 2 commits February 13, 2019 15:25
…lti-part tool's loop.

Added a new flag, isMultiPartTool to tools, and a global state trigger you may set called
isMultiPartToolActive. If this is true, only non-annotation tools may be used whilst you are in a
multi-part tool's event loop.

re cornerstonejs#833
@codecov
Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #836 into master will decrease coverage by 0.02%.
The diff coverage is 4.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #836      +/-   ##
==========================================
- Coverage   10.81%   10.78%   -0.03%     
==========================================
  Files         210      211       +1     
  Lines        6776     6813      +37     
  Branches     1075     1088      +13     
==========================================
+ Hits          733      735       +2     
- Misses       5095     5118      +23     
- Partials      948      960      +12
Impacted Files Coverage Δ
src/store/index.js 40% <ø> (ø) ⬆️
.../eventDispatchers/touchEventHandlers/touchStart.js 0% <0%> (ø) ⬆️
...ntDispatchers/touchEventHandlers/multiTouchDrag.js 0% <0%> (ø) ⬆️
src/tools/FreehandSculpterMouseTool.js 0% <0%> (ø) ⬆️
...c/eventDispatchers/mouseEventHandlers/mouseDown.js 0% <0%> (ø) ⬆️
src/eventDispatchers/touchEventHandlers/tap.js 0% <0%> (ø) ⬆️
src/store/filterToolsUsableWithMultiPartTools.js 0% <0%> (ø)
...c/eventDispatchers/mouseEventHandlers/mouseMove.js 0% <0%> (ø) ⬆️
...ispatchers/mouseEventHandlers/mouseDownActivate.js 0% <0%> (ø) ⬆️
...c/eventDispatchers/mouseEventHandlers/mouseDrag.js 0% <0%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e66375...dfd6f8b. Read the comment docs.

@JamesAPetts JamesAPetts requested a review from dannyrb February 13, 2019 15:53
@JamesAPetts
Copy link
Member Author

TODO:

  • Moving an individual node of the freehand tool locks things up.
  • Should we disable scrolling during multi-tool use? Surprisingly scrolling whilst using the freehand tool here doesn't break anything, but you can't continue to draw until you scroll back to the slice. Is there any need for the stack scroll tools to be usable with a multi-tool?

@pieper
Copy link

pieper commented Feb 15, 2019

  • Should we disable scrolling during multi-tool use? Surprisingly scrolling whilst using the freehand tool here doesn't break anything, but you can't continue to draw until you scroll back to the slice. Is there any need for the stack scroll tools to be usable with a multi-tool?

For a similar tool in Slicer it was helpful to allow scrolling in the middle of drawing. It lets you better understand the 3D nature of the outline. What I did was display the polyline-so-far as a dashed line to indicate it's not active. The line goes back to solid when you are back on the slice where you started.

@JamesAPetts
Copy link
Member Author

@pieper Hmm, that makes sense. With the current way cornerstoneTools works, rendering the WIP ROI in a different slice might be challenging. A tool's data is drawn by checking if there is annotation data for that imageId, so you'd lose the reference when you scroll.

I guess we could save a reference to the WIP toolData on the tool itself when start drawing, then in the renderToolData method we could check if the imageId isn't the same as the WIP annotation. You could then overlay this onto a different image.

Then you delete the reference on the tool when the annotation is complete.
It feels a bit hacky, but a way to do something like this in the current framework.

@pieper
Copy link

pieper commented Feb 15, 2019

The way the current Slicer Segment Editor operates is to let you scroll while keeping your WIP drawing and then applying it to the slice where you complete it. This works pretty well too and might be easier and more logical to implement. Basically you would just move the tool state to the new image.

@JamesAPetts
Copy link
Member Author

Following this discussion I won't block scroll from being used. Any expansion to the freehand tool UI to improve the UX whilst scrolling would be introduced in a subsequent PR.

@JamesAPetts JamesAPetts merged commit 53bf96f into cornerstonejs:master Feb 15, 2019
@dannyrb
Copy link
Member

dannyrb commented Feb 15, 2019

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants