fix(core): return CSS pixels from the CPU worldToCanvas/canvasToWorld - #2850
Conversation
fix(core): return CSS pixels from the CPU worldToCanvas/canvasToWorld The CPU transform built by calculateTransform is anchored on the canvas backing store, which getOrCreateCanvas sizes in device pixels, so pixelToCanvas and canvasToPixel speak device pixels. StackViewport passed their results straight through worldToCanvasCPU and canvasToWorldCPU, making the CPU path the only viewport path whose public coordinates are not CSS pixels: the GPU implementations divide by devicePixelRatio before returning, and the tools event layer derives its canvas points from getBoundingClientRect, which has no DPR term. The two agree only at devicePixelRatio 1, which is why this went unnoticed. At any other ratio - HiDPI display, zoomed browser, mobile device - interaction is mis-scaled by exactly that factor, most visibly as panning that moves the image devicePixelRatio times further than the pointer. The offset is permanent rather than transient, zero at the canvas centre and growing with distance from it. Convert at the public boundary only. The conversion deliberately does not live inside pixelToCanvas/canvasToPixel: internal rendering callers pass device pixels to those on purpose, and setToPixelCoordinateSystem applies the same transform to a 2D context where device pixels are the correct unit. The three internal round trips through canvasToPixel are bridged so their results are byte-for-byte unchanged. resetCameraCPU already passed element.clientWidth/2 - CSS pixels - into canvasToWorldCPU, so it was silently wrong at DPR != 1 and is corrected by this change. Reproduce before the fix: open a stack example with ?cpu=1, set browser zoom to 150%, and pan. Fixes #2849 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> @
📝 WalkthroughWalkthroughThe CPU fallback now converts public CSS-pixel coordinates to device pixels for internal transforms and converts results back to CSS pixels. New helpers handle device-pixel ratios, and tests cover scaling, fallback behavior, and round trips. ChangesCPU CSS Pixel Conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@jbocce - I've tested this on a tablet device (amazon fire) and it resolves hte scaling transform issue. |
| const prevFocalPointPixel = canvasToPixel( | ||
| this._cpuFallbackEnabledElement, | ||
| prevFocalPointCanvas | ||
| cssToDevicePixels(prevFocalPointCanvas) |
There was a problem hiding this comment.
Does the if(parallelScale) branch need to consider DPR as well?
jbocce
left a comment
There was a problem hiding this comment.
One small comment/question in the code changed, but I am also wondering why aren't other viewports affected by DPR or is it already handled?
The CPU transform built by calculateTransform is anchored on the canvas backing store, which getOrCreateCanvas sizes in device pixels, so pixelToCanvas and canvasToPixel speak device pixels. StackViewport passed their results straight through worldToCanvasCPU and canvasToWorldCPU, making the CPU path the only viewport path whose public coordinates are not CSS pixels: the GPU implementations divide by devicePixelRatio before returning, and the tools event layer derives its canvas points from getBoundingClientRect, which has no DPR term.
The two agree only at devicePixelRatio 1, which is why this went unnoticed. At any other ratio - HiDPI display, zoomed browser, mobile device - interaction is mis-scaled by exactly that factor, most visibly as panning that moves the image devicePixelRatio times further than the pointer. The offset is permanent rather than transient, zero at the canvas centre and growing with distance from it.
Convert at the public boundary only. The conversion deliberately does not live inside pixelToCanvas/canvasToPixel: internal rendering callers pass device pixels to those on purpose, and setToPixelCoordinateSystem applies the same transform to a 2D context where device pixels are the correct unit. The three internal round trips through canvasToPixel are bridged so their results are byte-for-byte unchanged.
resetCameraCPU already passed element.clientWidth/2 - CSS pixels - into canvasToWorldCPU, so it was silently wrong at DPR != 1 and is corrected by this change.
Reproduce before the fix: open a stack example with ?cpu=1, set browser zoom to 150%, and pan.
Fixes #2849
Note that testing this requires the touchAllTools PR to be merged first.
Context
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment