Allow marking methods as executable by method call tracks in the editor.

[HEX-23]

Resolves proposal#9939. Implements an idea similar to PR#92990, but with a method-based, more explicit and easy-to-externally-disable approach.

Introduction

This PR allows marking individual methods as executable by method call tracks in the editor.

At the base level, this is implemented as an additional bit flag in the flags bitfield of method info. This is language-agnostic and could easily extend to native C++ modules, any scripting language, or GDExtension.

This particular PR also adds an annotation @editor_allow_animation_call to the GDScript, which marks any annotated GDScript function to be executable by method call tracks in the editor. Implementation for C# and GDExtension can be done in follow-up PRs.

Usage

In GDScript, add @editor_allow_animation_call to any function would mark that function as allowed to be executed by method call tracks in the editor. When playing animation that contains function calls, only functions that are explicitly marked as such would be executed in the editor.

This annotation can only be used in a @tool script. If not, an error is generated during compilation.

Demo

For comparison, I use 2 methods: regular is not annotated, allowed is annotated. Both triggers some particle effects, and are called in an animation. The results are as follows:

In Editor	In Game

The code:

@tool

extends Node3D

@onready var regular_mesh:CPUParticles3D = $regular
@onready var allowed_mesh:CPUParticles3D = $allowed

func regular_method():
	print('regular method called')
	if regular_mesh:
		regular_mesh.emitting = true

@editor_allow_animation_call
func allowed_method():
	print('allowed method called')
	if allowed_mesh:
		allowed_mesh.emitting = true

The MRP: anim_method_call.zip

Security Concerns

According to the discussion in PR#92990, the main concern over such feature is that it (or more specifically the implementation of PR#92990) may allow arbitrary code execution simply by opening the project and cannot be easily detected and disabled from the outside. This PR addresses these issues by:

• Using an annotation to explicitly allow specific piece of code to be executed in this way.
• Putting all control in the script source level, allowing easy detection and disabling from outside the editor.

Thus it follows the suggestions in PR#92990 regarding security.

Potential Follow-Ups

Add support for more languages. This should only require a registration-time snippet that flips a certain bit (METHOD_FLAG_EDITOR_ALLOW_ANIMATION_CALL) in the method info (along with some front-end works for scripting languages, ofc).

[TokageItLab]

Since _blend_process() runs every frame, the overhead of checking for flags is too high. At the very least, you should have a flag in TrackCacheMethod indicating whether it can be executed, so that it can be cached in _update_caches().

	struct TrackCacheMethod : public TrackCache {
#ifdef TOOLS_ENABLED
		Vector<StringName> callable_methods;
#endif
		TrackCacheMethod() { type = Animation::TYPE_METHOD; }
	};

I’ll leave it to the gdscript team to decide whether this is acceptable from a security standpoint and whether it’s an appropriate way to collect methods annotated from scripts.

[HEX-23]

Since _blend_process() runs every frame, the overhead of checking for flags is too high. At the very least, you should have a flag in TrackCacheMethod indicating whether it can be executed, so that it can be cached in _update_caches().

It does not actually process each frame. The perf overhead here is quite marginal because:

• Checking logic only runs whenever there is actually a method to call in the current frame, rather than when there is a method track or something.
• The check, for each method, is basically part of an Object::callp() - that script method lookup and ClassDB::get_method happens every time a call is made through method call track. Now we are just adding something that's less than a regular method call (specifically a method lookup) each time a call attempt is to be made.

If we wish to pursue absolute minimal overhead, I can unfold the original callp() so that all runtime overhead is a single bit operation. But this adds another place where the function call logic is manually maintained (yeah what I did here is basically another method lookup, but it is still different from an actual method call). I am not sure if that would be a good idea from a maintenance perspective. So I am providing this info and leaving the decision to you (I would handle it if you think it is better to make a change like this) @TokageItLab .

[TokageItLab]

Retrieving the script_instance and running a while loop is clearly excessive in _blend_process(). If you want to call the same method for five consecutive frames, this would mean running that loop every frame.

If there is a way to retrieve the flag without retrieving reference and running a loop, you could use that method; means having a list of available methods by animation preview on the GDScript side. However, I consider it less than ideal from an architectural standpoint to have GDScript handle this for the sake of animation, as it would result in a tighter coupling between the animation and GDScript.

Otherwise, as mentioned above, you will need to cache the flag into TrackCache. However, if it is difficult to notify the cache of changes made to the object, you may need to choose the former method.

