Add theme_* authoring tools and ui_build_layout composer#16
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Theme authoring tools and a declarative UI layout composer to the Godot AI MCP surface, enabling “markup + styling” style UI creation workflows.
Changes:
- Introduces
theme_*MCP tools (create/apply theme + set color/constant/font_size/stylebox_flat). - Adds
ui_build_layoutto build an atomic Control subtree from a nested dict/JSON spec with undo support. - Expands Python + GDScript test coverage for the new UI/theme behaviors.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_runtime_handlers.py | Adds unit coverage for forwarding new UI/theme handler commands. |
| tests/integration/test_mcp_tools.py | Adds MCP integration tests for ui_build_layout and all theme_* tools. |
| test_project/tests/test_ui.gd | Extends Godot-side UI tests to cover build_layout validation + undo. |
| test_project/tests/test_theme.gd | New Godot-side ThemeHandler test suite (create, set slots, apply/clear). |
| src/godot_ai/tools/ui.py | Adds the ui_build_layout MCP tool and JSON coercion for the tree spec. |
| src/godot_ai/tools/theme.py | New MCP tool module for theme_* tools. |
| src/godot_ai/server.py | Registers theme tools and updates the server capability blurb. |
| src/godot_ai/handlers/ui.py | Adds runtime handler ui_build_layout command wiring. |
| src/godot_ai/handlers/theme.py | New runtime handlers for theme authoring/apply commands. |
| plugin/addons/godot_ai/plugin.gd | Registers ThemeHandler and wires new dispatcher commands. |
| plugin/addons/godot_ai/handlers/ui_handler.gd | Implements build_layout subtree builder + coercion helpers. |
| plugin/addons/godot_ai/handlers/theme_handler.gd | Implements Theme creation, theme slot setters with undo, and theme apply/clear. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not node is Control and not node is Window: | ||
| node.free() | ||
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "theme can only be set on Control / Window (got %s)" % node_type) | ||
| node.theme = theme_res |
There was a problem hiding this comment.
build_layout loads theme_res but doesn’t verify it’s actually a Theme resource before assigning node.theme. If the path points to a non-Theme resource, this will fail at runtime. Validate theme_res is Theme (and error otherwise) before assignment.
| node.theme = theme_res | |
| if theme_res == null or not theme_res is Theme: | |
| node.free() | |
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "theme path must point to a Theme resource: %s" % theme_path) | |
| node.theme = theme_res as Theme |
There was a problem hiding this comment.
Fixed in 8b8c793 — added theme_res == null or not theme_res is Theme check before assignment, and cast to Theme on assign. Regression test test_build_layout_rejects_non_theme_resource saves a StandardMaterial3D.tres and verifies it's rejected with INVALID_PARAMS.
Generated by Claude Code
| var coerced := _coerce_for_type(value, prop_type) | ||
| node.set(prop, coerced) | ||
| return null | ||
|
|
||
|
|
||
| static func _coerce_for_type(value: Variant, prop_type: int) -> Variant: | ||
| match prop_type: | ||
| TYPE_COLOR: | ||
| if value is Color: | ||
| return value | ||
| if value is String: | ||
| var a := Color.from_string(value, Color(0, 0, 0, 0)) | ||
| var b := Color.from_string(value, Color(1, 1, 1, 1)) | ||
| if a == b: | ||
| return a | ||
| if value is Dictionary and value.has("r") and value.has("g") and value.has("b"): | ||
| return Color(float(value.r), float(value.g), float(value.b), float(value.get("a", 1.0))) | ||
| TYPE_VECTOR2: | ||
| if value is Vector2: | ||
| return value | ||
| if value is Dictionary and value.has("x") and value.has("y"): | ||
| return Vector2(float(value.x), float(value.y)) | ||
| if value is Array and value.size() == 2: | ||
| return Vector2(float(value[0]), float(value[1])) | ||
| TYPE_VECTOR2I: | ||
| if value is Vector2i: | ||
| return value | ||
| if value is Dictionary and value.has("x") and value.has("y"): | ||
| return Vector2i(int(value.x), int(value.y)) | ||
| if value is Array and value.size() == 2: | ||
| return Vector2i(int(value[0]), int(value[1])) | ||
| TYPE_NODE_PATH: | ||
| if value is NodePath: | ||
| return value | ||
| if value is String: | ||
| return NodePath(value) | ||
| return value |
There was a problem hiding this comment.
_apply_property() always returns success after calling _coerce_for_type, even when coercion fails (e.g., invalid Color string). This means invalid property values can slip through despite the handler claiming it returns an error dict on failure and that specs are validated up-front. Make _coerce_for_type report failure (or add post-set validation) and return INVALID_PARAMS when a value can’t be coerced to the property’s declared type.
| var coerced := _coerce_for_type(value, prop_type) | |
| node.set(prop, coerced) | |
| return null | |
| static func _coerce_for_type(value: Variant, prop_type: int) -> Variant: | |
| match prop_type: | |
| TYPE_COLOR: | |
| if value is Color: | |
| return value | |
| if value is String: | |
| var a := Color.from_string(value, Color(0, 0, 0, 0)) | |
| var b := Color.from_string(value, Color(1, 1, 1, 1)) | |
| if a == b: | |
| return a | |
| if value is Dictionary and value.has("r") and value.has("g") and value.has("b"): | |
| return Color(float(value.r), float(value.g), float(value.b), float(value.get("a", 1.0))) | |
| TYPE_VECTOR2: | |
| if value is Vector2: | |
| return value | |
| if value is Dictionary and value.has("x") and value.has("y"): | |
| return Vector2(float(value.x), float(value.y)) | |
| if value is Array and value.size() == 2: | |
| return Vector2(float(value[0]), float(value[1])) | |
| TYPE_VECTOR2I: | |
| if value is Vector2i: | |
| return value | |
| if value is Dictionary and value.has("x") and value.has("y"): | |
| return Vector2i(int(value.x), int(value.y)) | |
| if value is Array and value.size() == 2: | |
| return Vector2i(int(value[0]), int(value[1])) | |
| TYPE_NODE_PATH: | |
| if value is NodePath: | |
| return value | |
| if value is String: | |
| return NodePath(value) | |
| return value | |
| var coercion := _coerce_for_type(value, prop_type) | |
| if not coercion.ok: | |
| return McpErrorCodes.make( | |
| McpErrorCodes.INVALID_PARAMS, | |
| "Property '%s' on %s expects type %s" % [prop, node.get_class(), type_string(prop_type)] | |
| ) | |
| node.set(prop, coercion.value) | |
| return null | |
| static func _coerce_for_type(value: Variant, prop_type: int) -> Dictionary: | |
| match prop_type: | |
| TYPE_COLOR: | |
| if value is Color: | |
| return {"ok": true, "value": value} | |
| if value is String: | |
| var a := Color.from_string(value, Color(0, 0, 0, 0)) | |
| var b := Color.from_string(value, Color(1, 1, 1, 1)) | |
| if a == b: | |
| return {"ok": true, "value": a} | |
| return {"ok": false} | |
| if value is Dictionary and value.has("r") and value.has("g") and value.has("b"): | |
| return { | |
| "ok": true, | |
| "value": Color(float(value.r), float(value.g), float(value.b), float(value.get("a", 1.0))) | |
| } | |
| return {"ok": false} | |
| TYPE_VECTOR2: | |
| if value is Vector2: | |
| return {"ok": true, "value": value} | |
| if value is Dictionary and value.has("x") and value.has("y"): | |
| return {"ok": true, "value": Vector2(float(value.x), float(value.y))} | |
| if value is Array and value.size() == 2: | |
| return {"ok": true, "value": Vector2(float(value[0]), float(value[1]))} | |
| return {"ok": false} | |
| TYPE_VECTOR2I: | |
| if value is Vector2i: | |
| return {"ok": true, "value": value} | |
| if value is Dictionary and value.has("x") and value.has("y"): | |
| return {"ok": true, "value": Vector2i(int(value.x), int(value.y))} | |
| if value is Array and value.size() == 2: | |
| return {"ok": true, "value": Vector2i(int(value[0]), int(value[1]))} | |
| return {"ok": false} | |
| TYPE_NODE_PATH: | |
| if value is NodePath: | |
| return {"ok": true, "value": value} | |
| if value is String: | |
| return {"ok": true, "value": NodePath(value)} | |
| return {"ok": false} | |
| return {"ok": true, "value": value} |
There was a problem hiding this comment.
Fixed in 8b8c793. _coerce_for_type now returns {"ok": bool, "value": ...}; _apply_property rejects with INVALID_PARAMS (including the expected type via type_string()) when coercion fails. Regression test test_build_layout_rejects_uncoercible_property passes "not a color!!" to Label.modulate and verifies the error.
Generated by Claude Code
| "path": path, | ||
| "overwritten": overwrite and FileAccess.file_exists(path), | ||
| "undoable": false, | ||
| "reason": "File creation is persistent; delete the file manually to revert", |
There was a problem hiding this comment.
overwritten is computed using FileAccess.file_exists(path) after saving, so it will be true whenever overwrite is true (even if the file did not exist beforehand). Track whether the file existed before the save and use that to report overwritten accurately.
There was a problem hiding this comment.
Fixed in 8b8c793. create_theme now captures existed_before := FileAccess.file_exists(path) at the top (before save) and reports that as overwritten. Regression test test_create_theme_overwritten_flag_tracks_pre_save_state verifies: fresh path with overwrite=true reports overwritten=false, second call reports overwritten=true.
Generated by Claude Code
| static func _validate_res_path(path: String, required_suffix: String) -> Variant: | ||
| if path.is_empty(): | ||
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: theme_path") | ||
| if not path.begins_with("res://"): | ||
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res:// (got %s)" % path) |
There was a problem hiding this comment.
_validate_res_path() always reports "Missing required param: theme_path" when path is empty, but it’s used to validate both theme_path and path (e.g. in create_theme). Consider passing the parameter name into the helper (or having separate validators) so error messages reflect the actual missing argument.
There was a problem hiding this comment.
Fixed in 8b8c793. _validate_res_path(path, required_suffix, param_name = "theme_path") now accepts the caller-facing param name and uses it in all three error branches ("missing", "must start with res://", "must end with suffix"). create_theme passes "path"; theme setters keep the default "theme_path". Regression test test_create_theme_missing_path_names_param_correctly asserts the error says "path" (and explicitly not "theme_path").
Generated by Claude Code
| var parsed = parser.call(params.get("value")) | ||
| if parsed == null and params.get("value") != null: | ||
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Invalid %s value: %s" % [kind, params.get("value")]) | ||
|
|
There was a problem hiding this comment.
In _set_scalar, a null value is treated as valid because the "invalid" check only triggers when value != null. For set_color, this can call Theme.set_color(...) with null, which is not a Color and can lead to runtime errors or invalid theme state. Reject null values explicitly (or define a clear-slot semantics and call the corresponding clear_* method).
| var parsed = parser.call(params.get("value")) | |
| if parsed == null and params.get("value") != null: | |
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Invalid %s value: %s" % [kind, params.get("value")]) | |
| var raw_value = params.get("value") | |
| if raw_value == null: | |
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Invalid %s value: null" % kind) | |
| var parsed = parser.call(raw_value) | |
| if parsed == null: | |
| return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Invalid %s value: %s" % [kind, raw_value]) |
There was a problem hiding this comment.
Fixed in 8b8c793. _set_scalar now checks raw_value == null explicitly before calling the parser and returns INVALID_PARAMS with a clear message pointing callers at the eventual clear_* commands. Regression test test_theme_set_color_rejects_null_value covers it. (On the clear-slot semantics: we have clear_color / clear_constant / clear_stylebox internally for undo restoration — exposing them as public theme_clear_* tools is a reasonable follow-up but felt out of scope for this PR.)
Generated by Claude Code
Five fixes from the Copilot reviewer pass:
1. build_layout: validate loaded resource is a Theme before assigning to
node.theme. A non-Theme resource path (e.g. a .tres StandardMaterial3D)
now errors with INVALID_PARAMS instead of silently failing at runtime.
2. build_layout _apply_property: when Color/Vector2/Vector2i/NodePath
coercion fails, return INVALID_PARAMS instead of silently passing the
raw value through to node.set(). _coerce_for_type now returns
{"ok": bool, "value": ...} so failures are explicit.
3. create_theme overwritten: captured pre-save existence flag instead of
computing it post-save (which was always true when overwrite=true).
4. _validate_res_path: accepts a param_name (default "theme_path") so
create_theme reports "Missing required param: path" rather than
"theme_path". Applies the name uniformly across all three validation
branches.
5. _set_scalar: reject null value explicitly with a clear error message.
Previously a null payload slipped past the parser failure check and
reached Theme.set_color(null) / equivalent.
Adds GDScript regression tests for:
- theme: null-value rejection, overwritten-flag accuracy on first vs
second create, param-name-aware missing-path error
- build_layout: non-Theme resource rejection, uncoercible property
rejection
Godot's answer to Unity's UXML+USS in one PR. Themes are Godot's
CSS-analog — a .tres resource holding (class, name) -> value entries
that cascade down a Control subtree when assigned. Combined with a
declarative layout builder, this is the closest the MCP surface gets
to "write markup, see styled UI."
Theme tools (6):
- theme_create — new empty Theme at res://... .tres
- theme_set_color — Label.font_color, Button.font_hover_color, etc.
- theme_set_constant — VBoxContainer.separation, MarginContainer margins
- theme_set_font_size — per-class text size
- theme_set_stylebox_flat — compose StyleBoxFlat (bg, border, corner radius,
content margin, drop shadow, AA) and assign to a
theme slot in one call — the usual 90% case for
Panel / Button / LineEdit styling
- theme_apply — assign a Theme to a Control; empty clears
ui_build_layout:
- Takes a nested dict describing a Control subtree (type + name + properties
+ anchor_preset + theme + children). Validates the whole tree up-front
(all types exist, all properties exist on their owning class, all res://
paths resolve). If anything fails, no node is created.
- Builds everything in memory, then attaches under one undo action — Ctrl+Z
in Godot rolls the entire subtree back in one step. No half-built HUDs.
- Type coercion for Color (hex / dict), Vector2 / Vector2i (dict or 2-tuple),
NodePath (string). Properties pass through for other scalars.
- Accepts stringified JSON for the tree param (MCP clients that stringify
complex args — see JsonCoerced, #11).
Plumbing:
- theme_handler.gd and extended ui_handler.gd on the plugin side, wired
into plugin.gd dispatcher.
- Python handlers + tool wrappers mirror the plugin API, with require_writable
gating and per-call session_id routing.
- server.py registers both tool sets and documents the new ui_* / theme_*
namespaces in the tool-categories blurb.
Tests (+24 Python, +25 GDScript):
- Python unit tests for every handler, including optional-param behavior
(stylebox_flat only forwards fields that were actually supplied).
- Python integration tests exercise the MCP -> plugin path for each tool
plus JsonCoerced handling on ui_build_layout.
- GDScript suites exercise the real editor path for Theme authoring
(create + color from hex/dict/bad input, constant, font_size, stylebox
field composition, apply to Control, clear theme, non-Control rejection)
and build_layout (simple tree, unknown type, non-Node type, missing type,
unknown property, bad parent, anchor_preset + color coercion, anchor_preset
on non-Control rejected, undo restores scene tree).
Captures follow-ups surfaced during theme_* / ui_build_layout work: - theme_set_stylebox_texture (9-slice for pixel-art UI) - theme_set_font / theme_set_icon (custom typography) - ui_set_text convenience - scene_instantiate for reusable button/enemy scenes - animation_player.* as the missing piece for UI juice and combat feel Also checks off the theme_* + ui_* items that just landed.
Five fixes from the Copilot reviewer pass:
1. build_layout: validate loaded resource is a Theme before assigning to
node.theme. A non-Theme resource path (e.g. a .tres StandardMaterial3D)
now errors with INVALID_PARAMS instead of silently failing at runtime.
2. build_layout _apply_property: when Color/Vector2/Vector2i/NodePath
coercion fails, return INVALID_PARAMS instead of silently passing the
raw value through to node.set(). _coerce_for_type now returns
{"ok": bool, "value": ...} so failures are explicit.
3. create_theme overwritten: captured pre-save existence flag instead of
computing it post-save (which was always true when overwrite=true).
4. _validate_res_path: accepts a param_name (default "theme_path") so
create_theme reports "Missing required param: path" rather than
"theme_path". Applies the name uniformly across all three validation
branches.
5. _set_scalar: reject null value explicitly with a clear error message.
Previously a null payload slipped past the parser failure check and
reached Theme.set_color(null) / equivalent.
Adds GDScript regression tests for:
- theme: null-value rejection, overwritten-flag accuracy on first vs
second create, param-name-aware missing-path error
- build_layout: non-Theme resource rejection, uncoercible property
rejection
dsarno
force-pushed
the
claude/theme-and-composer
branch
from
8b8c793 to
ac550eb
Compare
Consolidates the 10 one-shot UI authoring targets the current stack can hit now that ui_set_anchor_preset, ui_build_layout, and the theme_* family are in place — roguelite HUD, pause menu, upgrade draft, game over, settings, dialogue, main menu, inventory grid, tutorial prompt, boss overlay. Makes concrete what the benchmark exit criteria mean in UI terms, and names the gaps that still block polish (animation_player, audio, stylebox_texture, font). Also flips the ui.* entry under What Must Exist Before This Is A Fair Benchmark from [ ] to [~] partial, with a one-line note on what shipped vs. what's still missing.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Stacks on top of #15 (the base branch is
claude/ui-anchor-preset, notmain— GitHub will auto-retarget tomainonce #15 merges). The diff below is only the theme + composer work.Godot's answer to Unity's UXML+USS in one PR.
Themeis Godot's CSS-analog — a.tresresource holding(class, name) → valueentries that cascade down a Control subtree when assigned. Combined with a declarative layout builder, this is the closest the MCP surface gets to "write markup, see styled UI."Summary
Theme tools (6):
theme_create— new empty Theme atres://... .trestheme_set_color— e.g.Label.font_color,Button.font_hover_colortheme_set_constant— e.g.VBoxContainer.separation,MarginContainermarginstheme_set_font_size— per-class text sizetheme_set_stylebox_flat— composeStyleBoxFlat(bg, border, corner radius, content margin, drop shadow, AA) and assign to a theme slot in one call — the 90% case for Panel / Button / LineEdit stylingtheme_apply— assign a Theme to a Control; empty path clearsui_build_layout:type+name+properties+anchor_preset+theme+children.res://paths resolve). If anything fails, no node is created.JsonCoercedfor clients that stringify complex args.Example one-shot pause menu:
{ "type": "Panel", "name": "PauseMenu", "anchor_preset": "full_rect", "theme": "res://ui/themes/game.tres", "children": [{ "type": "VBoxContainer", "anchor_preset": "center", "properties": {"separation": 16}, "children": [ {"type": "Label", "properties": {"text": "Paused"}}, {"type": "Button", "name": "Resume", "properties": {"text": "Resume"}}, {"type": "Button", "name": "Quit", "properties": {"text": "Quit"}} ] }] }Test plan
ruff checkcleanJsonCoercedregression)test_theme.gd— theme create (path validation, overwrite semantics), color (hex/dict/bad input/missing params), constant, font_size, stylebox field composition, apply to Control, clear, non-Control rejectiontest_ui.gdwithbuild_layout— simple tree, unknown/non-Node/missing type, unknown property, bad parent path, anchor_preset + Color coercion, anchor_preset-on-non-Control rejection, undo restores treeGodot tests / {Linux,macOS,Windows}Notes
theme_createis markedundoable: false(with a reason in the response) because file creation is persistent.StyleBoxTexture(9-slice images) would be a natural follow-up but wants its own design pass.font_*slots on a Theme still require aFontresource — that's a separatetheme_set_fontwe can add once the icon/font-resource handling is hashed out.