Repository navigation
fix(wsi): stop scaling canvas points by devicePixelRatio in the generic WSI viewport - #2942
namespaceMarcello wants to merge 1 commit into
Conversation
…ic WSI viewport The generic WSI path sizes its transform on the element's clientWidth and clientHeight, CSS pixels, like the legacy WSIViewport, whose double devicePixelRatio scaling cornerstonejs#2769 removed. canvasToIndexForWSI still multiplied the canvas point by the ratio, indexToCanvasForWSI divided by it, and WSIResolvedView.getCenterIndexForCanvasPoint did the same when zooming at the cursor. At a ratio of 2 the canvas centre mapped half a canvas away from the view centre, and annotations drifted off the tissue as the view zoomed. Use the canvas point as is, and drop the devicePixelRatio argument that no caller passed. The test that described the double scaling goes with it; new cases at ratios 1, 2 and 3 check the centre, the step per pixel and zooming at the cursor. Fixes cornerstonejs#2754 Assisted-by: Claude Code Generated-By: Claude Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KdTu6m4ycpVjrxdZbMtztk
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughWSI canvas-to-index and index-to-canvas transforms now use CSS pixels without device-pixel-ratio scaling. Tests cover coordinate mapping, movement, and zoom behavior across device pixel ratios. ChangesWSI Coordinate Handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The CSS-pixel coordinate changes appear ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Context
Fixes #2754 for the generic (planar "next") WSI viewport.
#2769 fixed the legacy
WSIViewport: its canvas is sized in CSS pixels and its transform works in CSS pixels, so scaling canvas points bydevicePixelRatioapplied the ratio twice and misplaced annotations on HiDPI displays. The generic WSI path inGenericViewport/WSIstill does the same scaling:canvasToIndexForWSImultiplies the canvas point bydevicePixelRatioandindexToCanvasForWSIdivides the result by it, while the transform is centred oncanvasWidth / 2, canvasHeight / 2, and the viewport passeselement.clientWidth/clientHeight, which are CSS pixels.WSIResolvedView.getCenterIndexForCanvasPoint, used to zoom at the cursor, multiplies the canvas point the same way.With a ratio of 2, the canvas centre maps to a point half a canvas away from the view centre.
buildICamerathen puts the camera focal point there, and the annotations, which are drawn through this transform, move relative to the slide as the view zooms. Clicks still round-trip, which is why placement looks right until the zoom changes. That is the behaviour reported in #2754.Changes & Results
wsiTransformUtils.ts:canvasToIndexForWSIandindexToCanvasForWSIno longer scale bydevicePixelRatio, and the unuseddevicePixelRatioargument goes (the functions are internal, and no caller passed it).WSIResolvedView.getCenterIndexForCanvasPoint: the canvas point is used as is, in the same CSS-pixel space as the transform.test/wsiTransformUtils.jest.js: new cases at device pixel ratios 1, 2 and 3. The canvas centre maps to the view centre and back; one CSS pixel moves the index by one resolution step; zooming at a canvas point keeps the index under the cursor. The existing test "divides the whole transformed point by an explicit devicePixelRatio argument" described the double scaling (its comment notes that the whole point, centre offset included, scales by 1/dpr). It goes with the argument.At a device pixel ratio of 2, on a 300 × 150 canvas with the view centred on index (30, 70), resolution 2 and no rotation:
Testing
Verified:
main, the centre and step cases fail at ratios 2 and 3 and pass at 1. With the fix all 33 tests in the file pass.indexToCanvasForWSIalone fails only the centre case at ratios 2 and 3. Fixing the utilities but leaving the ratio ingetCenterIndexForCanvasPointfails only the zoom-at-cursor case at ratios 2 and 3.prettier --check,oxlint,tsc --noEmitfor core,jest, and the full karma suite pass locally.To try it:
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals. (No public API change.)
Tested Environment
Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KdTu6m4ycpVjrxdZbMtztk
Summary by CodeRabbit