Add fallback default_transition for travel requests among non-connected states

[Iceball457]

Arbitrary/code-driven animation transitions powered by a state machine are not currently compatible with the complex blending behavior provided by AnimationTree (AnimationPlayer does not accept BlendSpace2d, for example).
AnimationTreeStateMachine is compatible with all the AnimationNode classes to support complex blending, but are not compatible with arbitrary state machines (the only thing missing is smoothing between AnimationNodes).

The latter approach was chosen for being much simpler to implement. It does require the user to add all animations to an AnimationNodeStateMachine, but the user is no longer required to specify n^2 transitions to get reasonable blending between any two states.

Commit bd32776 adds additional functionality to the "default_transition" field that was present but unused in AnimationNodeStateMachinePlayback. It is now located in AnimationNodeStateMachine and used whenever the StateMachine wishes to teleport.
The default value of the AnimationNodeStateMachineTransition as provided by the GUI is identical to the previous teleporting behavior.

Commit cc87b83 is intended as a predecessor to a loop_blend_time field that will allow imperfect animations to loop a little smoother; although being able to restart animations without completely pausing the animator is useful for my projects as well.

Bugsquad edited:

• Closes Add support for AnimationTree state machine teleport blending godot-proposals#11769

[Calinou]

Thanks for opening a pull request 🙂

