feat: Make screenshot mouse coordinates easier to verify#1473
Conversation
Use Game View coordinates consistently for mouse input so rendering screenshot points can be passed directly at the default scale, and expose the converted Unity input position in tool responses. - add a raycast tool and raycast-grid screenshot annotations for 3D click checks - report screenshot/input coordinate systems and formulas in CLI docs and skills - cover coordinate conversion, raycast, and mouse input metadata with tests
Restore the screenshot-to-input Y offset metadata for rendering captures so raw image pixels can be converted even when the Game View input area is taller than the RenderTexture. Align runtime tool descriptions and skills with the formula-based coordinate contract.
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new ChangesRaycast and coordinate system unification
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
Packages/src/Editor/Utils/EditorWindowCaptureUtility.cs (1)
196-201: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing warning on RenderTexture unavailable, unlike sibling method.
CaptureGameRenderingAsynclogs a warning when the GameView RenderTexture is unavailable, butGetGameRenderingImageInfoAsyncsilently falls back to(gameViewSize, 0)without any log. Since this offset now feeds coordinate-conversion consumers (e.g., raycast-grid annotation), a silent fallback could mask a real rendering problem.🪵 Proposed fix for logging parity
RenderTexture rt = GameViewBridge.GetRenderTexture(); if (rt == null) { + Debug.LogWarning("[EditorWindowCaptureUtility] GameView RenderTexture is not available"); return (gameViewSize, 0); }🤖 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/Utils/EditorWindowCaptureUtility.cs` around lines 196 - 201, `GetGameRenderingImageInfoAsync` silently returns the fallback when `GameViewBridge.GetRenderTexture()` is null, unlike `CaptureGameRenderingAsync`, which already warns on the same failure. Update `GetGameRenderingImageInfoAsync` to log a warning before returning `(gameViewSize, 0)`, using the same `GameViewBridge.GetRenderTexture` null-check path and matching the existing logging style so both methods behave consistently when the RenderTexture is unavailable.Packages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputTool.cs (1)
203-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated coordinate-metadata block between
ExecuteClickandExecuteLongPress.The same 8 coordinate-metadata field assignments are copy-pasted in both methods. Extracting a small helper would reduce duplication and prevent drift if the metadata contract changes again.
♻️ Proposed refactor
+ private static void ApplyCoordinateMetadata( + SimulateMouseInputResponse response, GameViewCoordinateConversion conversion) + { + response.InputCoordinateSystem = McpConstants.COORDINATE_SYSTEM_TOP_LEFT_GAME_VIEW; + response.UnityCoordinateSystem = McpConstants.COORDINATE_SYSTEM_BOTTOM_LEFT_GAME_VIEW; + response.GameViewWidth = conversion.GameViewSize.x; + response.GameViewHeight = conversion.GameViewSize.y; + response.InputPositionX = conversion.InputPosition.x; + response.InputPositionY = conversion.InputPosition.y; + response.InjectedUnityPositionX = conversion.InjectedUnityPosition.x; + response.InjectedUnityPositionY = conversion.InjectedUnityPosition.y; + response.CoordinateConversionFormula = McpConstants.COORDINATE_CONVERSION_FORMULA_GAME_VIEW_INPUT_TO_UNITY; + }Then call
ApplyCoordinateMetadata(response, conversion);after constructing each response instead of repeating the assignments inline.Also applies to: 270-288
🤖 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/Api/McpTools/SimulateMouseInput/SimulateMouseInputTool.cs` around lines 203 - 220, The coordinate-metadata assignments are duplicated in both ExecuteClick and ExecuteLongPress, which should be centralized. Extract the repeated SimulateMouseInputResponse field population into a small helper such as ApplyCoordinateMetadata(response, conversion), then call it after constructing each response so the shared GameView/Input/InjectedUnity coordinate fields and CoordinateConversionFormula stay in sync. Keep the click/long-press-specific fields in their respective methods and leave the common metadata in the helper.Packages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputResponse.cs (1)
11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant coordinate fields:
PositionX/PositionYduplicateInputPositionX/InputPositionY.Per the call sites in
SimulateMouseInputTool.cs,PositionX = inputPos.xandInputPositionX = conversion.InputPosition.xalways hold the same value, sinceconversion.InputPositionis derived directly frominputPos. Likely intentional for backward compatibility, but worth a note in case consumers should migrate to the newer coordinate-namespaced fields exclusively.🤖 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/Api/McpTools/SimulateMouseInput/SimulateMouseInputResponse.cs` around lines 11 - 21, `SimulateMouseInputResponse` exposes redundant `PositionX`/`PositionY` alongside `InputPositionX`/`InputPositionY`, and the call sites in `SimulateMouseInputTool` populate both with the same input-space values. Keep the legacy `Position*` fields only for backward compatibility, but update the response/type documentation or naming notes to clearly mark them as aliases of the newer `InputPosition*` fields and indicate consumers should migrate to the coordinate-namespaced properties.Assets/Tests/Editor/RaycastToolTests.cs (1)
51-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for camera-not-found and invalid-MaxDistance response paths.
Existing tests only cover hit/no-hit/auto-sync scenarios. Adding a case for "no
Camera.main" would have caught that the response is missing coordinate metadata in that branch (see comment onRaycastTool.cs), and a case forMaxDistance <= 0would lock in the validation-error contract.🤖 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 `@Assets/Tests/Editor/RaycastToolTests.cs` around lines 51 - 104, Add two test cases in RaycastToolTests to cover the untested RaycastTool.ExecuteAsync branches: one for when Camera.main is missing, and one for when MaxDistance is invalid (<= 0). In the no-camera case, assert the response still includes the expected coordinate metadata fields on RaycastResponse, matching the contract used by the other ExecuteAsync tests. In the invalid-MaxDistance case, assert the validation error behavior and response shape are preserved, so the guard in ExecuteRaycast/ExecuteAsync is covered alongside the existing hit, miss, and auto-sync tests.
🤖 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/Cli`~/src/default-tools.json:
- Around line 385-400: The simulate-mouse-ui tool description is inconsistent
about coordinate space, mixing Game View pixels with EventSystem screen
coordinates. Update the surrounding schema text and examples in
default-tools.json, especially the simulate-mouse-ui entry and its field
descriptions such as X, Y, FromX, and FromY, so they all refer to the same
coordinate system throughout.
In `@Packages/src/Editor/Api/McpTools/Raycast/RaycastTool.cs`:
- Around line 35-42: The CameraFound false branch in RaycastTool.Raycast should
not build a fresh RaycastResponse and lose the precomputed conversion metadata
from GameViewRaycastUtility.RaycastFromInputPosition. Update the failure path to
use CreateBaseResponse (or otherwise preserve RaycastResult.Conversion) so the
returned response still includes InputPositionX/Y, InjectedUnityPositionX/Y,
GameViewWidth/Height, and CoordinateConversionFormula, while only setting
Success false and the missing-camera message.
In `@Packages/src/Editor/Api/McpTools/Screenshot/ScreenshotTool.cs`:
- Around line 220-224: The window-capture path in ScreenshotTool is dropping the
requested resolution scale, so ScreenshotInfo always ends up with the default
1.0f. Update the ScreenshotTool window capture flow where ScreenshotInfo is
constructed (including the branch that uses ApplyWindowCoordinateMetadata) to
pass parameters.ResolutionScale through the ScreenshotInfo constructor, matching
the rendering capture path, and apply the same fix to the additional
window-capture block referenced by the review.
- Around line 63-88: The ElementsOnly path in ScreenshotTool.Screenshot
currently drops the real raycast-grid offset, so ScreenshotInfo ends up with the
default ImageToInputOffsetY instead of the value computed by
EditorWindowCaptureUtility.GetGameRenderingImageInfoAsync. Pass the captured
initialImageToInputOffsetY through to ApplyRenderingCoordinateMetadata when
building elementsOnlyInfo, and keep the AnnotateRaycastGrid and ElementsOnly
flows consistent so ScreenshotResponse reports the same offset used by
RaycastGridAnnotator.
- Around line 69-77: Use one settled Game View snapshot for both raycast-grid
points and PNG metadata in ScreenshotTool. Capture the game view geometry once
in the AnnotateRaycastGrid path, then pass the same size/offset data into
CaptureGameRenderingAsync instead of recomputing with
Handles.GetMainGameViewSize; update GetGameRenderingImageInfoAsync,
CollectRaycastGridPoints, and the ImageToInputOffsetY calculation so both
overlay points and returned metadata come from the same captured values.
In `@Packages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiSchema.cs`:
- Around line 10-19: Update the `SimulateMouseUiSchema` property descriptions
for `X` and `Y` so they describe all actions that use these coordinates, not
just `Drag`; the current text is misleading because `Click`, `LongPress`,
`DragStart`, `DragMove`, and `DragEnd` also rely on them. Adjust the `X` and `Y`
`Description` attributes in `SimulateMouseUiSchema` to match the full action
contract while keeping the `FromX`/`FromY` drag-start wording as-is.
In `@Packages/src/Editor/Api/McpTools/SimulateMouseUi/Skill/SKILL.md`:
- Around line 31-34: The parameter table in SKILL.md is out of sync with the
action table for the SimulateMouseUi workflow. Update the descriptions for `--x`
and `--y` so they reflect all actions that use them, including `Click`,
`LongPress`, `DragStart`, `DragMove`, and `DragEnd`, and not just Drag
destinations. Make sure the wording in the parameter table matches the action
definitions referenced by the SimulateMouseUi skill.
In `@Packages/src/README.md`:
- Line 157: The bundled skills list count in the README is now out of sync
because `/uloop-raycast` was added to the section. Update the section heading
that summarizes the count so it matches the current number of items in the
bundled skills list, and keep the heading aligned with the entries shown below
it.
In `@Packages/src/TOOL_REFERENCE.md`:
- Around line 229-230: The simulate-mouse-ui reference has mixed coordinate
terminology: the parameter bullets in TOOL_REFERENCE.md now use Game View
pixels, but the surrounding intro still describes EventSystem screen
coordinates. Update the nearby prose for simulate-mouse-ui so it consistently
advertises only one coordinate system, and keep the wording aligned with the X
and Y parameter descriptions.
In `@README.md`:
- Line 157: The bundled skills list is now 17 items long after adding
/uloop-raycast, but the section heading still reflects 16. Update the heading
text in the README section that lists the bundled skills so it matches the
current count and stays in sync with the list.
---
Nitpick comments:
In `@Assets/Tests/Editor/RaycastToolTests.cs`:
- Around line 51-104: Add two test cases in RaycastToolTests to cover the
untested RaycastTool.ExecuteAsync branches: one for when Camera.main is missing,
and one for when MaxDistance is invalid (<= 0). In the no-camera case, assert
the response still includes the expected coordinate metadata fields on
RaycastResponse, matching the contract used by the other ExecuteAsync tests. In
the invalid-MaxDistance case, assert the validation error behavior and response
shape are preserved, so the guard in ExecuteRaycast/ExecuteAsync is covered
alongside the existing hit, miss, and auto-sync tests.
In
`@Packages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputResponse.cs`:
- Around line 11-21: `SimulateMouseInputResponse` exposes redundant
`PositionX`/`PositionY` alongside `InputPositionX`/`InputPositionY`, and the
call sites in `SimulateMouseInputTool` populate both with the same input-space
values. Keep the legacy `Position*` fields only for backward compatibility, but
update the response/type documentation or naming notes to clearly mark them as
aliases of the newer `InputPosition*` fields and indicate consumers should
migrate to the coordinate-namespaced properties.
In
`@Packages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputTool.cs`:
- Around line 203-220: The coordinate-metadata assignments are duplicated in
both ExecuteClick and ExecuteLongPress, which should be centralized. Extract the
repeated SimulateMouseInputResponse field population into a small helper such as
ApplyCoordinateMetadata(response, conversion), then call it after constructing
each response so the shared GameView/Input/InjectedUnity coordinate fields and
CoordinateConversionFormula stay in sync. Keep the click/long-press-specific
fields in their respective methods and leave the common metadata in the helper.
In `@Packages/src/Editor/Utils/EditorWindowCaptureUtility.cs`:
- Around line 196-201: `GetGameRenderingImageInfoAsync` silently returns the
fallback when `GameViewBridge.GetRenderTexture()` is null, unlike
`CaptureGameRenderingAsync`, which already warns on the same failure. Update
`GetGameRenderingImageInfoAsync` to log a warning before returning
`(gameViewSize, 0)`, using the same `GameViewBridge.GetRenderTexture` null-check
path and matching the existing logging style so both methods behave consistently
when the RenderTexture is unavailable.
🪄 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: ff8733a1-70fa-4682-abfe-3564dea67d6e
⛔ Files ignored due to path filters (14)
Assets/Tests/Editor/EditorWindowCaptureUtilityTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/GameViewCoordinateUtilityTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/RaycastGridAnnotatorTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/RaycastToolTests.cs.metais excluded by none and included by nonePackages/src/Editor/Api/McpTools/Raycast.metais excluded by none and included by nonePackages/src/Editor/Api/McpTools/Raycast/RaycastResponse.cs.metais excluded by none and included by nonePackages/src/Editor/Api/McpTools/Raycast/RaycastSchema.cs.metais excluded by none and included by nonePackages/src/Editor/Api/McpTools/Raycast/RaycastTool.cs.metais excluded by none and included by nonePackages/src/Editor/Api/McpTools/Raycast/Skill.metais excluded by none and included by nonePackages/src/Editor/Api/McpTools/Raycast/Skill/SKILL.md.metais excluded by none and included by nonePackages/src/Editor/Api/McpTools/Screenshot/RaycastGridPointInfo.cs.metais excluded by none and included by nonePackages/src/Editor/Utils/GameViewCoordinateUtility.cs.metais excluded by none and included by nonePackages/src/Editor/Utils/GameViewRaycastUtility.cs.metais excluded by none and included by nonePackages/src/Editor/Utils/RaycastGridAnnotator.cs.metais excluded by none and included by none
📒 Files selected for processing (32)
Assets/Tests/Editor/EditorWindowCaptureUtilityTests.csAssets/Tests/Editor/GameViewCoordinateUtilityTests.csAssets/Tests/Editor/RaycastGridAnnotatorTests.csAssets/Tests/Editor/RaycastToolTests.csAssets/Tests/PlayMode/SimulateMouseInputTests.csPackages/packages-lock.jsonPackages/src/Cli~/src/default-tools.jsonPackages/src/Editor/Api/McpTools/Raycast/RaycastResponse.csPackages/src/Editor/Api/McpTools/Raycast/RaycastSchema.csPackages/src/Editor/Api/McpTools/Raycast/RaycastTool.csPackages/src/Editor/Api/McpTools/Raycast/Skill/SKILL.mdPackages/src/Editor/Api/McpTools/Screenshot/RaycastGridPointInfo.csPackages/src/Editor/Api/McpTools/Screenshot/ScreenshotResponse.csPackages/src/Editor/Api/McpTools/Screenshot/ScreenshotSchema.csPackages/src/Editor/Api/McpTools/Screenshot/ScreenshotTool.csPackages/src/Editor/Api/McpTools/Screenshot/Skill/SKILL.mdPackages/src/Editor/Api/McpTools/Screenshot/Skill/references/annotated-elements.mdPackages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputResponse.csPackages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputSchema.csPackages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputTool.csPackages/src/Editor/Api/McpTools/SimulateMouseInput/Skill/SKILL.mdPackages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiSchema.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/Skill/SKILL.mdPackages/src/Editor/Shared/Config/McpConstants.csPackages/src/Editor/Utils/EditorWindowCaptureUtility.csPackages/src/Editor/Utils/GameViewCoordinateUtility.csPackages/src/Editor/Utils/GameViewRaycastUtility.csPackages/src/Editor/Utils/RaycastGridAnnotator.csPackages/src/README.mdPackages/src/TOOL_REFERENCE.mdPackages/src/package.jsonREADME.md
There was a problem hiding this comment.
7 issues found across 46 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Keep screenshot metadata, raycast results, and UI mouse documentation consistent so review tools and agents can rely on the same Game View coordinate contract.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Utils/EditorWindowCaptureUtility.cs`:
- Around line 155-158: The null-RenderTexture fallback is inconsistent between
CaptureGameRenderingAsync and GetGameRenderingImageInfoAsync, causing different
metadata for the same missing-texture state. Update the fallback path in
EditorWindowCaptureUtility so both methods use the same sane default by passing
the game view size and a zero offset into CreateGameRenderingImageInfo, rather
than a zero-sized image. Keep the behavior consistent wherever the unavailable
RenderTexture case is handled.
🪄 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: d2f42610-4185-4a25-96bc-008ebbb16391
📒 Files selected for processing (14)
Assets/Tests/Editor/RaycastToolTests.csPackages/src/Cli~/src/default-tools.jsonPackages/src/Editor/Api/McpTools/Raycast/RaycastTool.csPackages/src/Editor/Api/McpTools/Screenshot/ScreenshotTool.csPackages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputTool.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiSchema.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiTool.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/Skill/SKILL.mdPackages/src/Editor/Utils/EditorWindowCaptureUtility.csPackages/src/Editor/Utils/GameViewRaycastUtility.csPackages/src/Editor/Utils/RaycastGridAnnotator.csPackages/src/README.mdPackages/src/TOOL_REFERENCE.mdREADME.md
✅ Files skipped from review due to trivial changes (2)
- Packages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiTool.cs
- Packages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiSchema.cs
🚧 Files skipped from review as they are similar to previous changes (9)
- Packages/src/Editor/Utils/RaycastGridAnnotator.cs
- Packages/src/Editor/Utils/GameViewRaycastUtility.cs
- Packages/src/README.md
- Packages/src/Editor/Api/McpTools/Raycast/RaycastTool.cs
- README.md
- Packages/src/Editor/Api/McpTools/SimulateMouseInput/SimulateMouseInputTool.cs
- Packages/src/Cli~/src/default-tools.json
- Packages/src/Editor/Api/McpTools/Screenshot/ScreenshotTool.cs
- Packages/src/TOOL_REFERENCE.md
Use the same Game View-sized fallback metadata when a rendering texture is unavailable so screenshot coordinate formulas do not report a full-height Y offset for a missing image.
Summary
uloop raycastcan check what a Game View coordinate hits before clicking.User Impact
Mouse.current.positioncould look contradictory when the Game View render texture did not match the Unity input area.Changes
Verification
uloop compileuloop run-tests --test-mode EditMode --filter-type regex --filter-value "(GameViewCoordinateUtilityTests|RaycastGridAnnotatorTests|RaycastToolTests|EditorWindowCaptureUtilityTests)"uloop run-tests --test-mode PlayMode --filter-type exact --filter-value "Tests.PlayMode.SimulateMouseInputTests"npm run buildautoreview --mode branch --base origin/mainSummary
raycastMCP tool (schema + response DTO + docs) that raycasts fromCamera.mainusing top-left Game View (X,Y) coordinates and returns hit details plus shared coordinate-conversion metadata.screenshot,simulate-mouse-input, andsimulate-mouse-uiby standardizing coordinate system naming (top-left game view vs bottom-left injected Unity), Game View dimensions, screenshot→input conversion formulas, and injected Unity positions (including the internal Y-axis handling rules).ImageToInputOffsetY/ screenshot-to-input offset), and updated screenshot response/contracts to useImageCoordinateSystemand conversion/offset fields.screenshot(AnnotateRaycastGrid) and introduced shared point/overlay models (RaycastGridPointInfo, grid annotator that syncs physics and produces injected Unity positions + hit labels).GameViewCoordinateUtility,GameViewRaycastUtility) and physics-transform sync behavior needed for reliable raycast results.uloop raycastflow.raycasthit/miss/error cases (including camera-missing andPhysics.autoSyncTransformshandling).