Display and allow setting name/index of BlendSpace points

[vaner-org]

Closes #12487, closes #4417, closes #66336.

For ease of review and feedback, this PR's commits are currently separate. I'd like to walk through everything this commit adds, so that any additions deemed extraneous can be removed before the squash.

Allow naming points

Points can now be named, and their name will now hold their reference in code.

This is not a breaking change, all index references remain valid until they are replaced with a name. On first launch, indices are transcribed into the string names, that can then be changed by the user.

Visible index/name

In both BlendSpace editors, the index/name is now visible above the point.

If this text is too close to the left/right/top edge, it automatically moves itself to fit in the visible view.

blendspace-text-fitting.mp4

The point can also be dragged using its text.

Unique names upon creation

As mentioned in #12487, points are now given unique names in the same convention as BlendTree and StateMachine.

Inline editing for names

When a point's text is clicked and released without initiating a drag, its name can be edited inline. If this name is removed, the point reverts to using its index reference. A purely numeric name results in the forced prefixing of '#'. If an empty name is committed, the point reverts to a name based on its node type or animation.

inline-name-editing.mp4

Editing index via toolbar or scrolling directly over points

It is possible to edit the index of a point by incrementing/decrementing/specifying a new index via the toolbar, or scrolling over the points. This makes the BlendSpace editor enter a temporary state where indexes are displayed above points, rather than names. It is possible edit the name here too, and once again, a purely numeric name results in the forced prefixing of '#'.

Consolidating point editing tools to the right side

The toolbar was moved to the right to consolidate all the "point editing" features to one side, to better separate the tools editing the blend space as a whole versus a blend point within it.

However, since the name box can likely be removed entirely thanks to the inline editing, maybe this change not needed. That being said, I do feel it helps AnimationTree usability as a whole, and can relegate it to another PR.

---

The issue this PR closes was one of the first major pain points I had with animation in Godot. However, the scope of this is beyond anything I've attempted before, so feedback is very much appreciated. I will integrate recommendations as soon as I can.

What in my opinion needs the most rigorous testing is undo/redo, it took me a while to get it free of bugs.

[TokageItLab]

I agree that the current point names are just transcribed array indices and are not meaningful.

However, in my opinion, regardless of whether these points have meaningful names, we need to implement a way to control the array indices.

The reason is that triangle creation priority follows the order in which points are stored in the array. Therefore, this PR should not resolve #66336.

I'm still positive about this PR, but adding some kind of GUI that allows users to see the array indices storing points and reorder them (means users should be able to see both the point name and the index of the array) would provide a stronger reason to merge it.

Ah sorry, I realized I can change the index using the spinner. Please disregard the comment above.

Therefore, it should be fine as long as concern about naming restriction is resolved.

From a usability perspective, changing the index via the spinner is not so intuitive. Then I feel it would be helpful to have a function to move the selected point to either the most top or most bottom (this should allow creating a preferred triangle by applying it any three points in cases where symmetrical quadrilateral occur like #66336).

[vaner-org]

From a usability perspective, changing the index via the spinner is not so intuitive. Then I feel it would be helpful to have a function to move the selected point to either the most top or most bottom (this should allow creating a preferred triangle by applying it any three points in cases where symmetrical quadrilateral occur like #66336).

It's possible to type into the index field a number higher than the number of points to clamp it to the top, or a negative number to clamp it to 0. Is this not satisfactory?

[TokageItLab]

@vaner-org That might be sufficient to some extent. However, since there's no way for users to know this within the editor, it should probably be documented somewhere, like in a tooltip or at least in the documentation. Ideally, right-clicking might bring up options including renaming, but if that's too much work, just the documentation for now should suffice (and I think we can send the right-click option as follow up PR later).

[lyuma]

(from rocketchat) I'm not happy about changing from returning a cached StringName name; to calling String get_blend_point_name(int p_point) const; which then must perform itos(i) followed by String -> StringName conversion, at every _process

If the names could be cached, I might feel better about it. Or maybe, for compatibility, we should always store the number as the name, and if the name is numeric, then it always uses an index and ignores the name.

[TokageItLab]

@lyuma That case parameters/blendspace2d/ + get_blend_point_name(0) exists solely for compatibility with niche scenarios where paths depend on dynamically changing points. If you want to cache the path name in cases where it depends on this dynamic path, you would likely need to handle caching on the user side before and after add_point() or remove_point(). Alternatively, adding a signal like points_reordered could be another approach to consider.

However, as I mentioned above, I believe use cases that rely on paths being dynamically changed are niche. So in most normal cases where you're just accessing path parameters/blendspace2d/0 (0 is "name", not "index") shouldn't affect performance. The current BlendSpace dynamically transcribes index into name using itos, so my suggestion in #110369 (comment) for this PR simply always prevents name from being changed automatically. Furthermore, I argue that inconsistent path behavior depend on whether it is number or not should be avoided, as it can cause confusion.

[vaner-org]

Pending further opinions, I do request that this PR not close #66336, because if the important part that indexes play in animation blending isn't exposed and made apparent to the user, a lot of unpredictable things can occur. I know it caused me a great deal of confusion before I understood it.

While blendpoint indexes remain a necessary evil that hold serious influence over animation blending, I don't think they should be swept under the rug.

[vaner-org]

Looks like I'm going to have to add exclusions anyway to pass CI. Is it still worth it to do all of our circumventions to add_blend_point?

[TokageItLab]

If a compatibility method definitely exists, simply add the text file of the expected error to the CI exclusion as godot\misc\extension_api_validation\4.6-stable\GH-110369.txt.

[TokageItLab]

I think now this PR code is fine, I will test and re-review in Animation meeting this weekend. Can you squash commits into one?

[TokageItLab]

I've confirmed that it's working as expected.

I'll add the brakes compat label just in case, as it involves a minor breaking change; As long as disable_deprecated is false, it will issue a warning but won't break the project.

I'll confirm the final agreement later in the animation meeting.

[lyuma]

We discussed this at the animation meeting, but it was difficult to find consensus.

BlendPoint name is StringName, not String. The APIs to set_blend_point_name and get_blend_point_name and find_blend_point_by_name should take StringName.

Tokage notes: The compatibility impact is when the number of points is changed dynamically at runtime, so people who need this can use get_blend_point_name

@fire commented during the meeting that this is not something we need, and changing it from an integer to string and having an editable title might be a bad idea.

I and fire agree it may be helpful to have the index number visible at least.

[TokageItLab]

We discussed this at the animation meeting, but it was difficult to find consensus.

To clarify, the main concern was likely compatibility issues.

In older projects, for example, when a parameter like BlendSpace2D/3/blend_position existed, after deleting a point, its path would be automatically changed to BlendSpace2D/2/blend_position. However, there should be agreement that this "automatic path change behavior is inconvenient."

While scripts based on this automatic path change behavior (though I believe this is a niche case) will break compatibility, I believe the migration path is secure as demonstrated above using get_blend_point_name(). Except for that niche case, no migration is required as long as disable_deprecated is false.

Even without this, it might not prevent you from making games, but since it's inconvenient that we can't assign arbitrary names to nested BlendTrees like BlendSpace2D/NestedBlendSpace2D/blend_position, I suggest to merge this PR.

[Repiteo]

Thanks!