Respect UI raycast priority in simulate-mouse-ui#1318
Conversation
📝 WalkthroughWalkthroughEnhances UI raycast selection to prefer ScreenSpace-Overlay Graphic hits over mixed EventSystem results using priority-ranked comparison, integrates the logic into pointer updates, adds three playmode regression tests validating click dispatch with overlaid raycasters, includes test helper raycasters and dependencies, and implements CI workflow verification for built artifacts in fork PRs. ChangesUI Raycast Prioritization and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
Packages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs (1)
21-21: ⚡ Quick winConsider deferring
overlayHitcomputation for better performance.Computing
overlayHitupfront meansRaycastCanvasSpace(which callsFindObjectsByType<Canvas>and enumerates Graphics) runs even when the first EventSystem result is already fromGraphicRaycaster. Deferring the computation until you confirm the first hit is non-Graphics avoids unnecessary work in the common case where overlay UI is correctly prioritized by EventSystem.⚡ Suggested optimization
- RaycastResult? overlayHit = RaycastCanvasSpace(screenPosition); - if (results.Count > 0) { RaycastResult firstHit = results[0]; - if (overlayHit != null && !IsGraphicRaycast(firstHit)) + if (!IsGraphicRaycast(firstHit)) { - return overlayHit; + RaycastResult? overlayHit = RaycastCanvasSpace(screenPosition); + if (overlayHit != null) + { + return overlayHit; + } } return firstHit; } // EventSystem clips at Screen.width/height, which can be smaller than the // Canvas layout space (Game view target resolution). Fall back to manual hit testing. - return overlayHit; + return RaycastCanvasSpace(screenPosition);🤖 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/SimulateMouseUi/UiRaycastHelper.cs` at line 21, The current code eagerly calls RaycastCanvasSpace and assigns overlayHit even when the first EventSystem result comes from a GraphicRaycaster; change this to defer computing overlayHit: after you get the EventSystem raycast results, inspect the first result's module/gameObject (or whether it's a GraphicRaycaster/Graphic) and only call RaycastCanvasSpace() to compute overlayHit if the first hit is not already a UI Graphic; update the overlayHit variable to be nullable/uninitialized and assign it only in that conditional branch so you avoid unnecessary FindObjectsByType<Canvas> and Graphics enumeration.
🤖 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.
Nitpick comments:
In `@Packages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs`:
- Line 21: The current code eagerly calls RaycastCanvasSpace and assigns
overlayHit even when the first EventSystem result comes from a GraphicRaycaster;
change this to defer computing overlayHit: after you get the EventSystem raycast
results, inspect the first result's module/gameObject (or whether it's a
GraphicRaycaster/Graphic) and only call RaycastCanvasSpace() to compute
overlayHit if the first hit is not already a UI Graphic; update the overlayHit
variable to be nullable/uninitialized and assign it only in that conditional
branch so you avoid unnecessary FindObjectsByType<Canvas> and Graphics
enumeration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3791767-5fe9-4544-a856-159afaaac2f3
📒 Files selected for processing (3)
Assets/Tests/PlayMode/SimulateMouseUiTests.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiTool.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs
There was a problem hiding this comment.
2 issues found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Added a second fix for the same hit-priority mismatch: when EventSystem returns a lower-priority GraphicRaycaster hit, simulate-mouse-ui now compares it with the Screen Space Overlay canvas-space candidate using raycaster/canvas priority before choosing the target.\n\nThis covers the case where a lower-order full-screen UI mask is returned before a higher-order modal canvas. I also added a PlayMode regression test for that canvas-order case.\n\nVerification note: I could not run this repository's PlayMode suite locally because Unity 2022.3.62f3 is not installed on this machine. I validated the change in a downstream Unity project by temporarily applying the same package-cache patch: compile passed, and a modal button behind a lower-order full-screen mask changed from a stalled/no-hit click to the expected button hit, allowing the flow to complete. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs (1)
133-196: 💤 Low valuePriority comparison logic correctly implements Unity's raycast ordering.
The comparison follows Unity's documented GraphicRaycaster priority order (sortOrderPriority → renderOrderPriority → sortingLayer → sortingOrder → depth) and properly handles null modules by returning 0 as a safe default.
♻️ Optional: Use int.CompareTo for brevity
The
Comparehelper (lines 183-196) could be simplified usingint.CompareTo:private static int Compare(int left, int right) { - if (left > right) - { - return 1; - } - - if (left < right) - { - return -1; - } - - return 0; + return left.CompareTo(right); }The explicit logic is clear and readable, so this is purely a style preference.
🤖 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/SimulateMouseUi/UiRaycastHelper.cs` around lines 133 - 196, The Compare helper implements numeric comparison manually; simplify by replacing usages in CompareRaycastPriority and removing the custom Compare method—use left.CompareTo(right) (or right.CompareTo left appropriately) directly where Compare(...) is called (references: CompareRaycastPriority and the Compare method) to make the code more concise while preserving behavior.
🤖 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.
Nitpick comments:
In `@Packages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs`:
- Around line 133-196: The Compare helper implements numeric comparison
manually; simplify by replacing usages in CompareRaycastPriority and removing
the custom Compare method—use left.CompareTo(right) (or right.CompareTo left
appropriately) directly where Compare(...) is called (references:
CompareRaycastPriority and the Compare method) to make the code more concise
while preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed632951-4170-4a9f-ba2d-d2bb5d4e1936
📒 Files selected for processing (2)
Assets/Tests/PlayMode/SimulateMouseUiTests.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Assets/Tests/PlayMode/SimulateMouseUiTests.cs
|
Thanks again for this PR and the investigation behind it! Quick heads-up in case you watch this thread more closely than the issue: I've posted my findings on #1317 (including what I could and couldn't reproduce on 2022.3.62f3) and opened #1324 with an alternative minimal fix for the failure mode I could reproduce. There's a |
|
I added a minimal Unity repro project and posted the verification details on #1317: Repro project: https://github.com/prog-k-dev/uloop-raycast-repro Short version: with the saved repro scene, #1324 still reports no UI element hit for the front overlay button, while this PR's current commit ( |
Align the contributor PR with the verified issue 1317 repro coverage by replacing unstable Physics2D assumptions, adding the test-project Physics2D module dependency, and preserving the package manifest without Physics2D dependencies.
|
Thanks again for the PR and the follow-up repro details. Your broader priority-comparison approach was the right direction for this issue. I pushed a maintainer follow-up commit on top of this PR to keep that approach while tightening the regression coverage:
The package itself still does not depend on Physics2D. |
|
One more thing: could you please try the updated PR branch again against your minimal repro project and, if possible, the original failing setup? I verified the updated branch locally on this repository with:
All of those passed on my side, but your repro is the source of truth for the original issue. I'd appreciate it if you could confirm whether the front overlay button now receives the click in your environment too. |
|
Thanks for the follow-up commit. I retested the updated PR branch ( Minimal repro project:
Result: The repro button click count changed from I also retested the original downstream failing setup by temporarily pointing that project to the same updated PR branch. In that setup, a lower-priority full-screen UI mask is present behind a front ScreenSpace-Overlay button while a guided overlay is active. Result there:
So the updated PR branch fixes both the saved minimal UI-vs-UI repro and the original downstream scenario in my environment. |
Avoid fetching fork PR head branches from the base repository when the TypeScript server bundle workflow runs. Same-repository PRs can still auto-commit rebuilt bundles, while fork PRs now verify that generated bundle files are already committed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Assets/Tests/PlayMode/SimulateMouseUiTests.cs (1)
859-859: 💤 Low valueConsider using nullable Camera type.
The
eventCameraproperty returnsnullbut uses the null-forgiving operatornull!with a non-nullableCamerareturn type. This suppresses the nullability warning but is inconsistent with the actual return value.♻️ Cleaner nullability annotation
- public override Camera eventCamera => null!; + public override Camera? eventCamera => null;🤖 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/PlayMode/SimulateMouseUiTests.cs` at line 859, The eventCamera property override declares a non-nullable Camera return type while returning null with the null-forgiving operator null!, which is inconsistent and masks the actual nullability. Change the return type of the eventCamera property from Camera to Camera? (nullable Camera type) to properly reflect that it returns null, which removes the need for the null-forgiving operator and makes the code cleaner and more consistent with its actual behavior.
🤖 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.
Nitpick comments:
In `@Assets/Tests/PlayMode/SimulateMouseUiTests.cs`:
- Line 859: The eventCamera property override declares a non-nullable Camera
return type while returning null with the null-forgiving operator null!, which
is inconsistent and masks the actual nullability. Change the return type of the
eventCamera property from Camera to Camera? (nullable Camera type) to properly
reflect that it returns null, which removes the need for the null-forgiving
operator and makes the code cleaner and more consistent with its actual
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73abf90d-94a3-4be1-8345-da5d63f5f31e
📒 Files selected for processing (6)
.github/workflows/build-and-test.ymlAssets/Tests/PlayMode/SimulateMouseUiTests.csPackages/manifest.jsonPackages/packages-lock.jsonPackages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiTool.csPackages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs
💤 Files with no reviewable changes (1)
- Packages/src/Editor/Api/McpTools/SimulateMouseUi/SimulateMouseUiTool.cs
✅ Files skipped from review due to trivial changes (1)
- Packages/manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
- Packages/src/Editor/Api/McpTools/SimulateMouseUi/UiRaycastHelper.cs
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/build-and-test.yml">
<violation number="1" location=".github/workflows/build-and-test.yml:132">
P1: Fork-PR artifact verification can miss untracked bundle files: `git diff --quiet` only compares tracked files and will succeed if a previously-tracked bundle file was removed from git and regenerated by the build as an untracked file. Add a tracked-file check (e.g., `git ls-files --error-unmatch`) before `git diff --quiet` to ensure the paths are actually in git history.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| run: | | ||
| echo "Fork PR detected; skipping automatic bundle commit." | ||
| echo "Verifying built server bundle matches committed files..." | ||
| if git diff --quiet -- Packages/src/TypeScriptServer~/dist/server.bundle.js Packages/src/TypeScriptServer~/dist/server.bundle.js.map; then |
There was a problem hiding this comment.
P1: Fork-PR artifact verification can miss untracked bundle files: git diff --quiet only compares tracked files and will succeed if a previously-tracked bundle file was removed from git and regenerated by the build as an untracked file. Add a tracked-file check (e.g., git ls-files --error-unmatch) before git diff --quiet to ensure the paths are actually in git history.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/build-and-test.yml, line 132:
<comment>Fork-PR artifact verification can miss untracked bundle files: `git diff --quiet` only compares tracked files and will succeed if a previously-tracked bundle file was removed from git and regenerated by the build as an untracked file. Add a tracked-file check (e.g., `git ls-files --error-unmatch`) before `git diff --quiet` to ensure the paths are actually in git history.</comment>
<file context>
@@ -123,6 +123,20 @@ jobs:
+ run: |
+ echo "Fork PR detected; skipping automatic bundle commit."
+ echo "Verifying built server bundle matches committed files..."
+ if git diff --quiet -- Packages/src/TypeScriptServer~/dist/server.bundle.js Packages/src/TypeScriptServer~/dist/server.bundle.js.map; then
+ echo "Built server bundle is already committed."
+ else
</file context>
| if git diff --quiet -- Packages/src/TypeScriptServer~/dist/server.bundle.js Packages/src/TypeScriptServer~/dist/server.bundle.js.map; then | |
| # Ensure files are tracked before diffing; untracked files bypass git diff | |
| for f in Packages/src/TypeScriptServer~/dist/server.bundle.js Packages/src/TypeScriptServer~/dist/server.bundle.js.map; do | |
| if ! git ls-files --error-unmatch "$f" >/dev/null 2>&1; then | |
| echo "File is not tracked in git: $f" | |
| echo "Please commit the built server bundle files." | |
| exit 1 | |
| fi | |
| done | |
| if git diff --quiet -- Packages/src/TypeScriptServer~/dist/server.bundle.js Packages/src/TypeScriptServer~/dist/server.bundle.js.map; then |
Summary
Fixes #1317.
This updates
simulate-mouse-uiraycast selection so it better matches real user input priority when mixed raycasters or multiple ScreenSpace-Overlay canvases overlap at the same screen position.Previously,
simulate-mouse-uiused the firstEventSystem.RaycastAllresult directly. In some scenes that result can be a lower-priority target, such as:That could cause
simulate-mouse-uito dispatch to the wrong target or report that no clickable UI handler was hit even though manual user input reaches the frontmost button.Changes
EventSystem.RaycastAllresult.GraphicRaycaster.RaycastResultcandidates with module, sorting layer/order, and depth.Testing
EventSystem.RaycastAllfirst returned a Physics2D collider, whilesimulate-mouse-uiclicked the visible ScreenSpace-Overlay button.simulate-mouse-uiclicked the higher-order modal button and the flow completed.Summary
Problem
The
simulate-mouse-uitool was selecting raycast targets based on the first result fromEventSystem.RaycastAll(), which doesn't accurately reflect UI visual priority when multiple raycasters overlap. This caused clicks to be dispatched to lower-priority targets such as Physics2D colliders behind visible overlay UI buttons, or lower-sorting-order full-screen UI masks positioned between gameplay and higher-priority modal buttons.Solution
Implemented priority-aware raycast selection that compares candidate hits using UI priority factors (raycaster module priority, canvas sorting layer/order, and graphic depth) before selecting the target. The fix reuses canvas-space hit testing and prefers GraphicRaycaster-based UI results over non-UI raycaster results.
Changes
Core Logic (
UiRaycastHelper.cs)RaycastUI()to compute canvas-space hits before EventSystem results and apply intelligent preference logic viaShouldPreferCanvasSpaceHit()RaycastCanvasSpace()to build fully populatedRaycastResultcandidates with complete metadata (module, sorting layer/order, graphic depth) instead of partial hitsCompareRaycastPriority) that orders hits by:Integration (
SimulateMouseUiTool.cs)UpdatePointerRaycast()to delegate raycast selection toUiRaycastHelper.RaycastUI()instead of directly using EventSystem results, removing prior first-result selection logicTest Coverage (
SimulateMouseUiTests.cs)Click_Should_PreferClippedOverlayUiOverNonUiRaycastHit(): Clipped ScreenSpace-Overlay UI preferred over non-UI raycaster hitClick_Should_PreferOverlayUiOverPhysics2DRaycastHit(): Overlay UI correctly selected even when Physics2D collider appears first in raycast orderingClick_Should_PreferClippedHigherOrderOverlayUiOverLowerGraphicRaycasterHit(): Higher-order overlay UI preferred over lower-priority GraphicRaycaster hitAlwaysHitRaycaster: Simulates non-UI raycaster (e.g., PhysicsRaycaster) without requiring physics modulesAlwaysHitGraphicRaycaster: Lower-priority GraphicRaycaster for testing canvas-order scenariosHighPriorityPhysics2DRaycaster: Physics2D raycaster with maximum sort/render priority for testing priority comparisonDependencies (
Packages/manifest.json)com.unity.modules.physics2d(1.0.0) to support Physics2D test scenariosCI/CD (
build-and-test.yml)dist/server.bundle.js) are committed by fork contributors, preventing workflow failures on missing buildsVerification
The fix has been validated against both original failure scenarios described in issue
#1317:Backward compatibility maintained by limiting Physics2D module dependency to test project only, not the main package manifest.