Add screenshot coordinate GameObject picker#1199
Open
Alex-Ma0 wants to merge 2 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a Changespick_gameobject_from_image: screenshot-to-object picking
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Client,ManageScene: Screenshot + pickView capture
Client->>ManageScene: capture_screenshot(camera / scene_view / positioned)
ManageScene->>BuildPickView: camera, viewportW, viewportH
BuildPickView-->>ManageScene: pickView (projection, transform, captureSource)
ManageScene-->>Client: screenshot + pickView metadata
end
rect rgba(144, 238, 144, 0.5)
Note over Client,PickGameObjectFromImage: Pick GameObject from image coordinate
Client->>pick_gameobject_from_image: image_x, image_y, image_w, image_h, pick_view
pick_gameobject_from_image->>PickGameObjectFromImage: validated params
PickGameObjectFromImage->>CameraResolver: resolve or construct from pickView
CameraResolver-->>PickGameObjectFromImage: Camera instance
alt captureSource == scene_view
PickGameObjectFromImage->>HandleUtility.PickGameObject: ray from viewport UV
HandleUtility.PickGameObject-->>PickGameObjectFromImage: GameObject or null
PickGameObjectFromImage->>MeshIntersection: fallback if not found
else dimension == 3d
PickGameObjectFromImage->>Physics.RaycastAll: ray, layerMask, trigger interaction
else dimension == 2d
PickGameObjectFromImage->>Physics2D.GetRayIntersectionAll: ray, layerMask
end
PickGameObjectFromImage-->>pick_gameobject_from_image: hit/no-hit payload
pick_gameobject_from_image-->>Client: result dict
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 777-802: The BuildPickView method needs to include the camera's
culling mask in the returned dictionary so the downstream picker respects which
layers were actually rendered. Add a new dictionary entry in the BuildPickView
method that captures camera.cullingMask (use a key like "cullingMask") alongside
the existing camera properties like projection, fieldOfView, and clipping
planes. Then ensure the picker implementation uses this cullingMask value as the
default layer mask when performing raycast hit tests, preventing colliders on
invisible layers from being selected.
In `@MCPForUnity/Editor/Tools/PickGameObjectFromImage.cs`:
- Around line 275-307: The issue is that HandleUtility.PickGameObject uses the
current active SceneView camera instead of the serialized pickView camera that
was used to capture the screenshot. If the user pans or resizes the Scene View
after capture, it may pick an incorrect object. Before calling
HandleUtility.PickGameObject, verify that the live Scene View camera position,
rotation, and viewport dimensions still match the pickView camera and viewport
from when the screenshot was taken. If they don't match, skip the HandleUtility
path and use only the ray-based or mesh-based picking fallback to ensure correct
object selection.
- Around line 362-365: When a SkinnedMeshRenderer is detected in the renderer
type check, the current code uses sharedMesh which returns the undeformed
bind-pose geometry. To account for animation deformation before ray-testing,
call BakeMesh() on the skinnedRenderer to capture the current deformed state and
assign the baked mesh result to the mesh variable instead of using sharedMesh
directly. This ensures TryIntersectRayMesh will test against the actual animated
pose rather than the static bind-pose geometry.
In `@Server/src/cli/commands/camera.py`:
- Line 645: The run_command function expects arguments in the order:
command_type, params, config. Line 645 correctly uses this order with
run_command("pick_gameobject_from_image", params, config). However, most other
run_command calls throughout camera.py have reversed the argument order, passing
config first followed by the command_type string and params. Search for all
other run_command calls in the file (the comment identifies 19+ affected calls
including those related to manage_camera operations) and reorder their arguments
to match the correct signature: move the command_type string to the first
position, params to the second, and config to the third, aligning them with the
correct call at line 645 and matching test assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea40709c-93e6-4473-833c-b8e612dde8f3
📒 Files selected for processing (10)
MCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/PickGameObjectFromImage.csMCPForUnity/Editor/Tools/PickGameObjectFromImage.cs.metaServer/src/cli/commands/camera.pyServer/src/services/tools/manage_camera.pyServer/src/services/tools/pick_gameobject_from_image.pyServer/tests/test_cli.pyServer/tests/test_pick_gameobject_from_image.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PickGameObjectFromImageTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PickGameObjectFromImageTests.cs.meta
Comment on lines
+777
to
+802
| private static Dictionary<string, object> BuildPickView( | ||
| Camera camera, | ||
| string captureSource, | ||
| int viewportWidth, | ||
| int viewportHeight) | ||
| { | ||
| if (camera == null) | ||
| return null; | ||
|
|
||
| var euler = camera.transform.eulerAngles; | ||
| var position = camera.transform.position; | ||
| return new Dictionary<string, object> | ||
| { | ||
| { "captureSource", captureSource }, | ||
| { "position", new[] { position.x, position.y, position.z } }, | ||
| { "rotation", new[] { euler.x, euler.y, euler.z } }, | ||
| { "projection", camera.orthographic ? "orthographic" : "perspective" }, | ||
| { "orthographic", camera.orthographic }, | ||
| { "fieldOfView", camera.fieldOfView }, | ||
| { "orthographicSize", camera.orthographicSize }, | ||
| { "nearClipPlane", camera.nearClipPlane }, | ||
| { "farClipPlane", camera.farClipPlane }, | ||
| { "aspect", camera.aspect }, | ||
| { "viewportWidth", viewportWidth }, | ||
| { "viewportHeight", viewportHeight }, | ||
| }; |
Contributor
There was a problem hiding this comment.
Serialize the capture camera’s culling mask in pickView.
pickView currently preserves pose/projection only. Downstream picking falls back to an all-layers raycast when layer_mask is omitted, so colliders on layers the screenshot camera never rendered can still win the hit test. Include camera.cullingMask here and have the picker use it as its default mask to keep picks constrained to what was actually visible in the captured image.
🤖 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 `@MCPForUnity/Editor/Tools/ManageScene.cs` around lines 777 - 802, The
BuildPickView method needs to include the camera's culling mask in the returned
dictionary so the downstream picker respects which layers were actually
rendered. Add a new dictionary entry in the BuildPickView method that captures
camera.cullingMask (use a key like "cullingMask") alongside the existing camera
properties like projection, fieldOfView, and clipping planes. Then ensure the
picker implementation uses this cullingMask value as the default layer mask when
performing raycast hit tests, preventing colliders on invisible layers from
being selected.
Author
|
Addressed the CodeRabbit review feedback in 81e74d4:
Validation:
|
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
pick_gameobject_from_imageMCP tool and Unity handler for reverse-picking GameObjects from supported Unity screenshot coordinatespickViewmetadata to explicit camera, positioned, and Scene View screenshotsunity-mcp camera pickCLI command and testsDetails
Tests
python -m pytest tests/test_pick_gameobject_from_image.py tests/test_cli.py::TestCameraCommands -vuv run --extra dev pytest tests/ -q(1242 passed)MCPForUnityTests.Editor.Tools.PickGameObjectFromImageTests(15 total, 14 passed, 0 failed, 1 skipped: Scene View screenshot skipped in batchmode)Summary by CodeRabbit
unity-mcp camera pickto select GameObjects from screenshot pixel coordinates.pickViewmetadata (multiview returnsshotsinstead of a singlepickView) for accurate coordinate mapping.pickViewbehavior.