Skip to content

Conversation

@igoroctaviano
Copy link
Contributor

@igoroctaviano igoroctaviano commented Sep 3, 2025

  • Local: http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.1706.8374.643249677828306008300337414785
  • Preview (from PR): https://deploy-preview-5381--ohif-dev.netlify.app

Context

Changes & Results

Load derived display sets (SEGs / RTSTRUCTs / SRs) for the active viewport

  • Adds Cornerstone commands to load derived content for the active viewport:
    • loadSegmentationsForActiveViewport
    • loadSRsForActiveViewport
  • How it works (high level):
    • Finds derived/overlay display sets (e.g. SEG, RTSTRUCT) that reference the currently displayed data.
    • Loads the derived display sets (using auth headers when available).
    • Hydrates/attaches the derived content into the active viewport:
      • segmentations → add segmentation representation to the active viewport
      • SRs → run SR hydration command for the derived SR display set

Hanging Protocol: viewport data change callback

  • New HP callback: adds callbacks.onViewportDataChanged?: Command[]
  • Behavior: runs whenever cornerstoneViewportService.EVENTS.VIEWPORT_DATA_CHANGED fires, passing:
    • viewportId
    • viewportData
    • protocol
    • stageIndex
  • Why: enables “react to viewport series/display set changes” workflows (e.g. auto-load overlays whenever the viewport content changes)

HP listener lifecycle + initialization fix

  • Adds attach/detach listener commands to prevent duplicate subscriptions and repeated callback firing across protocol/stage/layout changes.
  • Fixes the onViewportDataInitialized callback execution so the configured command list runs correctly and the subscription is cleaned up.

Docs

  • Documents onViewportDataChanged in:
    • platform/docs/docs/platform/extensions/modules/hpModule.md
    • platform/docs/versioned_docs/version-3.11/platform/extensions/modules/hpModule.md

Testing

  • Open:
    • http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.1706.8374.643249677828306008300337414785
    • (or) https://deploy-preview-5381--ohif-dev.netlify.app
  • Derived load commands
    • Run loadSegmentationsForActiveViewport and verify derived SEG/RTSTRUCT overlays get loaded and applied to the active viewport.
    • Run loadSRsForActiveViewport and verify derived SRs are loaded and hydrated.
  • HP callback
    • Configure a HP with callbacks.onViewportDataChanged: [...] that runs one of the new load commands.
    • Trigger a viewport data change (e.g. swap series/display set in a viewport).
    • Verify the callback runs and derived data loads for the updated viewport.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Suggested PR title

  • feat(Cornerstone): load derived display sets for active viewport

(or if you want to emphasize the HP API)

  • feat(HangingProtocol): add onViewportDataChanged callback

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS:
  • Node version:
  • Browser:

@netlify
Copy link

netlify bot commented Sep 3, 2025

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 8ff6d06
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6953d40394af8a0008d70a94
😎 Deploy Preview https://deploy-preview-5381--ohif-dev.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 project configuration.

@cypress
Copy link

cypress bot commented Sep 3, 2025

Viewers    Run #5846

Run Properties:  status check passed Passed #5846  •  git commit 8ff6d06637: Merge branch 'master' into feat/load-derived-display-sets
Project Viewers
Branch Review feat/load-derived-display-sets
Run status status check passed Passed #5846
Run duration 02m 21s
Commit git commit 8ff6d06637: Merge branch 'master' into feat/load-derived-display-sets
Committer Igor Octaviano
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 37
View all changes introduced in this branch ↗︎

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

see my comments thanks

Copy link
Collaborator

@jbocce jbocce left a comment

Choose a reason for hiding this comment

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

Minor comment

@fedorov
Copy link
Member

fedorov commented Nov 10, 2025

Can someone clarify what needs to be happen to merge this? @igoroctaviano

@igoroctaviano igoroctaviano changed the title feat: Add command to load derived display sets feat: Add command to load derived display sets [on hold] Nov 20, 2025
@igoroctaviano igoroctaviano changed the title feat: Add command to load derived display sets [on hold] feat: Add command to load derived display sets Nov 20, 2025
@igoroctaviano igoroctaviano changed the title feat: Add command to load derived display sets [on hold] feat: Add command to load derived display sets Nov 20, 2025
@igoroctaviano igoroctaviano changed the title [on hold] feat: Add command to load derived display sets feat: Add command to load derived display sets Nov 26, 2025
@igoroctaviano igoroctaviano changed the title feat: Add command to load derived display sets feat: Add commands to load segs and srs for active viewport Nov 26, 2025
@igoroctaviano igoroctaviano changed the title feat: Add commands to load segs and srs for active viewport feat: Add commands to load segs and srs for the active viewport Nov 26, 2025
@igoroctaviano igoroctaviano changed the title feat: Add commands to load segs and srs for the active viewport feat: Add commands to load SEGs and SRs for the active viewport + new hp callback for viewport changes Dec 18, 2025
@sedghi
Copy link
Member

sedghi commented Dec 19, 2025

I understand your idea, but I'm not convinced it's the best way forward anymore. We've got new plans for the viewportGrid state. It starts with this PR: #5624, which aims to fix timing issues where the state updates but the event doesn't reflect it. You should check out @wayfarer3130's comments on that PR; it shows the right approach. We should refactor the viewportGrid to separate the layout logic from the individual viewport data. This will give us the hooks needed to run callbacks assigned by the Hanging Protocol. Let me know if you wanna meet with me and Bill to go over this

@igoroctaviano
Copy link
Contributor Author

I understand your idea, but I'm not convinced it's the best way forward anymore. We've got new plans for the viewportGrid state. It starts with this PR: #5624, which aims to fix timing issues where the state updates but the event doesn't reflect it. You should check out @wayfarer3130's comments on that PR; it shows the right approach. We should refactor the viewportGrid to separate the layout logic from the individual viewport data. This will give us the hooks needed to run callbacks assigned by the Hanging Protocol. Let me know if you wanna meet with me and Bill to go over this

This is not just about the callback. I'm happy to remove the callback part of it.

@wayfarer3130
Copy link
Contributor

I understand your idea, but I'm not convinced it's the best way forward anymore. We've got new plans for the viewportGrid state. It starts with this PR: #5624, which aims to fix timing issues where the state updates but the event doesn't reflect it. You should check out @wayfarer3130's comments on that PR; it shows the right approach. We should refactor the viewportGrid to separate the layout logic from the individual viewport data. This will give us the hooks needed to run callbacks assigned by the Hanging Protocol. Let me know if you wanna meet with me and Bill to go over this

This is not just about the callback. I'm happy to remove the callback part of it.

The fact that there are load commands for various things actually fits into the new framework fairly well because it means the load action has already gotten isolated to the command module. That will make things more reliable and break fewer things going forward. I'm hoping that the hanging protocol methods will be backwards compatible as well for some length of time as we switch to a better design around the viewport grid. That way people can continue using the commands to allow loading/specifying applicability, and the commands can be enhanced in the future to add more options to allow finer control over viewport setup related to loading of data. Ideally the existing sr/seg viewports should be switched to use the commands, and then once the viewport grid is ready, we only have to fix the commands in one spot to fix loading generally.

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.

[Feature Request] Make it possible to load segmentation automatically

7 participants