[TokageItLab]

I assume this should be considered first: whether this should be managed on a per-method basis. Additionally, there's the consideration that this annotation is only valid for custom scripts, so if you want to apply it to the built-in functions of the base class, a wrapper function is required everytime.

Another point of concern is that when @tool is disabled, @editor_allow_animation_call becomes meaningless.

For example, if we were to manage this at the class level rather than the method level, one possible implementation might be to allow @tool to take an argument to enable animations, like:

@tool(enum ToolFlag)

enum ToolFlag {
	TOOL_FLAG_NONE = 0,
	TOOL_FLAG_ALLOW_CALL_METHOD_BY_ANIMATION = 1,
}

[HEX-23]

Added a cache. All runtime overhead is now a single random access of LocalVector<bool>, whenever there is a method call attempt.

[HEX-23]

I assume this should be considered first: whether this should be managed on a per-method basis.

I am not sure which one seems better: Marking the whole class is perhaps more convenient and requires less intrusive modifications to method level logic. But method-level approach offers extra flexibility and makes it easier to track down and isolate the function being invoked (especially when something went wrong). I am leaving this to reviewers.

Additionally, there's the consideration that this annotation is only valid for custom scripts, so if you want to apply it to the built-in functions of the base class, a wrapper function is required everytime.

Adding a way to mark built-in functions has its own complexities: when should these registration code be executed? Should there be some sort of dedicated scripts that does this? Or should it be arbitrary and "recommended to be put into an autoload"? And how should script functions be marked as such? The design about execution time of this can be a bit tricky. Marking the whole class could, in a way, circumvent this (by marking the whole script as safe to call in editor, all functions of the node with that script attached can be invoked by method call track in the editor). But, again, this is the aforementioned whole class vs individual method thing. Marking individual methods is more flexible and, imo, more secure because you have to explicitly enable the feature for methods that you intend to call, there's no chance to enable this feature for dangerous functions (like free()) and call it by accident.

Another point of concern is that when @tool is disabled, @editor_allow_animation_call becomes meaningless.

Current implementation issues an error at compile time if @tool is absent and @editor_allow_animation_call is present.

[TokageItLab]

It is necessary to have some method of notification to update the cache when a script is modified during animation. For example, when a script is modified or an object’s Script property is changed, we need to notify the singleton in the AnimationPlayerEditor or AnimationTreeEditor that the cache needs to be updated.

[HEX-23]

It is necessary to have some method of notification to update the cache when a script is modified during animation. For example, when a script is modified or an object’s Script property is changed, we need to notify the singleton in the AnimationPlayerEditor or AnimationTreeEditor that the cache needs to be updated.

Addressed.

There might be multiple animation players / trees in the edited scene, and they do not automatically become the actively edited one even if they are the only anim node in the scene. Hence I did a tree traversal instead of fetching the active player. Such traversal and cache invalidation is limited to at most once per frame (if requested multiple times in a row) and is editor-only, the performance impact should be minimal (w.r.t. the logic required).

[TokageItLab]

they do not automatically become the actively edited

Ah, right. In that case, maybe you should place it in the EditorNode class (since it has similar functions such as the resetting animation during saving), rather than the AnimationPlayerEditor class. Also, you need to find AnimationMixer instead of AnimationPlayer.

[HEX-23]

they do not automatically become the actively edited

Ah, right. In that case, maybe you should place it in the EditorNode class (since it has similar functions such as the resetting animation during saving), rather than the AnimationPlayerEditor class. Also, you need to find AnimationMixer instead of AnimationPlayer.

Addressed. The logic is moved to EditorNode now, and is looking for AnimationMixer rather than AnimationPlayer.

[TokageItLab]

For now, I think it's ready to be discussed as one approach from a coding perspective within the Animation area.

However, as mentioned above, we need to consider whether to manage this on a per-method basis or a per-class basis. I assume you could likely implement this with less code if you manage it on a per-class basis, but we'll need to discuss this from a safety perspective. I'll bring this up as one of the topics for the next Animation meeting.

Separately from animation, it must be reviewed by the gdscript team. While this PR adds new annotations and flags, but as mentioned above, an approach involving class-level management via tools arguments is also a possibility.

[dalexeev]

