Add multi-angle coverage and camera control for editor_screenshot#6
Merged
Conversation
The AI can now direct the editor camera like a photographer: - coverage=true returns two reference shots (establishing perspective + orthographic top-down) plus AABB geometry metadata (center, size, longest ground axis). The establishing angle is computed from the target bounding box to face the longest horizontal dimension. - elevation/azimuth/fov parameters give the AI explicit camera control for targeted follow-up shots: closeups, back views, telephoto details. The AI iterates until it has the coverage it needs (up to ~15 shots). - AABB metadata is included in ALL view_target responses (not just coverage), so the AI always has geometry info for planning shots. - Orthographic top-down uses RenderingServer.camera_set_orthogonal for a true plan view. Camera transform and projection are saved/restored after each shot. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Required for the screenshot coverage PR to compile and pass CI: - connection.gd + transport/websocket.py: increase WebSocket buffer to 4 MB for large screenshot base64 payloads (default 64 KB was too small) - plugin.gd: register take_screenshot, clear_logs, get_performance_monitors dispatchers; pass Connection to EditorHandler - log_buffer.gd: add clear() method for clear_logs handler - project_handler.gd + handlers/project.py + tools/project.py: run_project and stop_project tools - Rename 'MCP Studio' to 'Godot AI' in doc comments - test_project.gd: new run_project validation tests - test_scene.gd: relax MeshInstance3D count assertion (scene has more now) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the editor_screenshot MCP tool to support multi-angle “coverage” screenshots and explicit camera controls (elevation/azimuth/fov), returning geometry (AABB) metadata to enable iterative shot planning workflows across the Python server and Godot plugin.
Changes:
- Add
editor_screenshotMCP tool + shared handler support forcoverage,view_target, and camera parameters. - Implement plugin-side screenshot framing (including orthographic top-down) and AABB metadata emission for
view_targetscreenshots. - Expand unit/integration + Godot-side tests to cover screenshots, log clearing, project run/stop, and performance monitor retrieval.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/godot_ai/tools/editor.py |
Adds the editor_screenshot MCP tool and documents new parameters/workflow. |
src/godot_ai/handlers/editor.py |
Implements Python-side response shaping (single vs. coverage multi-image) and MCP content blocks. |
plugin/addons/godot_ai/handlers/editor_handler.gd |
Adds screenshot framing logic, coverage presets, orthographic top-down, AABB metadata, monitors mapping, and log clearing. |
tests/unit/test_runtime_handlers.py |
Extends StubClient to emulate take_screenshot responses and adds unit tests for new handlers. |
tests/integration/test_mcp_tools.py |
Adds integration coverage for logs_clear, project_run/stop, and editor_screenshot tool round-trips. |
test_project/tests/test_editor.gd |
Adds editor-side GDScript tests for screenshot behavior (coverage, view_target, angles, fov) and logs clear. |
test_project/main.tscn |
Updates test scene content/groups for view_target/group-target testing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The earlier commit included interactive test artifacts (SnowGroup, _McpTest* nodes) that referenced snowman.tscn, which isn't committed. In CI this caused main.tscn to fail loading, cascading to ~55 test failures with EDITOR_NOT_READY. The new screenshot coverage tests don't require any specific scene structure beyond a Node3D, so the original simple scene suffices. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Restores SnowGroup as a multi-subject target for screenshot coverage testing and benchmarking. Includes snowman.tscn (reusable snowman scene) so main.tscn has no unresolved external references. Clean version — no _McpTest orphan nodes that would pollute tree/find tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The coverage test previously picked the first Node3D child (Camera3D), which has no visible geometry. The orthographic top-down shot ended up empty in headless CI rendering and was silently skipped, making the image count != 2. Target SnowGroup which has actual meshes to render. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In headless CI, force_draw(false) produces no rendered output and
get_image() returns an empty image. The single-shot path already
errors with 'Framed viewport rendered an empty image', so all
non-coverage screenshot tests skip cleanly via the result.has('data')
gate.
The coverage path was returning {'data': {'images': []}} instead of
erroring, so its test saw 'data' and asserted 2 images, failing.
Make coverage consistent: return INTERNAL_ERROR when no shots render.
Verified locally: 179/179 tests pass in headless Godot.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Clamp resize dimensions to 1px min in _finalize_image to avoid zero-dim crash on extreme aspect ratios at small max_resolution - Fix stale comments in _compute_coverage_angles and StubClient that referenced a removed complementary/3-image preset - Correct editor_screenshot docstring: clarify that the viewport camera is temporarily adjusted and restored (not a separate camera) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dsarno
force-pushed
the
screenshot-coverage-angles
branch
from
2114e97 to
357b7a7
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced May 3, 2026
Closed
dsarno
added a commit
that referenced
this pull request
May 5, 2026
… loop, YAGNI (#370) * Drop SessionRegistry threading.RLock — single asyncio loop, YAGNI Closes #350 (Audit v2 finding #6). The RLock provided no asyncio-level mutual exclusion (it wasn't awaited) and the only callers run inside the single asyncio WS handler — single event loop, no thread crossings. It misled readers into assuming thread-safety the registry doesn't deliver. If a future caller ever runs from another thread, asyncio.Lock is the right answer for this async-first code, not threading.RLock — so the existing lock wasn't even the future-proof choice. Removed: - `from threading import RLock` - `self._lock = RLock()` - every `with self._lock:` block (10 sites) Renamed `_find_matching_session_locked` → `_find_matching_session` since the "_locked" suffix is now misleading. Updated the wait_for_session docstring's "registry lock" reference. Added a class docstring on SessionRegistry making the asyncio-only invariant explicit and naming asyncio.Lock as the future-proof choice should a thread crossing ever appear. Tests: TestSessionRegistryNoThreadingLock pins the deletion against accidental re-introduction (no `_lock` attribute, no `threading` / `RLock` imported in the module). All 28 test_session_registry.py tests plus the full 769-test pytest suite pass. Live smoke: GDScript test_run via MCP against a real Godot 4.6.2 editor — 1157 passed, 0 failed (registry exercised end-to-end via session_manage list, editor_state, and every test that round-trips through the WS handler). Refs umbrella #343. https://claude.ai/code/session_optimistic-tesla-84obZ * Trim docstrings + drop narrating comment per /simplify - SessionRegistry class docstring: trim 5 lines re-litigating the removed RLock down to 3 lines stating the asyncio-only invariant and the forward-looking choice (asyncio.Lock). - TestSessionRegistryNoThreadingLock: collapse the 7-line docstring into one line — test names already convey the invariant. - Drop "# Notify any waiters blocked on wait_for_session()" — the loop body, the to_notify name, and future.set_result(session) already say this. Per CLAUDE.md, avoid comments narrating WHAT. https://claude.ai/code/session_optimistic-tesla-84obZ * Drop prescriptive cross-thread advice from docstrings (Copilot review) asyncio.Lock is a cooperative-scheduling primitive within a single event loop — it does NOT make cross-thread access safe. Recommending it as the future-proof choice for a hypothetical thread crossing was misleading and would steer the next maintainer toward the wrong fix. Drop the "use asyncio.Lock" prescription entirely. Class docstring now states only the current invariant (single asyncio loop, no locking). Test docstring trimmed to neutral framing. If a thread crossing is ever genuinely introduced, the right answer is to marshal through the event loop (asyncio.run_coroutine_threadsafe / loop.call_soon_threadsafe), not to add a lock — but that decision belongs to whoever has the actual cross-thread requirement, not to this docstring. https://claude.ai/code/session_optimistic-tesla-84obZ * Cover the done-waiter skip branch in register() (Codecov patch) The if future.done(): continue branch in register() was uncovered; Codecov flagged it as the one missing line in the patch. Added a regression test that pre-completes a future, sticks it in _session_waiters, and verifies the next register() doesn't re-resolve it. This pins the "don't double-resolve out-of-band-completed waiters" invariant that was previously untested. https://claude.ai/code/session_optimistic-tesla-84obZ --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced May 5, 2026
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
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
coverage=truereturns geometry-informed reference shots (establishing + orthographic top-down) plus AABB metadata so the AI can plan follow-up shots like a photographerelevation/azimuth/fovparameters give the AI explicit camera control for closeups, back views, and telephoto details — iterate until coverage is sufficient (up to ~15 shots)view_targetresponses, not just coverageRenderingServer.camera_set_orthogonalfor a true plan viewStill to polish
shotsbatch parameter to reduce round trips for multi-shot workflowsTest plan
ruff check— lint cleanpytest -v— 267 Python tests passrun_tests suite=editor— 25 GDScript tests passcoverage=truereturns 2 reference shots + AABB metadataelevation/azimuth/fovproduce targeted follow-up shotsview_targetframing unchangedcoverage=truewithoutview_targetsilently ignored🤖 Generated with Claude Code