-
Notifications
You must be signed in to change notification settings - Fork 686
Fix: prevent scrolling outside the map #2360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: prevent scrolling outside the map #2360
Conversation
WalkthroughA new private helper method Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TransformHandler
participant clampOffsets
User->>TransformHandler: onMove(dragDelta)
TransformHandler->>TransformHandler: update offsetX, offsetY
TransformHandler->>clampOffsets: constrain offsets within bounds
clampOffsets-->>TransformHandler: return clamped values
TransformHandler->>TransformHandler: apply constrained offsets
User->>TransformHandler: onZoom(scale)
TransformHandler->>TransformHandler: update scale
TransformHandler->>clampOffsets: constrain offsets within bounds
clampOffsets-->>TransformHandler: return clamped values
TransformHandler->>TransformHandler: apply constrained offsets
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/TransformHandler.ts (1)
255-289: Clamping logic looks correct; renamegameHfor consistency.The mathematical derivation matches the implementation and correctly constrains panning to half-viewport outside the map. However, line 260 uses
gameHwhile line 259 usesgameWidth—usegameHeightfor consistency.Apply this diff:
- const gameH = this.game.height(); + const gameHeight = this.game.height(); const scale = this.scale; // Allow panning so that up to half of the viewport can be outside the map on each side. @@ -271,8 +271,8 @@ const minOffsetX = -gameWidth / 2 + (gameWidth - canvasWidth) / (2 * scale); const maxOffsetX = gameWidth / 2 + (gameWidth - canvasWidth) / (2 * scale); - const minOffsetY = -gameH / 2 + (gameH - canvasHeight) / (2 * scale); - const maxOffsetY = gameH / 2 + (gameH - canvasHeight) / (2 * scale); + const minOffsetY = -gameHeight / 2 + (gameHeight - canvasHeight) / (2 * scale); + const maxOffsetY = gameHeight / 2 + (gameHeight - canvasHeight) / (2 * scale);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/TransformHandler.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T15:14:40.664Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1956
File: src/client/InputHandler.ts:430-449
Timestamp: 2025-08-28T15:14:40.664Z
Learning: On trackpads and touchscreens, users don't expect purely vertical two-finger swipes to trigger zoom. Pan detection using deltaX !== 0 is appropriate since any horizontal movement indicates pan intent, and zoom should be handled via pinch gestures instead.
Applied to files:
src/client/graphics/TransformHandler.ts
🧬 Code graph analysis (1)
src/client/graphics/TransformHandler.ts (1)
src/client/InputHandler.ts (1)
DragEvent(61-66)
🔇 Additional comments (4)
src/client/graphics/TransformHandler.ts (4)
251-251: Good: Bounds enforced after zoom.Calling
clampOffsets()after zoom operations correctly prevents the view from moving outside the allowed bounds.
295-295: Good: Bounds enforced after drag.Calling
clampOffsets()after drag operations correctly prevents the view from being dragged outside the allowed bounds.
217-224: Consider clamping offsets ingoTo()animation.The
goTo()method animates the camera toward a target (player, position, or unit), but doesn't callclampOffsets(). If the target is near a map edge, the animation might move the view outside the newly established bounds.Verify whether this causes issues by:
- Zooming out fully
- Using "go to player" on a player at a map corner
- Observing if the view temporarily violates bounds during animation
If confirmed, apply this diff:
this.offsetY += Math.max( Math.min((this.target.y - screenY) * r, CAMERA_MAX_SPEED), -CAMERA_MAX_SPEED, ); + this.clampOffsets(); this.changed = true; }
307-314: Note:override()bypasses clamping.The
override()method directly sets view position without callingclampOffsets(). This might be intentional (the name suggests bypassing normal constraints), but verify whether bounds should be enforced here or if there's a specific reason to allow unrestricted positioning.
evanpelle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description:
Prevent users from "scrolling"/moving the map outside of viewport and "being lost and unable to find the map back". This can happen by pressing keys intentionally (RIP me) or conflict with browser shortcut containing a WASD key which would keep moving.
Related to reports from discord as highlighted by here:
https://discord.com/channels/1359946986937258015/1360078040222142564/1432750863994192003
Here is the new behavior. I am actively pressing WASD key to try to "get out" but I can't. Same with mouse clicks.
https://www.loom.com/share/a9ecb0a7514d4e54b92d24678833eb2e
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
sorikairo