fix(3d-viewport): preserve image color preset when resetting 3D viewport#5952
fix(3d-viewport): preserve image color preset when resetting 3D viewport#5952GhadeerAlbattarni wants to merge 3 commits intoOHIF:masterfrom
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| page, | ||
| locator: activeViewport.pane, | ||
| screenshotPath: screenShotPaths.reset3D.colorPresetPreservedAfterReset, | ||
| maxDiffPixelRatio: 0.18, |
There was a problem hiding this comment.
maxDiffPixelRatio: 0.18 (18%) is very permissive compared to the existing 3D screenshot test that uses 0.04 (4%). At 18%, a partially-grayscale render could still pass the comparison, which is the exact regression this test is meant to catch. Consider tightening the ratio; the existing 3D test at 4% already accounts for cross-environment color variation in 3D rendering.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/3DOnly.spec.ts
Line: 50
Comment:
**High pixel-diff tolerance may hide regressions**
`maxDiffPixelRatio: 0.18` (18%) is very permissive compared to the existing 3D screenshot test that uses `0.04` (4%). At 18%, a partially-grayscale render could still pass the comparison, which is the exact regression this test is meant to catch. Consider tightening the ratio; the existing 3D test at 4% already accounts for cross-environment color variation in 3D rendering.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
yeah it might be best to simply not have a test for this fix.
| // appearance is preserved. This is because resetProperties() restores the | ||
| // transfer function to grayscale nodes captured at volume load time, | ||
| // which is before the initial preset (e.g. skin color) was ever applied. | ||
| const { preset } = viewport.getProperties?.() ?? {}; |
There was a problem hiding this comment.
I'm not sure this is the correct fix. I think the fix would be in cornerstone.
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
We can live without the test. However, we should spend more time looking at fixing this in CS3D.
2 participants
Context
Fixes #5922
When a user opens a study in 3D layout, the viewport correctly renders with the configured color preset (e.g. CT-Skin showing skin colors). However, clicking
Resetcauses the 3D image to display in black and white.Root cause
The root cause is a timing issue in Cornerstone3D: the "initial" state snapshot of the color transfer function is taken at volume load time, before OHIF applies the configured preset. So when Reset restores that snapshot, it restores a grayscale state that predates the color preset.
Changes & Results
In the
resetViewport(commandsModule.ts), capture the current preset fromviewport.getProperties()before callingresetProperties(), then re-apply it afterwards. This ensures the color preset survives the reset while all other properties (camera, zoom, etc ) are still reset as expected.Before
3dReset-Before.mov
After
3dReset-After.mov
Testing
1- Open a study in the viewer mode (e.g., StudyInstanceUIDs=2.16.840.1.114362.1.11972228.22789312658.616067305.306.2)
2- Switch to 3D layout
3- Verify image color is shown
4- Zoom in
5- Click Reset
6- Verify the image color is preserved and only the camera/zoom/pan is reset
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This PR fixes a timing issue where
resetViewportwas restoring the 3D viewport's color transfer function to a grayscale snapshot taken at volume load time — before OHIF's configured preset was ever applied. The fix capturespresetfromgetProperties()immediately beforeresetProperties()and re-applies it afterward. The guardif (preset)makes the change a no-op for 2D stack viewports wherepresetis not a property.Confidence Score: 5/5
Safe to merge; the fix is minimal, well-guarded, and correct for both 3D and 2D viewports.
No P0 or P1 findings. The only remark is the generous screenshot diff tolerance (18%) in the new test, which is a P2 quality concern and does not block the fix from being correct or safe.
tests/3DOnly.spec.ts — consider tightening maxDiffPixelRatio from 0.18 to match the existing 3D test's 0.04.
Vulnerabilities
No security concerns identified.
Important Files Changed
presetfromgetProperties()beforeresetProperties()and re-applies it afterwards; safe for non-3D viewports becausepresetwill beundefinedand the guard prevents any re-application.reset3D.colorPresetPreservedAfterResetpath entry, consistent with existing naming patterns.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile