-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(segmentation-tools): toggle passive and active tool modes based on segment actions #4633
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for ohif-platform-docs canceled.
|
✅ Deploy Preview for ohif-dev canceled.
|
9f767bf
to
b794564
Compare
@sedghi Could you please review this PR and let me know if it's possible to include these changes in 3.9? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this PR last week and I'm pretty sure it's not the right approach. I need to think about how to fix this issue, but it's definitely not this way. One problem is that SegmentationTools are hardcoded in that file, which doesn't make sense.
The right approach is to let the evaluator handle the disabled state, and have the segmentation service simply publish events.
64eb986
to
c763885
Compare
@sedghi We have implemented a new approach by adding listeners to the Segmentation Tools, similar to what we did for the ReferenceLines Tool. With these changes, modifications to the Segmentation Service have been removed, and it now simply publishes events. Additionally, we have removed the hardcoded Segmentation Tools. However, we encountered a performance issue due to multiple event subscriptions. To address this, we have implemented an unsubscribe mechanism before subscribing to events https://github.com/Devu-trenser/Viewers/blob/fix-brush-tool-is-active-after-deleting-segments-from-segmentation/extensions/cornerstone/src/init.tsx#L289, ensuring optimal performance. Could you please review the latest changes? |
I don't think we need this pull request anymore; I've already added the two fixes here: |
What do you think |
we don't need this anymore right? |
@sedghi The issue has been resolved. When deleting all segments, the segmentation tools are now disabled, and we are able to create new segments. However, we can still see a black cursor in the viewport as seen in the below attached screenshot. Is this black cursor intended as a feature to indicate that brushing in the viewport is not possible? |
Context
Changes & Results
Changes to
1. Adding new segments.
2. Activating a segmentation that contains at least one segment.
3. Switching between segmentations that contain segments.
Results:
OSS4633.mp4
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment