Skip to content

Conversation

igoroctaviano
Copy link
Contributor

@igoroctaviano igoroctaviano commented Sep 18, 2025

Context

This PR addresses the issue where DICOM SR (Structured Report) measurements that cannot be mapped to existing measurement tools were not being displayed in the measurement panels. Previously, unmapped measurements were silently ignored, making them invisible to users even though the data existed in the DICOM SR.

The changes ensure that all measurements from DICOM SR files are visible to users, with clear visual indicators when measurements cannot be properly mapped to standard measurement tools.

Changes & Results

  • SCOORD bounding box
Screenshot 2025-09-18 at 17 30 29

  • SCOORD3D point
Screenshot 2025-09-24 at 15 52 00

Changes Made:

  • Enhanced MeasurementService: Added support for storing and retrieving unmapped measurements alongside regular measurements
  • Updated addSRAnnotation utility: Improved function signature and added comprehensive JSDoc documentation
  • Fixed JSDoc documentation: Updated all JSDoc comments in getSopClassHandlerModule.ts to use proper TypeScript types and consistent formatting
  • Added unmapped measurement handling: Created new methods to handle measurements that don't map to standard tools
  • Enhanced UI components: Added visual indicators for unmapped measurements with warning icons and tooltips
  • Updated measurement filters: Added new filter for invalidated measurements
  • Improved measurement hooks: Updated useMeasurements to include unmapped measurements in the display

Results:

  • Before: Unmapped DICOM SR measurements were invisible to users
  • After: All DICOM SR measurements are displayed with appropriate visual indicators for compatibility status
  • Users can now see all measurement data from DICOM SR files, even when they don't map to standard measurement tools
  • Clear visual feedback indicates which measurements may have compatibility issues

Testing

To test these changes:

  1. Load a DICOM SR file with measurements that don't map to standard tools:

    • Open OHIF viewer
    • Load a study containing DICOM SR with measurements
    • Navigate to the measurements panel
  2. Verify unmapped measurements are displayed:

    • Check that all measurements from the SR appear in the measurement table
    • Look for warning icons next to unmapped measurements
    • Hover over warning icons to see tooltip explaining compatibility issues
  3. Test measurement interactions:

    • Verify that both mapped and unmapped measurements can be toggled for visibility
    • Test measurement locking functionality works for both types
    • Confirm color changes work for all measurements
  4. Check JSDoc improvements:

    • Verify IntelliSense now provides proper type information for all functions
    • Confirm parameter types are correctly documented

Checklist

PR

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

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: macOS 10.15.7
  • Node version: 18.16.1
  • Browser: Chrome 83.0.4103.116, Firefox 77.0.1, Safari 13.1.1

Copy link

netlify bot commented Sep 18, 2025

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 813fa71
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/68dea092806756000886d96a
😎 Deploy Preview https://deploy-preview-5416--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.

Copy link

cypress bot commented Sep 18, 2025

Viewers    Run #5594

Run Properties:  status check passed Passed #5594  •  git commit 813fa71191: Merge remote-tracking branch 'origin/master' into feat/display-unmapped-measurem...
Project Viewers
Branch Review feat/display-unmapped-measurements
Run status status check passed Passed #5594
Run duration 02m 36s
Commit git commit 813fa71191: Merge remote-tracking branch 'origin/master' into feat/display-unmapped-measurem...
Committer Bill Wallace
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 ↗︎

@igoroctaviano igoroctaviano changed the title [wip] feat(MeasurementService): add rendering of unmapped measurements feat(MeasurementService): add rendering of unmapped measurements Sep 24, 2025
@fedorov
Copy link
Member

fedorov commented Sep 25, 2025

@sedghi this PR is the follow up on our desperate attempts to fix point/contour SR annotation rendering for the past 5 years. Appreciate your support to get this merged.

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.

please see my comments

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 comment please

@sedghi
Copy link
Member

sedghi commented Oct 1, 2025

@jbocce can you check/help Igor why tests are failing? then we merge

@jbocce
Copy link
Collaborator

jbocce commented Oct 1, 2025

@jbocce can you check/help Igor why tests are failing? then we merge

@igoroctaviano

It looks like only one test is failing: SRHydration-should-hydrate-SR-reports-correctly. It looks like it is failing because there are two more measurements than what was there once before as you can see by the diff screen shot.

srPostHydration-1-diff

I will let you decide whether this is expected or not. 😊

@igoroctaviano
Copy link
Contributor Author

@jbocce can you check/help Igor why tests are failing? then we merge

@igoroctaviano

It looks like only one test is failing: SRHydration-should-hydrate-SR-reports-correctly. It looks like it is failing because there are two more measurements than what was there once before as you can see by the diff screen shot.

srPostHydration-1-diff I will let you decide whether this is expected or not. 😊

@jbocce, thx for pointing it out. The screenshot should look like this? hard to read

@jbocce
Copy link
Collaborator

jbocce commented Oct 1, 2025

@jbocce, thx for pointing it out. The screenshot should look like this? hard to read

@igoroctaviano , that is the diff of the expected versus actual.

Here is the expected...

srPostHydration-1-expected

And this is the actual (i.e. with your changes in this PR)...

srPostHydration-1-actual

}

export const DataRow: React.FC<DataRowProps> = ({
const DataRowComponent: React.FC<DataRowProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Please export it - having this sort of thing exported allows for re-use.

Copy link
Member

Choose a reason for hiding this comment

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

@igoroctaviano can you address this one last comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's being exported as you can see at the bottom:

const DataRow = DataRowComponent as React.FC<DataRowProps> & {
  Status: typeof Status;
};

DataRow.Status = Status;

export default DataRow;
export { DataRow };

@sedghi sedghi merged commit 851e74d into master Oct 6, 2025
11 checks passed
@sedghi sedghi deleted the feat/display-unmapped-measurements branch October 6, 2025 12:50
@sedghi
Copy link
Member

sedghi commented Oct 6, 2025

@fedorov Thanks for your team's contribution. Just a friendly reminder that we need to add tests for these so that it does not get broken again

@fedorov
Copy link
Member

fedorov commented Oct 6, 2025

@sedghi thank you! I completely agree. Issue created, and we will address this.

@sedghi
Copy link
Member

sedghi commented Oct 6, 2025

@fedorov Do you have such data at IDC?

@fedorov
Copy link
Member

fedorov commented Oct 6, 2025

Not yet - we've been holding it up to get visualization supported. But we have several datasets that are being prepared right now - those we discussed in #5081 and #5082 - and I hope we can get them out in the next IDC release.

@fedorov
Copy link
Member

fedorov commented Oct 6, 2025

Unfortunately, there are still issues:

  • MR point measurements show numbers accompanied by "HU" - not clear what those numbers are, and Hounsfeld units is not applicable to MR
  • we do not see the list of bounding boxes under measurements for SCOORD bounding boxes

@deepakri201 will add details to #5081 / #5082.

@igoroctaviano
Copy link
Contributor Author

Unfortunately, there are still issues:

  • MR point measurements show numbers accompanied by "HU" - not clear what those numbers are, and Hounsfeld units is not applicable to MR
  • we do not see the list of bounding boxes under measurements for SCOORD bounding boxes

@deepakri201 will add details to #5081 / #5082.

I added a screenshot to this issue with the bounding boxes rendered and displayed in the right panel. Please share the dataset you are using.

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.

5 participants