Accept scene-absolute target_path in animation presets (closes #328) (#337)

* Accept scene-absolute target_path in animation presets (closes #328)

Animation preset tools (preset_fade/slide/shake/pulse) historically
required target_path to be relative to the AnimationPlayer's root_node,
while every other scene tool takes scene-absolute paths like /Main/Foo.
Agents repeatedly tripped on the inconsistency and got an unhelpful
"Target node not found" error that didn't mention either convention.

_resolve_preset_target now accepts both shapes: scene-absolute paths
(via McpScenePath.resolve) are converted to root_node-relative track
paths via root_node.get_path_to(target). Targets outside the player's
root_node subtree are rejected with a clear error explaining why
animation tracks need that constraint. The relative-path error now
names both supported conventions so callers can self-correct.

Also extracts _player_root_node — the same is_inside_tree → root_node
NodePath → get_parent walk was duplicated three times in the file
(validate_animation, _resolve_track_prop_context, the new resolver).

Coverage: four new GDScript tests (each preset's absolute-path branch,
plus a target-equals-root_node edge case verifying "." track shape and
an outside-subtree rejection). Augmented the existing missing-target
test to lock the improved error message. Live-smoked: scene-absolute
target_path now succeeds end-to-end, animation_manage validate reports
broken_count=0 on the resulting track.

https://claude.ai/code/session_01EduM6GCbywUfRhMQAL8M5N

* Apply Copilot review feedback on preset target_path

Three findings from the PR review:

1. Drop the `is_ancestor_of` strict check on scene-absolute targets. The
   relative form already accepts `..`-prefixed paths via
   `root_node.get_node_or_null`, and Godot's animation engine stores and
   resolves them natively. Rejecting the same node when callers spell it
   absolutely made the new shape strictly less capable than the relative
   form. `get_path_to` yields the right `..`-prefixed track path.

2. The hardcoded `/Main/World/Cube` example in the not-found error is
   misleading in any scene whose root isn't named Main. Use the actual
   `scene_root.name` so the hint always points at a valid path shape.

3. Add `test_preset_slide_accepts_scene_absolute_target` so slide gets
   the same positive abs-path coverage as fade/shake/pulse, and convert
   the old slide rejection test into
   `test_preset_slide_accepts_target_outside_root_node` to lock in the
   new permissive behavior (track stored as `../ForeignTarget:position`).

Also updates `tools/animation.py` module docstring to match the new
behavior.

ruff check src/ tests/: clean.
script/ci-check-gdscript: clean.
pytest: 762 passed.

https://claude.ai/code/session_01DwmN9ouQq88KnabiW42y2C

* Fix slide preset test animation names

The two new slide tests asserted on `slide_in` but `preset_slide`
actually defaults `animation_name` to `slide_<mode>_<direction>`
(see animation_handler.gd:923). Use `slide_in_left` to match.

Caught by CI (Linux/macOS/Windows Godot test jobs).

https://claude.ai/code/session_01DwmN9ouQq88KnabiW42y2C

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: dsarno

plugin/addons/godot_ai/handlers/animation_handler.gd

@@ -588,13 +588,7 @@ func validate_animation(params: Dictionary) -> Dictionary:
 
 	var anim: Animation = player.get_animation(anim_name)
 
-	var root_node: Node = null
-	if player.is_inside_tree():
-		var rn := player.root_node
-		if rn != NodePath():
-			root_node = player.get_node_or_null(rn)
-		if root_node == null:
-			root_node = player.get_parent()
+	var root_node := _player_root_node(player)
 
 	var broken_tracks: Array[Dictionary] = []
 	var valid_count := 0
@@ -810,6 +804,7 @@ func preset_fade(params: Dictionary) -> Dictionary:
 	if target_resolved.has("error"):
 		return target_resolved
 	var target: Node = target_resolved.node
+	var track_target: String = target_resolved.track_path_root
 
 	# Fade requires a `modulate` property (CanvasItem/Control/Node2D/Sprite3D/etc).
 	var has_modulate := false
@@ -839,7 +834,7 @@ func preset_fade(params: Dictionary) -> Dictionary:
 	anim.length = duration
 	anim.loop_mode = Animation.LOOP_NONE
 
-	var track_path := "%s:modulate:a" % target_path
+	var track_path := "%s:modulate:a" % track_target
 	_do_add_property_track(anim, track_path, "linear", [
 		{"time": 0.0, "value": start_a, "transition": "linear"},
 		{"time": duration, "value": end_a, "transition": "linear"},
@@ -905,6 +900,7 @@ func preset_slide(params: Dictionary) -> Dictionary:
 		return target_resolved
 	var target = target_resolved.node
 	var kind: String = target_resolved.kind
+	var track_target: String = target_resolved.track_path_root
 
 	# Default distance picks 3D units vs screen pixels based on target kind.
 	var default_distance: float = 1.0 if kind == "3d" else 100.0
@@ -937,7 +933,7 @@ func preset_slide(params: Dictionary) -> Dictionary:
 	anim.length = duration
 	anim.loop_mode = Animation.LOOP_NONE
 
-	var track_path := "%s:position" % target_path
+	var track_path := "%s:position" % track_target
 	_do_add_property_track(anim, track_path, "linear", [
 		{"time": 0.0, "value": start_pos, "transition": "linear"},
 		{"time": duration, "value": end_pos, "transition": "linear"},
@@ -1001,6 +997,7 @@ func preset_shake(params: Dictionary) -> Dictionary:
 		return target_resolved
 	var target = target_resolved.node
 	var kind: String = target_resolved.kind
+	var track_target: String = target_resolved.track_path_root
 
 	var default_intensity: float = 0.1 if kind == "3d" else 10.0
 	var intensity: float = float(params.get("intensity", default_intensity))
@@ -1048,7 +1045,7 @@ func preset_shake(params: Dictionary) -> Dictionary:
 	anim.length = duration
 	anim.loop_mode = Animation.LOOP_NONE
 
-	var track_path := "%s:position" % target_path
+	var track_path := "%s:position" % track_target
 	_do_add_property_track(anim, track_path, "linear", kfs)
 
 	_commit_animation_add(
@@ -1110,6 +1107,7 @@ func preset_pulse(params: Dictionary) -> Dictionary:
 	if target_resolved.has("error"):
 		return target_resolved
 	var kind: String = target_resolved.kind
+	var track_target: String = target_resolved.track_path_root
 
 	if anim_name.is_empty():
 		anim_name = "pulse"
@@ -1134,7 +1132,7 @@ func preset_pulse(params: Dictionary) -> Dictionary:
 	anim.length = duration
 	anim.loop_mode = Animation.LOOP_NONE
 
-	var track_path := "%s:scale" % target_path
+	var track_path := "%s:scale" % track_target
 	_do_add_property_track(anim, track_path, "linear", [
 		{"time": 0.0, "value": from_vec, "transition": "linear"},
 		{"time": duration * 0.5, "value": to_vec, "transition": "linear"},
@@ -1165,26 +1163,77 @@ func preset_pulse(params: Dictionary) -> Dictionary:
 # Helpers — preset resolution
 # ============================================================================
 
-## Resolve a preset target node relative to the player's animation root and
-## classify its transform kind. Mirrors the same root-node fallback that
-## `_resolve_track_prop_context` uses so tool inputs match how the track path
-## will resolve at playback.
-## Returns {node, kind} where kind ∈ {"control", "2d", "3d"}, or an error dict.
+## Resolve the AnimationPlayer's effective `root_node` — the node animation
+## tracks resolve their paths against at playback. Honors a non-default
+## `root_node` NodePath, then falls back to `player.get_parent()` (Godot's
+## documented default behavior for `root_node = ".."`). Returns null if the
+## player isn't in the tree and has no resolvable parent.
+static func _player_root_node(player: AnimationPlayer) -> Node:
+	if not player.is_inside_tree():
+		return null
+	var rn := player.root_node
+	if rn != NodePath():
+		var resolved := player.get_node_or_null(rn)
+		if resolved != null:
+			return resolved
+	return player.get_parent()
+
+
+## Resolve a preset target node and classify its transform kind.
+##
+## Accepts two `target_path` shapes:
+##   * Scene-absolute (starts with "/") — resolved through `McpScenePath.resolve`,
+##     matching the convention used by every other scene-mutating tool. Targets
+##     outside the player's `root_node` subtree are converted to `..`-prefixed
+##     paths via `root_node.get_path_to(target)`, mirroring what the relative
+##     form accepts and how Godot stores track paths.
+##   * Relative — used as-is against the player's `root_node`, matching how
+##     animation tracks themselves are stored.
+##
+## Returns `{node, kind, track_path_root}` where `track_path_root` is the path
+## (relative to `root_node`) that callers should embed in the track path. For
+## scene-absolute inputs this is the converted relative path; for relative
+## inputs it equals the input. `kind` ∈ {"control", "2d", "3d"}.
+##
+## Mirrors the same root-node fallback that `_resolve_track_prop_context` uses
+## so tool inputs match how the track path will resolve at playback.
 func _resolve_preset_target(player: AnimationPlayer, target_path: String) -> Dictionary:
-	var root_node: Node = null
-	if player.is_inside_tree():
-		var rn := player.root_node
-		if rn != NodePath():
-			root_node = player.get_node_or_null(rn)
-		if root_node == null:
-			root_node = player.get_parent()
+	var root_node := _player_root_node(player)
 	if root_node == null:
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
 			"AnimationPlayer at %s has no resolvable root_node (is the scene open?)" % str(player.get_path()))
-	var target: Node = root_node.get_node_or_null(target_path)
-	if target == null:
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
-			"Target node not found at '%s' (resolved against player's root_node)" % target_path)
+
+	var target: Node = null
+	var track_path_root: String = target_path
+	if target_path.begins_with("/"):
+		var scene_root := EditorInterface.get_edited_scene_root()
+		if scene_root == null:
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
+				"Cannot resolve scene-absolute target_path '%s': no scene open" % target_path)
+		target = McpScenePath.resolve(target_path, scene_root)
+		if target == null:
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
+				McpScenePath.format_node_error(target_path, scene_root))
+		# Convert to a root_node-relative path. For targets outside the
+		# subtree this yields a `..`-prefixed path, matching what the
+		# relative form already accepts (root_node.get_node_or_null
+		# resolves `..` segments) and what Godot's animation engine
+		# stores natively.
+		track_path_root = str(root_node.get_path_to(target))
+	else:
+		target = root_node.get_node_or_null(target_path)
+		if target == null:
+			# root_node.get_path() leaks the editor's SubViewport-wrapped
+			# path; use the clean scene-relative form so the hint is
+			# actionable.
+			var scene_root := EditorInterface.get_edited_scene_root()
+			var root_hint := McpScenePath.from_node(root_node, scene_root) if scene_root != null else str(root_node.name)
+			var abs_example := "/%s/path/to/target" % scene_root.name if scene_root != null else "/SceneRoot/path/to/target"
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
+				("Target node not found at '%s' (resolved relative to AnimationPlayer's root_node '%s'). "
+				+ "Pass a path relative to root_node (e.g. \"path/to/target\") or a scene-absolute path (e.g. \"%s\").")
+				% [target_path, root_hint, abs_example])
+
 	var kind: String
 	if target is Control:
 		kind = "control"
@@ -1195,7 +1244,7 @@ func _resolve_preset_target(player: AnimationPlayer, target_path: String) -> Dic
 	else:
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
 			"Target '%s' must be a Control, Node2D, or Node3D (got %s)" % [target_path, target.get_class()])
-	return {"node": target, "kind": kind}
+	return {"node": target, "kind": kind, "track_path_root": track_path_root}
 
 
 ## Build a directional offset for slide presets.
@@ -1481,13 +1530,7 @@ static func _resolve_track_prop_context(track_path: String, player: AnimationPla
 	var prop_base := prop_full if sub_colon < 0 else prop_full.substr(0, sub_colon)
 	var prop_sub := "" if sub_colon < 0 else prop_full.substr(sub_colon + 1)
 
-	var root_node: Node = override_root_node
-	if root_node == null and player.is_inside_tree():
-		var rn := player.root_node
-		if rn != NodePath():
-			root_node = player.get_node_or_null(rn)
-		if root_node == null:
-			root_node = player.get_parent()
+	var root_node: Node = override_root_node if override_root_node != null else _player_root_node(player)
 	if root_node == null:
 		return {"pass_through": true}
 

src/godot_ai/tools/animation.py

@@ -58,6 +58,13 @@
   • preset_pulse(player_path, target_path, from_scale=1.0, to_scale=1.1,
                   duration=0.4, animation_name="", overwrite=False)
         One-call pulse / hover-bounce (3-keyframe scale ping-pong).
+
+Preset target_path: accepts either a scene-absolute path (e.g. "/Main/World/Cube",
+matching every other scene tool) or a path relative to the AnimationPlayer's
+root_node (e.g. "World/Cube", matching how Animation tracks store node paths).
+Scene-absolute targets outside the player's root_node subtree are converted to
+a `..`-prefixed track path via root_node.get_path_to(target), the same shape
+the relative form already accepts.
 """
 
 
@@ -76,6 +83,10 @@ async def animation_create(
 
         After creating the clip, add tracks via ``animation_manage`` ops
         ``add_property_track`` / ``add_method_track`` / ``create_simple``.
+        Track node paths are stored relative to the AnimationPlayer's
+        ``root_node`` (default: its parent), not to the scene root — see
+        ``animation_manage`` preset ops for a forgiving target_path that
+        accepts either form.
         If ``player_path`` doesn't resolve, an AnimationPlayer is auto-created
         at that path (parent must exist).