Skip to content

Commit 9b75b94

Browse files
dsarnoclaude
andauthored
Fix validate_animation subpath track parsing (rfind → find) (#371)
* Fix validate_animation subpath track parsing (rfind → find) validate_animation was extracting the node portion of each track path with rfind(":"), so a subpath track like "Target:modulate:a" (the shape every preset_* produces, plus any "position:y" / "modulate:a" / etc. subpath track) had its property+subpath glued onto the node part. The validator then looked up "Target:modulate" against root_node, found nothing, and reported the track as broken even when the target node was perfectly healthy. Split on the FIRST colon — the node↔property boundary — instead. The rest of the string is property+subpath, which validate_animation doesn't need to inspect (it's only checking that the target node exists). Add a regression test that creates a Sprite2D sibling of the player, attaches a "Target:modulate:a" track, and asserts validate_animation reports valid_count == 1 / broken_count == 0. Surfaced by Copilot review on #367. Filing here so the cherry-pick stays clean and the bug fix carries its own test. https://claude.ai/code/session_011QxzADbf9zHwfZicjzdvCw * Narrow comment claim: only preset_fade produces subpath tracks Per Copilot review on #371: of the four motion presets, only preset_fade emits a subpath track ("Target:modulate:a"). preset_slide / preset_shake / preset_pulse use plain ":position" or ":scale" paths. Tighten the comments in animation_values.gd and test_animation.gd so future readers debugging validator behavior aren't misled. https://claude.ai/code/session_011QxzADbf9zHwfZicjzdvCw * Empty commit to retrigger CI (Godot tests / macOS flake) * Retrigger CI (third attempt; Godot tests / macOS keeps flaking) * Replace regression test with broken_tracks.node_path shape assertion Godot's `get_node_or_null` strips the `:property` suffix of a NodePath natively — so the original `rfind(":")` and the new `find(":")` produce the same valid/broken classification at runtime. The bug Copilot flagged on #371 turns out to only be visible in the broken_tracks[].node_path field for missing-target subpath tracks: rfind surfaces "MissingTarget:modulate" while find correctly surfaces "MissingTarget". The previous test asserted valid_count == 1 on a healthy subpath track, which passed equally with both rfind and find — i.e. it didn't actually prove the fix. Replace it with a missing-target test that asserts the broken_tracks[0].node_path field, which is the real behavior change. The new test also no longer needs a sibling Sprite2D (the missing target intentionally doesn't exist), so the test is smaller and avoids any macOS-specific test-runner sensitivity that surfaced on the prior shape. Comment in animation_values.gd narrowed to match — the fix matters for diagnostic output, not for the broken/valid classification. https://claude.ai/code/session_011QxzADbf9zHwfZicjzdvCw --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 758d821 commit 9b75b94

2 files changed

Lines changed: 41 additions & 1 deletion

File tree

plugin/addons/godot_ai/handlers/animation_values.gd

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,14 @@ func validate_animation(params: Dictionary) -> Dictionary:
183183

184184
for i in anim.get_track_count():
185185
var track_path_str := str(anim.track_get_path(i))
186-
var colon := track_path_str.rfind(":")
186+
# Split on the FIRST colon (node↔property boundary), not the last.
187+
# Godot's get_node_or_null strips the ":property" tail natively, so
188+
# the valid/broken classification is the same either way — but for
189+
# BROKEN tracks the broken_tracks[].node_path field is what callers
190+
# read to diagnose the missing node, and rfind would surface
191+
# "MissingTarget:modulate" instead of "MissingTarget" for subpath
192+
# tracks like the "Target:modulate:a" shape preset_fade emits.
193+
var colon := track_path_str.find(":")
187194
var node_part: String
188195
if colon >= 0:
189196
node_part = track_path_str.substr(0, colon)

test_project/tests/test_animation.gd

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1494,6 +1494,39 @@ func test_validate_animation_not_found() -> void:
14941494
_remove_node(player_path)
14951495

14961496

1497+
## Regression: validate_animation must split track paths on the FIRST colon
1498+
## (node↔property boundary), not the last. For broken subpath tracks
1499+
## (target node missing), the broken_tracks[i].node_path field must be the
1500+
## bare node name — not "MissingTarget:modulate" with the property colons
1501+
## preserved — otherwise the diagnostic misleads agents debugging missing
1502+
## targets. Note: Godot's get_node_or_null strips the ":property" tail
1503+
## natively, so the rfind/find difference is only user-visible in this
1504+
## broken_tracks payload, not in the valid/broken classification itself.
1505+
func test_validate_animation_broken_subpath_reports_clean_node_path() -> void:
1506+
var player_path := _add_player("TestValidateBrokenSubpath")
1507+
if player_path.is_empty():
1508+
skip("Scene not ready — _add_player returned empty path")
1509+
return
1510+
_handler.create_animation({"player_path": player_path, "name": "fade", "length": 0.5})
1511+
_handler.add_property_track({
1512+
"player_path": player_path,
1513+
"animation_name": "fade",
1514+
"track_path": "MissingFadeTarget:modulate:a",
1515+
"keyframes": [
1516+
{"time": 0.0, "value": 0.0},
1517+
{"time": 0.5, "value": 1.0},
1518+
],
1519+
})
1520+
var result := _handler.validate_animation({
1521+
"player_path": player_path, "animation_name": "fade",
1522+
})
1523+
assert_has_key(result, "data")
1524+
assert_eq(result.data.broken_count, 1)
1525+
assert_eq(result.data.broken_tracks[0].node_path, "MissingFadeTarget",
1526+
"broken node_path must be the bare node name — not 'MissingFadeTarget:modulate'")
1527+
_remove_node(player_path)
1528+
1529+
14971530
# ─── animation_preset_* — shared helpers ─────────────────────────────────────
14981531

14991532
## Add a sibling node to the scene_root. The preset tools resolve target_path

0 commit comments

Comments
 (0)