Feature pull requests should be associated to a feature proposal to justify the need for implementing the feature in Godot core. Please open a proposal on the Godot proposals repository (and link to this pull request in the proposal's body).

[fire]

Please separate your enhancements into different pull requests and attach Godot improvement proposals.

Edited: There is code changes here that is unrelated to Animation Tweaks.

[Iceball457]

I merged godotengine/godot/master into iceball457/godot/animation-tweaks on accident and then reverted it. There are no extraneous differences between iceball457/godot/animation-tweaks and godotengine/godot/master as of commit f418603 ("Merge pull request #102749 from timothyqiu/tree-buttons-offset ... Fix TreeItem button tooltip trigger area offset") at the time this branch was forked.

@fire Let me know if I should merge master back into my branch, I don't know if that could make the code review easier now that my branch is all sloppy. In the meantime, I'll make a feature proposal

[fire]

These code changes have nothing to do with animation.

• core/os
• doc/classes/TileMap
• platform/ios
• scene/gui
• servers/rendering/renderer_rd
• scene/2d/line_2d
• Parts of doc/classes

[Iceball457]

Those changes come from commit d070530. I don't know why changes in 04d7fd2 result in "This merge commit was added into this branch cleanly", but I do see that the sum of the merge and revert show diffs even though the actual files are identical.

What I need to know is if I should close this PR and re-open it with a fresh branch.

Edit: I may also want to split this PR into "Animation Tree State Machine Teleport Blending" and "Animation Player Force Restart Animation", as the two changes aren't strictly mutual.

[TokageItLab]

I still don't know what to do about line 1073. The build system claims that the overloaded operator is invalid or something like that.

Isn't default transition Ref<Resource>? Then it should be p_state_machine->default_transition.is_null() since Ref.ptr can be nullptr but Ref isn't.

[Iceball457]

Although parameters were removed from the user facing API, they are still accessible internally. Every case that previously resulted in a teleport still results in a teleport EXCEPT for when the user requests a non-connected state with the travel command, which is now processed via default_transition. Because the user can no longer force a teleport via travel, there is no need to specify what to do when a teleport is triggered.

That being said, I don't find it hard to imagine users wishing to manually trigger a teleport for one reason or another. It would be possible to allow the user to request a teleport with a teleport(StringName state, bool reset_animation). Users are currently only able to get teleports via start in this commit (not very flexible).

[Iceball457]

@AThousandShips Is the "breaks compat" a tag that I should try and fix? Or is it more of a "wait until the next version number to consider this PR" kinda deal

[AThousandShips]

It's something to discuss, you've changed the parameters of an exposed method which breaks compatibility

[Iceball457]

The parameter would no longer do anything. The removed parameter is p_reset_on_teleport, but the whole point of this PR is preventing the need to teleport inside a travel request.

It could be left as a vestigial parameter without any additional work on my end; however, I think it's best to leave this change for later unless there are other teams who desperately need this change.

[AThousandShips]

It still breaks compatibility and needs to be handled, you will need to register compatibility methods, see here for instructions

[TokageItLab]

In my opinion, p_reset_on_teleport is still needed as commented in #102784 (comment) and compatibility is not broken.

[Iceball457]

To be clear, I did not modify _travel_main, which still needs that parameter to pass to start. I did modify the parameters of travel whose only job is to pass the parameters of the exposed method into the private method. However, in the context of a user-requested travel, the branch of code that reset_on_teleport is checked in cannot be reached. For that reason, the parameter has no effect from the user's perspective.

Edit: I reviewed again, the branch actually can be reached if the user requests a travel while the state machine is sitting in the "/Start" state. This means the parameter is like 99% useless, but not vestigial.

Edit 2: Further inspection reveals that the branch that reset_request_on_teleport used to be checked in now uses p_state_machine->default_transition->is_reset().

[TokageItLab]

For example, when state "N" and "A" are not connected and your current state is "N" (but "N"'s playback is end) with that "A"'s current time is 0.5:

• start("A", true) will move "A" immediately and playback "A" at time 0.0
• start("A", false) will move "A" immediately and playback "A" at time 0.5
• travel("A", true) will move "A" immediately and playback "A" at time 0.0
• travel("A", false) will move "A" immediately and playback "A" at time 0.5
• queue("A") will use default_transition from "N" and playback "A" time is depend on the default_transition's Reset property
• queue_travel("A") will use default_transition from "N" and playback "A" time is depend on the default_transition's Reset property

Since the above 4 are existing behavior, you must keep them.

[TokageItLab]

Well, if the specification that we are currently discussing and coming to terms with, it may not be necessary to distinguish Start and End from other States. Since it is only important whether they are connected or not.

[Iceball457]

Original behavior:

• start("A", true) will move "A" immediately and playback "A" at time 0.0
• start("A", false) will move "A" immediately and playback "A" at time 0.5
• travel("A", true) will move "A" immediately and playback "A" at time 0.0
• travel("A", false) will move "A" immediately and playback "A" at time 0.5
• queue is not implemented
• queue_travel is not implemented

Current behavior:

• start("A", true) will move "A" immediately and playback "A" at time 0.0
• start("A", false) will move "A" immediately and playback "A" at time 0.5
• travel("A", true) will clear the empty path then add "A" to the travel path
• travel("A", false) will clear the empty path then add "A" to the travel path
• queue("A") will add "A" to the end of the travel path
• queue_travel will add "A" to the end of the travel path

The behavior in the latest commit uses the default_transition instead of teleporting whenever no path is found. That was the premise of the PR, after all.
The user has NO access to manual teleports. I was concerned about this a while back, and proposed adding a public teleport(state) to re-introduce this behavior in case it was desired.

It is also possible for me to revert the changes to travel and re-implement the PR as an new public travel_smooth that uses the default transition instead of teleporting.

Edit: Accidentally called start "not-implemented", whoops

[Iceball457]

Well, if the specification that we are currently discussing and coming to terms with, it may not be necessary to distinguish Start and End from other States. Since it is only important whether they are connected or not.

Start and end actually do work properly even without the special case I added for them.

[Iceball457]

The state machine for this example is ["A", "B", "C", "D", "E"] connected in order. Playback is currently looping inside A (or at the end, whatever). All other states are waiting at 0.5.

Since the above 4 are existing behavior, you must keep them.

Proposed behavior for the next commit (regarding compatibility):

start("E", true) will move to "E" immediately and playback "E" at time 0.0
start("E", false) will move to "E" immediately and playback "E" at time 0.5
travel("E", true) will clear the travel path then add ["B", "C", "D", "E"] to the travel path (valid connection found)
travel("E", false) will clear the travel path then add ["B", "C", "D", "E"] to the travel path (valid connection found)
travel_smooth("E") will clear the travel path then add ["B", "C", "D", "E"] to the travel path (valid connection found)
queue("E") will add "E" to the end of the travel path (it will end up using default_transition)
queue_travel("E") will add ["B", "C", "D", "E"] to the end of the travel path (valid connection found)

Now, we wait 10 seconds so that all the machines are at "E".

start("A", true) will move to "A" immediately and playback "A" at time 0.0
start("A", false) will move to "A" immediately and playback "A" at time 0.5
travel("A", true) will move to "A" immediately and playback "A" at time 0.0 (no connection found)
travel("A", false) will move to "A" immediately and playback "A" at time 0.5 (no connection found)
travel_smooth("A") will clear the travel path then add ["A"] to the travel path (no connection found)
queue("A") will add "A" to the end of the travel path
queue_travel("A") will add ["A"] to the end of the travel path (no connection found)

For the final example, let's imagine the player gave us an input while we were in the middle of travel in the first example. We are currently in state B. The travel path is ["C"]

start("A", true) will move to "A" immediately and playback "A" at time 0.0
start("A", false) will move to "A" immediately and playback "A" at time 0.5
travel("A", true) will move to "A" immediately and playback "A" at time 0.0 (no connection found)
travel("A", false) will move to "A" immediately and playback "A" at time 0.5 (no connection found)
travel_smooth("A") will clear the travel path then add ["A"] to the travel path (no connection found)
queue("A") will add "A" to the end of the travel path for a total of ["C", "A"]
queue("E") will add "E" to the end of the travel path for a total of ["C", "E"]
queue_travel("A") will find a path from the last entry in the travel path "C" to the requested "A" (it finds none and just queues "A") for total of ["C", "A"]
queue_travel("E") will find a path from the last entry in the travel path "C" to the requested "E" (it finds ["D", "E"]) for total of ["C", "D", "E"]

[TokageItLab]

Current behavior:
start("A", true) will move "A" immediately and playback "A" at time 0.0
start("A", false) will move "A" immediately and playback "A" at time 0.5
travel("A", true) will clear the empty path then add "A" to the travel path
travel("A", false) will clear the empty path then add "A" to the travel path

travel() should not add “A” to the path. Instead, it should call start(“A”), which is the previous behavior.

It is also possible for me to revert the changes to travel and re-implement the PR as an new public travel_smooth that uses the default transition instead of teleporting.

Also travel_smooth() is not needed, This is because in the same way that we discussed jump() above, it should be almost equivalent to the following:

StateMachinePlayback.clear_path()
StateMachinePlayback.queue_travel("State")

[Iceball457]

travel_smooth() is also not needed, since, like jump, it should be roughly equivalent to

This is true. The name queue_travel wouldn't exactly catch my eye if that's what it were called back when I originally opened this PR, though. Calling two lines of script every time I want to travel with a default transition is ugly.

I may grumble about it but I totally get where you're coming from. You're absolutely certain that exposing fewer members is the higher priority? Currently the only actual difference between all the differently named public members is which flags they set on the internal _travel_main call.

[TokageItLab]

You're absolutely certain that exposing fewer members is the higher priority?

Yes, the addition of APIs should be minimal, and this is Godot's philosophy. If you want to show users how to use path replacement, you can place sample code in the documentation.

[TokageItLab]

So now that we have an agreement on the API, I think you can continue working on it. So please let it be a draft until it is ready for review. Once the implementation is done and it is ready for test/review, please let me know when it works and is ready for review by removing the draft mark.

[Iceball457]

It was pretty simple to replace the existing behavior, but it is proving to be much more difficult to properly swap between both teleporting and jumping. I don't think I will be able to implement this feature the way it was requested with my current skill level

I can roll the branch back to "working, but breaks compat", or push my most recent changes which are "the whole queue system got broken". Either way, I will be taking a break from this PR

[TokageItLab]

Well, if you leave this PR as is, I will try to salvage it when I have time.