resolved issue #876: Pinch Zoom Y-Axis Calculation Error#978
resolved issue #876: Pinch Zoom Y-Axis Calculation Error#978MaheepTulsian wants to merge 1 commit intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughA calculation error in the pinch-zoom functionality within 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/simulator/src/listeners.js (4)
153-154:⚠️ Potential issue | 🟡 MinorCanvas pan resets to the origin when zoom is clamped.
globalScope.ox/globalScope.oyare overwritten withcurrCentreX * (scale - oldScale). WhenpinchZhits its floor (0.5) or ceiling (2),globalScope.scale === oldScale, making the product0and snapping the canvas to the origin mid-gesture. These should accumulate into the existing offset, not replace it.🐛 Proposed fix
- globalScope.ox = Math.round(currCentreX * (globalScope.scale - oldScale)); - globalScope.oy = Math.round(currCentreY * (globalScope.scale - oldScale)); + globalScope.ox += Math.round(currCentreX * (globalScope.scale - oldScale)); + globalScope.oy += Math.round(currCentreY * (globalScope.scale - oldScale));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` around lines 153 - 154, The pan offsets are being overwritten causing the canvas to snap to origin when pinchZ is clamped; change the assignment so the computed delta (currCentreX * (globalScope.scale - oldScale) and currCentreY * (globalScope.scale - oldScale)) is added to the existing globalScope.ox/globalScope.oy instead of replacing them (i.e., accumulate the delta into globalScope.ox and globalScope.oy), ensuring the code that updates offsets (referencing globalScope.ox, globalScope.oy, currCentreX, currCentreY, globalScope.scale and oldScale/pinchZ) uses addition rather than assignment.
153-154:⚠️ Potential issue | 🟡 MinorCanvas pan position resets to origin when zoom is clamped.
globalScope.oxandglobalScope.oyare overwritten rather than accumulated. WhenpinchZreaches its boundary (0.5 or 2),globalScope.scaleequalsoldScale, making(globalScope.scale - oldScale) === 0. Both offsets are zeroed out, snapping the canvas to the origin mid-gesture.🐛 Proposed fix (accumulate delta rather than overwrite)
- globalScope.ox = Math.round(currCentreX * (globalScope.scale - oldScale)); - globalScope.oy = Math.round(currCentreY * (globalScope.scale - oldScale)); + globalScope.ox += Math.round(currCentreX * (globalScope.scale - oldScale)); + globalScope.oy += Math.round(currCentreY * (globalScope.scale - oldScale));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` around lines 153 - 154, The pan offsets are being overwritten which zeros them when zoom is clamped; change the assignment to accumulate the delta instead: compute the delta as Math.round(currCentreX * (globalScope.scale - oldScale)) and Math.round(currCentreY * (globalScope.scale - oldScale)) and add those deltas to globalScope.ox and globalScope.oy (instead of setting them), so in the pinch/zoom handler (symbols: globalScope.ox, globalScope.oy, globalScope.scale, oldScale, currCentreX, currCentreY, pinchZ) the offsets are incremented by the zoom-induced delta and the canvas won't snap to origin when scale is clamped.
126-126:⚠️ Potential issue | 🟠 Major
Math.sqrtsilently drops the Y component — pinch distance is horizontal-only.
Math.sqrt()returns the square root of a number and accepts only one parameter. The second argument(e.touches[1].clientY - e.touches[0].clientY) ** 2is silently ignored by the JS runtime, sodistanceresolves to|Δx|— purely horizontal. A mostly-vertical or diagonal pinch gesture produces near-zerodistance, makingpinchZbarely change and rendering the zoom unresponsive for those orientations.The idiomatic fix is
Math.hypot(), which returns the square root of the sum of squares of its arguments:🐛 Proposed fix
- distance = Math.sqrt((e.touches[1].clientX - e.touches[0].clientX) ** 2, (e.touches[1].clientY - e.touches[0].clientY) ** 2); + distance = Math.hypot(e.touches[1].clientX - e.touches[0].clientX, e.touches[1].clientY - e.touches[0].clientY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` at line 126, The computed pinch distance incorrectly passes two arguments to Math.sqrt making the Y delta ignored (so distance effectively equals |Δx|); update the calculation for distance in the touch handler (the line that assigns to distance using e.touches[0]/e.touches[1]) to compute the true Euclidean distance either by using Math.hypot(e.touches[1].clientX - e.touches[0].clientX, e.touches[1].clientY - e.touches[0].clientY) or by summing squares and taking Math.sqrt((dx*dx)+(dy*dy)); this fixes pinchZ/zoom responsiveness for vertical/diagonal gestures.
126-126:⚠️ Potential issue | 🟠 Major
Math.sqrtsilently ignores the second argument — distance is horizontal-only.
Math.sqrtaccepts exactly one argument per the ECMAScript specification; the Y-component(e.touches[1].clientY - e.touches[0].clientY) ** 2is silently discarded. The computeddistanceequals|Δx|, not true Euclidean distance. A primarily vertical pinch gesture produces near-zerodistance(e.g., vertical-only pinch with Δx=0, Δy=5 yieldsMath.sqrt(0, 25) = 0), meaningpinchZbarely changes and zoom is unresponsive for vertical axis pinches.🐛 Proposed fix
- distance = Math.sqrt((e.touches[1].clientX - e.touches[0].clientX) ** 2, (e.touches[1].clientY - e.touches[0].clientY) ** 2); + distance = Math.sqrt((e.touches[1].clientX - e.touches[0].clientX) ** 2 + (e.touches[1].clientY - e.touches[0].clientY) ** 2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` at line 126, The computed `distance` in listeners.js is wrong because Math.sqrt is called with two arguments, discarding the Y component; fix the calculation of `distance` (used to compute pinch/zoom, e.g., `distance` and `pinchZ`) by summing the squared Δx and Δy and then taking Math.sqrt of that single sum (i.e., compute dx = e.touches[1].clientX - e.touches[0].clientX, dy = e.touches[1].clientY - e.touches[0].clientY, then distance = Math.sqrt(dx*dx + dy*dy)) so vertical-only pinches change `distance` correctly and zoom/pinch logic behaves as expected.
🧹 Nitpick comments (2)
src/simulator/src/listeners.js (2)
143-154: Lingering "not working as expected" comment — zoom-to-pinch-center is still inaccurate.The comment on Line 143 was already present and still applies: the centre-based pan update doesn't follow the standard zoom-to-point formula. The correct derivation for preserving the world point under the pinch centre after a scale change is
new_ox = RawX - Xf_pixels * new_scale(and equivalently for Y), whereXf_pixels = (RawX - old_ox). The current grid-snapped approximation (currCentreX * Δscale) will drift from the actual pinch midpoint.Would you like me to open a follow-up issue to track a full zoom-to-pinch-center implementation, or generate a corrected formula as a starting point?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` around lines 143 - 154, The zoom-to-pinch-centre math is incorrect: replace the grid-snapped approximation that sets globalScope.ox/globalScope.oy using currCentreX * (globalScope.scale - oldScale) with the proper preserve-point formula; compute new offsets as newOx = RawX - ((RawX - globalScope.ox) / oldScale) * globalScope.scale and newOy = RawY - ((RawY - globalScope.oy) / oldScale) * globalScope.scale (use RawX/RawY, globalScope.ox/oy, oldScale and the updated globalScope.scale) and assign them to globalScope.ox/globalScope.oy, removing the currCentreX/currCentreY rounding-based approach.
143-154: Lingering "not working as expected" comment indicates the zoom-to-center algorithm is still incomplete.After the
oyfix, the midpoint coordinates (centreX/centreY) are correctly fed into both axes, but the comment on line 143 still applies: the resultingox/oyupdate doesn't implement standard zoom-to-point semantics (which would requirenew_ox = RawX - Xf_pixels * new_scale). The current formula computes a grid-snapped approximation that drifts from the actual pinch center.Would you like me to open a new issue to track the full zoom-to-pinch-center implementation, or generate a corrected formula draft?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` around lines 143 - 154, The current pinch-center math updates globalScope.ox/oy using a grid-snapped delta which drifts from the true pinch point; replace that with the standard zoom-to-point formula: compute new_ox = RawX - ((RawX - globalScope.ox) / oldScale) * globalScope.scale and new_oy = RawY - ((RawY - globalScope.oy) / oldScale) * globalScope.scale (using RawX/RawY, oldScale and globalScope.scale from the diff), and if you still need grid snapping apply rounding to the final world coordinates after this calculation rather than using the snapped Xf/Yf to derive ox/oy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/simulator/src/listeners.js`:
- Around line 153-154: The pan offsets are being overwritten causing the canvas
to snap to origin when pinchZ is clamped; change the assignment so the computed
delta (currCentreX * (globalScope.scale - oldScale) and currCentreY *
(globalScope.scale - oldScale)) is added to the existing
globalScope.ox/globalScope.oy instead of replacing them (i.e., accumulate the
delta into globalScope.ox and globalScope.oy), ensuring the code that updates
offsets (referencing globalScope.ox, globalScope.oy, currCentreX, currCentreY,
globalScope.scale and oldScale/pinchZ) uses addition rather than assignment.
- Around line 153-154: The pan offsets are being overwritten which zeros them
when zoom is clamped; change the assignment to accumulate the delta instead:
compute the delta as Math.round(currCentreX * (globalScope.scale - oldScale))
and Math.round(currCentreY * (globalScope.scale - oldScale)) and add those
deltas to globalScope.ox and globalScope.oy (instead of setting them), so in the
pinch/zoom handler (symbols: globalScope.ox, globalScope.oy, globalScope.scale,
oldScale, currCentreX, currCentreY, pinchZ) the offsets are incremented by the
zoom-induced delta and the canvas won't snap to origin when scale is clamped.
- Line 126: The computed pinch distance incorrectly passes two arguments to
Math.sqrt making the Y delta ignored (so distance effectively equals |Δx|);
update the calculation for distance in the touch handler (the line that assigns
to distance using e.touches[0]/e.touches[1]) to compute the true Euclidean
distance either by using Math.hypot(e.touches[1].clientX - e.touches[0].clientX,
e.touches[1].clientY - e.touches[0].clientY) or by summing squares and taking
Math.sqrt((dx*dx)+(dy*dy)); this fixes pinchZ/zoom responsiveness for
vertical/diagonal gestures.
- Line 126: The computed `distance` in listeners.js is wrong because Math.sqrt
is called with two arguments, discarding the Y component; fix the calculation of
`distance` (used to compute pinch/zoom, e.g., `distance` and `pinchZ`) by
summing the squared Δx and Δy and then taking Math.sqrt of that single sum
(i.e., compute dx = e.touches[1].clientX - e.touches[0].clientX, dy =
e.touches[1].clientY - e.touches[0].clientY, then distance = Math.sqrt(dx*dx +
dy*dy)) so vertical-only pinches change `distance` correctly and zoom/pinch
logic behaves as expected.
---
Nitpick comments:
In `@src/simulator/src/listeners.js`:
- Around line 143-154: The zoom-to-pinch-centre math is incorrect: replace the
grid-snapped approximation that sets globalScope.ox/globalScope.oy using
currCentreX * (globalScope.scale - oldScale) with the proper preserve-point
formula; compute new offsets as newOx = RawX - ((RawX - globalScope.ox) /
oldScale) * globalScope.scale and newOy = RawY - ((RawY - globalScope.oy) /
oldScale) * globalScope.scale (use RawX/RawY, globalScope.ox/oy, oldScale and
the updated globalScope.scale) and assign them to globalScope.ox/globalScope.oy,
removing the currCentreX/currCentreY rounding-based approach.
- Around line 143-154: The current pinch-center math updates globalScope.ox/oy
using a grid-snapped delta which drifts from the true pinch point; replace that
with the standard zoom-to-point formula: compute new_ox = RawX - ((RawX -
globalScope.ox) / oldScale) * globalScope.scale and new_oy = RawY - ((RawY -
globalScope.oy) / oldScale) * globalScope.scale (using RawX/RawY, oldScale and
globalScope.scale from the diff), and if you still need grid snapping apply
rounding to the final world coordinates after this calculation rather than using
the snapped Xf/Yf to derive ox/oy.
1 participant
Fixes #876
Describe the changes you have made in this PR -
Pinch zoom on mobile was calculating Y-axis position wrongly at listeners.js line 118 because it was using globalScope.ox (X offset) instead of globalScope.oy (Y offset). I've corrected it back to globalScope.oy (Y offset).
Screenshots of the UI changes (If any) -
Screen.Recording.2026-02-23.at.11.37.25.PM.mov
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Pinch zoom on mobile was calculating Y-axis position wrongly because it was using globalScope.ox (X offset) instead of globalScope.oy (Y offset). It was a clear typo error.
Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.
Summary by CodeRabbit