Skip to content

Keeping measurements inside displayedArea when options' flags are set #1278

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

brunoalvesdefaria
Copy link
Collaborator

@brunoalvesdefaria brunoalvesdefaria commented Aug 3, 2020

  • What kind of change does this PR introduce?
    Fixes Keep annotations inside displayedArea #1277

  • What is the current behavior?
    Keep annotations inside displayedArea #1277

  • What is the new behavior?
    New deleteIfHandleOutsideDisplayedArea and preventHandleOutsideDisplayedArea flags were created in order to prevent handles outside image's displayed area.

  • Does this PR introduce a breaking change?
    No, it will only add a new feature to control measurements outside the displayed area of the image.

  • Other information:
    This PR will fix the problem mentioned in this issue: "Crop" image #745

@brunoalvesdefaria brunoalvesdefaria added the Change: Feature 💻 Change that expands the API label Aug 3, 2020
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #1278 (e96d8ee) into master (95c3b7c) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1278      +/-   ##
==========================================
- Coverage   20.16%   20.16%   -0.01%     
==========================================
  Files         287      291       +4     
  Lines       10102    10103       +1     
  Branches     2060     2041      -19     
==========================================
  Hits         2037     2037              
- Misses       6852     6860       +8     
+ Partials     1213     1206       -7     
Impacted Files Coverage Δ
...ispatchers/touchEventHandlers/addNewMeasurement.js 3.70% <0.00%> (+0.57%) ⬆️
src/lib.js 100.00% <ø> (ø)
src/manipulators/anyHandlesOutsideDisplayedArea.js 0.00% <0.00%> (ø)
src/manipulators/clipHandle.js 0.00% <0.00%> (ø)
src/manipulators/deleteIfHandleOutsideLimits.js 0.00% <0.00%> (ø)
src/manipulators/getHandleMovingOptions.js 0.00% <0.00%> (ø)
src/manipulators/moveAllHandles.js 6.06% <0.00%> (+0.72%) ⬆️
src/manipulators/moveHandle.js 5.88% <0.00%> (+0.50%) ⬆️
src/manipulators/moveNewHandle.js 5.12% <0.00%> (+0.42%) ⬆️
src/store/index.js 50.00% <ø> (ø)
... and 7 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 95c3b7c...e96d8ee. Read the comment docs.

Copy link
Contributor

@ladeirarodolfo ladeirarodolfo left a comment

Choose a reason for hiding this comment

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

During some manual tests found the following details:

  1. Displayed area is image area or actually viewport? Saying that because, having flag deleteIfHandleOutsideDisplayedArea: true, I`m able to set a measurement tool if its placed inner image area but even out of viewport (what it is really displayed). Is this expected? To easily reproduce it: use pan to move image, then add a measurement starting from a visible region up to a region out of viewport but inner image area.

  2. Bugs I believe: FreehandRoi tool.
    2.1 For deleteIfHandleOutsideDisplayedArea: true

  • Enable freehandRoi tool
  • Start drawing from invalid area(outside image)
  • Drawing color must change to red
  • At this point you can not even cancel drawing (maybe this is another potential and not related bug)
  • Force cancel drawing by changing to another selected to
    Expected: FreehandRoi draw must be delete
    Actual: FreehandRoi is persisted and also with a different drawing line

2.2 For preventHandleOutsideDisplayedArea: true

  • Enable freehandRoi tool
  • Start drawing from a valid area(inner image boundaries)
  • Draw over valid area
  • Drawing color must change to red
    Expected: FreehandRoi drawing must be blocked to go outside
    Actual: Drawing is kind of blocked, but limits looks like are different from other tools. Also after finishing a handle outside area drawing might change on an unexpectedly way

},
options
);
options = getHandleMovingOptions(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to create a new variable instead of options. I know this come from old implementation, but doing this we avoid potential js reference issues.

@igoroctaviano
Copy link
Contributor

@sedghi can you review + test this pull request please? for more details, check out the ticket in the issue description.

@sedghi
Copy link
Member

sedghi commented Jun 9, 2021

I'm not able to replicate the behaviour that is intended here.

  • run demo with cornerstoneTools.store.state.preventHandleOutsideDisplayedArea = true

  • do

    const viewport = cornerstone.getViewport($0);
    Object.assign(viewport.displayedArea.tlhc, { x: 128, y: 128 });
    cornerstone.setViewport($0, viewport);
    
    
  • Image is transformed

  • use length

  • it does not allow drawing inside, it always snaps

chrome-capture (10)

@igoroctaviano
Copy link
Contributor

I'm not able to replicate the behaviour that is intended here.

  • run demo with cornerstoneTools.store.state.preventHandleOutsideDisplayedArea = true
  • do
    const viewport = cornerstone.getViewport($0);
    Object.assign(viewport.displayedArea.tlhc, { x: 128, y: 128 });
    cornerstone.setViewport($0, viewport);
    
  • Image is transformed
  • use length
  • it does not allow drawing inside, it always snaps

chrome-capture (10)

This should work since we used it in another project. @brunoalvesdefaria the usage is correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep annotations inside displayedArea
4 participants