In my opinion, @editor_allow_animation_call and the entire METHOD_FLAG_EDITOR_ALLOW_ANIMATION_CALL mechanism are redundant. I think we should probably implement the proposal as originally formulated. That is, just add the "Call In Editor" checkbox for keypoints on call method tracks.

From a security perspective, nothing will change. You can already encounter code executing in the editor when opening a project if you have @tool scripts in general. And for animations in particular, you can use property setters in @tool scripts. It would be somewhat inconsistent to have such "protection" for methods but not for properties.

Also, as TokageItLab noted, the METHOD_FLAG_EDITOR_ALLOW_ANIMATION_CALL requirement makes it impossible to use built-in methods. In my opinion, this requirement doesn't solve any important problem, it merely creates unnecessary formality when you need to mark a method as executable from both sides (a checkbox in the editor and an annotation/flag in the code). @tool already implies that you must be careful that executing code in the editor doesn't lead to data loss or other unintended consequences. Note that there is a separate discussion for a per-method @tool annotation.

To be fair, this option has one drawback. The editor likely won't be able to distinguish whether a method is executable in the editor (built-in, belonging to a normal extension class, or @tool script) or not (belonging to a runtime extension class or a non-@tool script). It would be nice to disable the checkbox for methods that can't be executed in the editor.

Finally, I think we should probably highlight executable keypoints on call method tracks with a different color, fill, or shape (similar to how @tool scripts are highlighted). Also, add a doc tooltip description for the "Call In Editor" checkbox.

[HEX-23]

In my opinion, @editor_allow_animation_call and the entire METHOD_FLAG_EDITOR_ALLOW_ANIMATION_CALL mechanism are redundant. I think we should probably implement the proposal as originally formulated. That is, just add the "Call In Editor" checkbox for keypoints on call method tracks.

From a security perspective, nothing will change. You can already encounter code executing in the editor when opening a project if you have @tool scripts in general. And for animations in particular, you can use property setters in @tool scripts. It would be somewhat inconsistent to have such "protection" for methods but not for properties.

Also, as TokageItLab noted, the METHOD_FLAG_EDITOR_ALLOW_ANIMATION_CALL requirement makes it impossible to use built-in methods. ...

From a convenience perspective, using a checkbox (a.k.a. a property of the keyframe) may be an easy way (possibly with a limit that non-tool script methods are never called). But it also means you need to explicitly specify that "I want this keyframe triggered in editor" every time you are keying. Whereas a method based approach allows one to mark any "visual-only" methods as allowed to be executed and forget about it when creating and editing animations.

The practicality of method call tracks is two-fold: for one thing they can be used to trigger gameplay events that are synced to animation, for another they can be used to drive more sophisticated visual logic. The feature being discussed here is clearly focusing on the latter use case. I feel it more natural to implement gameplay and visual logic as separate functions, and automatically allow all visual logics to be played in the editor, whereas blocking any gameplay events that could possibly break things.

TokageItLab explained in PR#92990 the reason why they disagree with the checkbox approach, and think that a code-side, "which functions are allowed" registration is preferred. This PR is also an attempt to do this the code-side way (note that there's no checkbox thing in this PR, it is still a single-place permission, just at a difference place).

We can also make a certain subset of built-in functions callable if using the design in this PR. It's only a matter of registration (we would also need to add compatibility functions since this affects their hashes / alternatively, we may also mask the METHOD_FLAG_EDITOR_ALLOW_ANIMATION_CALL out when computing hashes of methods).

[dalexeev]

But it also means you need to explicitly specify that "I want this keyframe triggered in editor" every time you are keying.

Can't this be solved by copying and pasting keypoints? Also, in my experience, the number of keypoints in call method tracks is usually small compared to property animation tracks.

Alternatively, we could add a checkbox to the call method track, not to individual keypoints. If you want some methods to be called in the editor, but not others, you can create two tracks and check only one of them.

[HEX-23]

Can't this be solved by copying and pasting keypoints?

Yes, technically this can be done. But I still prefer to just mark the functions I need once and for all. Since the definition is a one-time thing, but the invocations are numerous (especially when we have a lot of animations in the project).

Alternatively, we could add a checkbox to the call method track, not to individual keypoints. If you want some methods to be called in the editor, but not others, you can create two tracks and check only one of them.

From a convenience perspective this seems viable to me. Yet some team members do not agree to adopt the checkbox approach, and demand explicit control from the callee side, as in the discussion here. For now I will leave this PR as-is, and see if any further consensus can be reached among devs.