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

Fix/crosshairs annotations #303

Merged
merged 4 commits into from
Nov 21, 2022
Merged

Fix/crosshairs annotations #303

merged 4 commits into from
Nov 21, 2022

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Nov 19, 2022

  • Make Crosshairs to remove their annotations upon disabled
  • Make annotation rendering engine to reset its rendering flags when a viewport is removed

@netlify
Copy link

netlify bot commented Nov 19, 2022

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit b7cfec5
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/637b7fd960a76e0008c688ed
😎 Deploy Preview https://deploy-preview-303--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sedghi sedghi force-pushed the fix/crosshairs-annotations branch from 84db1e1 to 26b125d Compare November 19, 2022 02:50
@@ -210,6 +210,7 @@ class RenderingEngine implements IRenderingEngine {

// 5. Remove the requested viewport from the rendering engine
this._removeViewport(viewportId);
viewport.hasBeenDisabled = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The naming here is very odd, and I think it ends up not being quite right. hasBeenDisabled doesn't mean it is currently disabled, but suggests that at some point in the past it got disabled. Is it safe to say:
viewport.isDisabled = true
or
viewport.disabled = true
Otherwise, what about making it more prescriptive, something like disableOverlayDrawing, so that it is specifying that no overlays are to be drawn this viewport, and that would include things like the crosshairs, but may also include things like segmentation overlays etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense

@sedghi sedghi merged commit aeb205e into main Nov 21, 2022
Copy link
Collaborator

@wayfarer3130 wayfarer3130 left a comment

Choose a reason for hiding this comment

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

Looks good now.

NeilMacPhee pushed a commit to NeilMacPhee/cornerstone3D-beta that referenced this pull request Nov 22, 2022
* feat: add hasBeenDestroyed to viewport and fix bugs

* fix: reset annotation render on remove element

* new api

* apply review comments
wayfarer3130 pushed a commit that referenced this pull request Jan 20, 2023
…303)

* feat(anonymizer) - export Array tagNamesToEmpty and add parameter in function cleanTags -> manage the tags to anonymize/modify in dicom

* fix comma

* return a copy of the list tagsNameToEmpty and then pass in the lists as optional arguments to cleanTags

Co-authored-by: CASTELNEAU Julien <[email protected]>
@sedghi sedghi deleted the fix/crosshairs-annotations branch January 22, 2025 16:02
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.

2 participants