Add AnimationMixer configuration warning on conflicting track types

[mihe]

Resolves godotengine/godot-proposals#14208.

This pull request aims to improve the usability of AnimationTree and AnimationPlayer, by showing a node configuration warning if deterministic is enabled and there are two tracks in the RESET animation that both reset the same property, but using different NodePath specificity, e.g. MyNode:position and MyNode:position:x.

This should hopefully help catch some of the playback/blending issues that arise from mixing bezier tracks and value tracks, which is discouraged in the documentation, as seen here.

Note that this is a more conservative approach of a previous attempt that aimed to emit this warning for conflicts between non-RESET animations as well, but after seeing some questionable false positives we scaled it back to this. We'd love to hear feedback on whether there are more problematic cases we could/should include here.

[TokageItLab]

Since the track conflict occurs in the AnimationLibrary, it should be an AnimationLibrary warning rather than a Node warning. Therefore, similar to the readonly warning, it needs to be displayed as a warning in the AnimationPlayerEditor, not SceneTreeDock.

While it is right that conflicts can occur when two or more AnimationMixers exist in the same scene and modify the same property, the animation played changes dynamically at runtime. Detecting this in the SceneTreeDock would likely be of little practical use.

[mihe]

Since the track conflict occurs in the AnimationLibrary, it should be an AnimationLibrary warning rather than a Node warning. Therefore, similar to the readonly warning, it needs to be displayed as a warning in the AnimationPlayerEditor, not SceneTreeDock.

That would effectively mean showing the warning even for non-deterministic mixers. Are there any concerns about false positives when doing that? When testing this it seemed like much of the playback/blending jank when mixing these types of tracks show up with deterministic mixers in particular.

While it is right that conflicts can occur when two or more AnimationMixers exist in the same scene and modify the same property, the animation played changes dynamically at runtime. Detecting this in the SceneTreeDock would likely be of little practical use.

I'm not sure what this is referring to. This PR does not currently concern itself with multiple AnimationMixers. Did you mean two or more AnimationMixer tracks? If so, then how would moving the warning to AnimationLibrary be any different?

[TokageItLab]

That would effectively mean showing the warning even for non-deterministic mixers. Are there any concerns about false positives when doing that?

Even in non-deterministic cases, when these tracks exist within the same animation, conflicts can cause one track to overwrite the other. This should be flagged with a warning.

As an additional warning, I believe it should be possible to iterate through RESET animations and other animations as a special case, issuing warnings only in deterministic scenarios. Since the AnimationTrackEditor is associated with the Scene's AnimationMixer (in the case of AnimationTree, it looks at the original AnimationMixer when generating a dummy mixer internally), this should be possible.

I'm not sure what this is referring to. This PR does not currently concern itself with multiple AnimationMixers. Did you mean two or more AnimationMixer tracks?

I assume the cases in this issue correspond to this scenario: #80971 #80951

[mihe]

I assume the cases in this issue correspond to this scenario: #80971 #80951

Possibly. More specifically it's this issue that we keep seeing. So it's not necessarily about multiple AnimationMixers being involved.

There's a comment further down in that issue saying this was caused by #49431, which has since been resolved, but having bezier and non-bezier tracks animate the same property but in two different animations is still very much an issue. More so with deterministic AnimationMixers perhaps.

Even in non-deterministic cases, when these tracks exist within the same animation, conflicts can cause one track to overwrite the other. This should be flagged with a warning.

As an additional warning, I believe it should be possible to iterate through RESET animations and other animations as a special case, issuing warnings only in deterministic scenarios. Since the AnimationTrackEditor is associated with the Scene's AnimationMixer (in the case of AnimationTree, it looks at the original AnimationMixer when generating a dummy mixer internally), this should be possible.

The additional warning case that you mention is presumably the more relevant/common one here, since I imagine it would be difficult to end up with both a MyNode:position and MyNode:position:x track within the same animation, at least when keying through the inspector. This additional warning is also what this PR is trying to do already, albeit in the wrong place, if I'm understanding you correctly.

If we were to move this to instead be a warning Button in AnimationTrackEditor, then we run into a bit of a problem. Seeing as how AnimationPlayer leaves deterministic at its default value of false, you would never actually see this additional warning when editing the animations through such an AnimationPlayer, unless you go out of your way to set deterministic to true yourself. But if that AnimationPlayer is only ever used with an AnimationTree then you would have no reason to do that, since AnimationTree (from what I understand) effectively overrides deterministic with its own value, which is true by default.

With this in mind, would you say it makes more sense to keep this as a node configuration warning? If not, and you still want it to be in AnimationTrackEditor, would it be reasonable to show a warning if tracks from two separate animations conflict as well, regardless of any deterministic value, or would that produce too many false positives?

[TokageItLab]

I assume there is space to add warnings to the AnimationTreeEditor as well, so you can add the warning about Deterministic there.

Additionally, I think it would be clearer to compare the AnimationPlayer assigned to the AnimationTree with the Deterministic options and, if they differ, display a warning “Deterministic options are not matched between AnimationTree and assigned AnimationPlayer, the AnimationTree setting is prioritized.".

In any case, based on my experience, node configration warnings in the TreeDock typically indicate that the node itself is non-functional due to parent-child relationships or missing resources (I'm not sure if other cases exist). Therefore, I consider it inappropriate to display warnings about Deterministic in TreeDock.

[mihe]

I assume there is space to add warnings to the AnimationTreeEditor as well, so you can add the warning about Deterministic there.

Additionally, I think it would be clearer to compare the AnimationPlayer assigned to the AnimationTree with the Deterministic options and, if they differ, display a warning “Deterministic options are not matched between AnimationTree and assigned AnimationPlayer, the AnimationTree setting is prioritized.".

Alright, I'll go ahead and try that approach.

In any case, based on my experience, node configration warnings in the TreeDock typically indicate that the node itself is non-functional due to parent-child relationships or missing resources (I'm not sure if other cases exist). Therefore, I consider it inappropriate to display warnings about Deterministic in TreeDock.

I wouldn't say node configuration warnings are limited to just those categories. The only real common theme I've seen is that they involve some property or relationship of the node. So in this case it could maybe be seen as a validation of the deterministic property, but since we're talking about showing this warning even when disregarding deterministic then I guess the AnimationPlayer/AnimationTree editor is the more appropriate place to show this.

[TokageItLab]

Deterministic options are not matched between AnimationTree and assigned AnimationPlayer, the AnimationTree setting is prioritized.

Regarding this, it may be necessary to check not only the Deterministic option but also matches like CallbackMode options. In any case, if they don't match the original AnimationPlayer, it might be possible to display a list of those mismatched options in a popup, but it could be a bit noisy.