Address retroactive review of PRs #17 and #21#28
Merged
Conversation
Nine follow-ups from a retroactive code review of the animation_* tool family (#17) and the cyberpunk HUD friction fixes (#21). All 388 Python + 345 GDScript tests pass; smoke-tested live against the editor. 1. node_create scene_path now uses PackedScene.GEN_EDIT_STATE_INSTANCE so the editor treats the result as a real scene instance (foldout icon, .tscn stores a reference, not an exploded subtree). Removes the _set_owner_recursive call that was running outside the undo action and also broke the instance link by forcing descendant ownership. 2. Animation track undo no longer caches the baseline track_count at do time - it resolves the index via anim.find_track(path, type) at undo time. Robust to any history interleaving. New test stresses this path. 3. _coerce_value_for_track now errors with INVALID_PARAMS when the target node exists but the property doesnt - previously the raw value passed through and produced garbage keyframes at playback. Matches the handlers own docstring. 4. add_method_track validates keyframe args is an Array and method is a non-empty string. Previously a typed-assign crash or silent garbage. 5. animation_delete now uses _resolve_animation to locate the owning library, matching the read-side symmetry with animation_get / animation_play. Delete from non-default libraries works end-to-end. 6. theme_set_stylebox_flat flattened from 27 parameters to a nested-dict API: border / corners / margins / shadow each take {all, sides} with side-specific keys overriding all. Unknown keys fail loudly. Breaking change - tool was only merged in #21 and has no external clients yet. 7. project_stop replaces the fixed 150ms sleep with a bounded poll on session.readiness. Reaches truth as soon as the existing readiness_changed event arrives (~17ms) instead of blanket-sleeping 150ms, with a 1s timeout for a hung play process. 8. ui_build_layout pseudo-property tests now read back via get_theme_*_override (not the fallback get_theme_* getters), per the CLAUDE.md "assert on the stored Variant" rule. Added missing theme_override_styles/ test. 9. signal_connect distinguishes declared-but-uninstantiated autoloads from genuinely missing nodes, with a clear error pointing users at @onready/runtime-connect as the workaround. Previous "not found" error masked the real reason most runtime-only autoloads cant be connected at edit time. Also updates CLAUDE.md with reference patterns for find_track-at-undo, GEN_EDIT_STATE_INSTANCE, and get_theme_*_override readback tests. Deferred to a follow-up PR: plugin dispatcher coroutine support + readiness Option C (plugin-side await). That refactor touches the hot path every command flows through and deserves its own bisect-friendly landing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CLAUDE.md: 387 → 388 Python tests - implementation-plan.md: 387+333=720 → 388+345=733 total; theme_* blurb now describes the nested border/corners/margins/shadow dicts and animation undo-robust-to-history-interleaving Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. animation_handler.gd: _coerce_value_for_track split into _resolve_track_prop_context (per-track) + _coerce_with_context (per-key). add_property_track resolves the target property type once before the keyframe loop instead of re-walking get_property_list() for every keyframe. Dense clips (100+ keys) avoid 100x the work. Existing API (_coerce_value_for_track) preserved for create_simple, which still resolves per-tween since target/property change per tween. 2. project.py: replace asyncio.get_event_loop() with get_running_loop() (former is deprecated in Python 3.12+ inside async functions). All 388 Python + 345 GDScript tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # plugin/addons/godot_ai/handlers/theme_handler.gd
11 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
Nine follow-ups from a retroactive code review of the
animation_*tool family (#17) and the cyberpunk HUD friction fixes (#21). Review surfaced a mix of confirmed bugs (undo/redo, coercion fall-through), test gaps (pseudo-property readback, interleaved-track undo), and brittle design choices (baseline-index undo, stylebox parameter sprawl).Bug fixes
node_create scene_pathnow usesGEN_EDIT_STATE_INSTANCEso the editor treats the result as a real scene instance. Removes the_set_owner_recursivethat was running outside the undo action and also broke the instance link.baseline = track_countat do time. Resolves the index viaanim.find_track(path, type)at undo time — robust to history interleaving._coerce_value_for_tracknow errors when the target node exists but the property doesn't. Previously the raw value silently became a garbage keyframe.argsmust be an Array;methodmust be non-empty. No more typed-assign crashes on bad JSON._resolve_animationso deleting from non-default libraries works (not just the default library).API reshape (breaking)
theme_set_stylebox_flatflattened from 27 parameters to nestedborder/corners/margins/shadowdicts. Each takes{all, <sides>}with side keys overridingall. Unknown keys fail loudly. Tool was only merged in Fix 10 friction points from cyberpunk HUD stress test #21 and has no external clients yet.Architecture
project_stopreadiness: replaces the fixed 150ms sleep with a bounded poll onsession.readiness. Reaches truth as soon as the existingreadiness_changedevent arrives (~17ms) instead of blanket-sleeping. 1s timeout for hung play processes.Test quality
test_ui.gdnow reads back viaget_theme_color_override/get_theme_constant_override/get_theme_font_size_override/get_theme_stylebox_override— not the fallback getters — per the CLAUDE.md "assert on the stored Variant" rule. Added previously-missingtheme_override_styles/test.Diagnostics
signal_connectautoloads: distinguishes declared-but-uninstantiated autoloads from genuinely missing nodes. Clear error pointing users at@onready/runtime-connect as the workaround when the autoload isn't instanced at edit time.CLAUDE.md
New reference patterns for
find_track-at-undo-time,GEN_EDIT_STATE_INSTANCE, andget_theme_*_overridereadback tests.Deferred to follow-up PR
Plugin dispatcher coroutine support + readiness Option C (plugin-side
await get_tree().process_frame). Touches the hot path every MCP command flows through — isolating it into its own PR keeps this one's blast radius on the tools themselves and makes regression bisect trivial. Once that lands, the server-side polling loop inproject_stopcan be replaced with an authoritativereadiness_afterfrom the plugin response.Test plan
ruff check src/ tests/— all cleanpytest -v— 388 passedtest_runon live editor — 345 passed across 15 suitestheme_set_stylebox_flatwithborder={all:1, top:2}→ top=2, others=1animation_add_property_trackwith.:nonexistent_property→ INVALID_PARAMSscene_file_path(GDScript test coverage)🤖 Generated with Claude Code