fix: Mouse click and long-press coordinates now match the Game View's actual resolution#1662
Merged
Merged
Conversation
Ported from origin/main's c81f6b0. simulate-mouse-input previously flipped the Y coordinate using Screen.height (the runtime window's pixel size), which can diverge from the Game View's target resolution when the view is resized or docked differently — silently misaligning injected clicks with coordinates read from a `screenshot --capture-mode rendering` image. Switches to GameViewCoordinateUtility.ConvertInputToUnity (already used by raycast/screenshot tools, ported during C1) and echoes the full conversion (GameView size, input position, injected Unity position, formula) in the response so callers can verify the math. simulate-mouse-ui needed no change: its MouseUiCoordinateConverter already used Handles.GetMainGameViewSize().
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Game View coordinate metadata to simulate-mouse-input, updates click/long-press execution to use the new conversion, extends the response schema and tests, and revises skill docs and tool schema to document the coordinate sources and output fields. ChangesCoordinate Conversion Metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Executor as MouseInputPressActionExecutor
participant Conversion as GameViewCoordinateConversion
participant InputSystem as Unity Input System
participant Factory as MouseInputSimulationResponseFactory
Executor->>Conversion: ConvertInputToUnity(inputPos)
Conversion-->>Executor: InjectedUnityPosition
Executor->>InputSystem: SetPositionState(InjectedUnityPosition)
Executor->>Factory: SuccessButtonResult(action, message, button, inputPos, conversion)
Factory-->>Executor: SimulateMouseInputResponse with coordinate metadata
Possibly related PRs
🚥 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 |
The previous commit switched the tool's X/Y coordinate basis from Screen.height to the Game View's target resolution, but this project's docs still said "screen pixels" and didn't mention the new response metadata fields, actively misleading callers about how to source coordinates. Updated the X/Y schema description in default-tools.json and the skill source (workflow step 2, parameter table, examples, new Coordinate System section, Output field list), then regenerated the .claude/.agents copies via `uloop skills install --claude --agents`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/SimulateMouseInput/MouseInputSimulationResponseFactory.cs (1)
45-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider including conversion metadata in
InterruptedButtonResult.When the executor hits a pause point, the mouse position was already set via
conversion.InjectedUnityPositionand the button was pressed, yet the interrupted response omits all coordinate-conversion fields. Callers can't verify what was actually injected. Theconversionis available at both call sites inMouseInputPressActionExecutor(lines 120 and 239) but can't be passed because the method signature doesn't accept it.♻️ Suggested refactor
internal static SimulateMouseInputResponse InterruptedButtonResult( UnityCliLoopMouseInputAction action, string buttonName, - Vector2 inputPos) + Vector2 inputPos, + GameViewCoordinateConversion conversion) { SimulateMouseInputResponse result = InterruptedActionResult(action); result.Button = buttonName; result.PositionX = inputPos.x; result.PositionY = inputPos.y; + result.InputCoordinateSystem = UnityCliLoopConstants.COORDINATE_SYSTEM_TOP_LEFT_GAME_VIEW; + result.UnityCoordinateSystem = UnityCliLoopConstants.COORDINATE_SYSTEM_BOTTOM_LEFT_GAME_VIEW; + result.GameViewWidth = conversion.GameViewSize.x; + result.GameViewHeight = conversion.GameViewSize.y; + result.InputPositionX = conversion.InputPosition.x; + result.InputPositionY = conversion.InputPosition.y; + result.InjectedUnityPositionX = conversion.InjectedUnityPosition.x; + result.InjectedUnityPositionY = conversion.InjectedUnityPosition.y; + result.CoordinateConversionFormula = UnityCliLoopConstants.COORDINATE_CONVERSION_FORMULA_GAME_VIEW_INPUT_TO_UNITY; return result; }Then update both call sites in
MouseInputPressActionExecutor.csto passconversion:return MouseInputSimulationResponseFactory.InterruptedButtonResult( UnityCliLoopMouseInputAction.Click, buttonName, - inputPos); + inputPos, + conversion);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/src/Editor/FirstPartyTools/SimulateMouseInput/MouseInputSimulationResponseFactory.cs` around lines 45 - 55, Interrupted mouse button responses are missing the coordinate-conversion metadata that is already available at the pause points. Update MouseInputSimulationResponseFactory.InterruptedButtonResult to accept the conversion object and populate the same conversion fields used elsewhere in the response, then update both MouseInputPressActionExecutor call sites to pass the existing conversion value so callers can see what was injected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/src/Editor/FirstPartyTools/SimulateMouseInput/Skill/SKILL.md`:
- Line 80: The example in SKILL.md hardcodes “center” as if it always maps to
400x300, which is misleading for other Game View sizes. Update the sample label
around the SimulateMouseInput guidance to use a neutral description or derive
the click position from the current Game View dimensions so the example stays
accurate across sizes.
---
Nitpick comments:
In
`@Packages/src/Editor/FirstPartyTools/SimulateMouseInput/MouseInputSimulationResponseFactory.cs`:
- Around line 45-55: Interrupted mouse button responses are missing the
coordinate-conversion metadata that is already available at the pause points.
Update MouseInputSimulationResponseFactory.InterruptedButtonResult to accept the
conversion object and populate the same conversion fields used elsewhere in the
response, then update both MouseInputPressActionExecutor call sites to pass the
existing conversion value so callers can see what was injected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4078d4a1-5083-4120-9f3f-334b658d820c
⛔ Files ignored due to path filters (2)
Assets/Tests/PlayMode/UnityCLILoop.Tests.PlayMode.asmdefis excluded by none and included by nonePackages/src/Editor/FirstPartyTools/SimulateMouseInput/UnityCLILoop.FirstPartyTools.SimulateMouseInput.Editor.asmdefis excluded by none and included by none
📒 Files selected for processing (9)
.agents/skills/uloop-simulate-mouse-input/SKILL.md.claude/skills/uloop-simulate-mouse-input/SKILL.mdAssets/Tests/PlayMode/SimulateMouseInputTests.csPackages/src/Editor/FirstPartyTools/Common/GameView/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/MouseInputPressActionExecutor.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/MouseInputSimulationResponseFactory.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputResponse.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/Skill/SKILL.mdcli/common/tools/default-tools.json
CodeRabbit flagged that the example comment's "Game View center" wording implies (400, 300) is universally the center, but that only holds for one specific Game View resolution. Reworded to "a representative Game View point" and regenerated the .claude/.agents skill copies to match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
simulate-mouse-input's click/long-press coordinates now align with the Game View's target resolution instead of the runtime window's pixel size, so injected clicks land where ascreenshot --capture-mode renderingimage says they should.User Impact
simulate-mouse-inputcould inject clicks at the wrong Y position relative to a screenshot taken with--capture-mode rendering.Changes
MouseInputPressActionExecutor: replaced theScreen.height-based Y-flip withGameViewCoordinateUtility.ConvertInputToUnity(the same utility already used by the raycast/screenshot tools).SimulateMouseInputResponse: added coordinate-system/conversion metadata fields (Game View size, input position, injected Unity position, conversion formula).MouseInputSimulationResponseFactory: added a success-result builder that populates the new metadata.InternalsVisibleTo/asmdef references soSimulateMouseInput.EditorandTests.PlayModecan use the sharedGameViewCoordinateUtility.simulate-mouse-uirequired no change — it already usedHandles.GetMainGameViewSize().Ported from origin/main's
c81f6b0b(mouse-coordinate portion only; the raycast/screenshot portions of that commit were already ported as C1–C3).Verification
uloop compile: 0 errors, 0 warningsuloop run-tests(PlayMode,SimulateMouseInputTests|SimulateMouseUi.*Tests): 46/46 passeduloop run-tests(EditMode, full suite): 1774/1774 passed, 7 pre-existing skipsuloop run-tests(PlayMode, full suite): 92/92 passed