godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.02k stars 21.17k forks source link

Non-numeric Property animation tracks with no RESET track and continuous blending are always Null in a blend tree #79948

Closed MisoShiru closed 4 months ago

MisoShiru commented 1 year ago

Godot version

v4.1.stable.mono.official [970459615]

System information

Windows 10 64-bit

Issue description

This issue is also present in master:202e4b2c1e7f8b25738b93d0e4d5066453d3edf3 which is the changelist I debugged this on.

Repro Conditions In an AnimationTree with an AnimationNodeBlendTree as its root, some properties will always be null if the following conditions are met:

Discovery Brand new Godot user experimenting with a 2d action RPG. I came across this originally in release v4.1.stable.mono.official [970459615]. I was having a different issue with continuous-mode sprite animations interacting with OneShot and decided to try using continuous blending with nearest sampling to see if that would fix my problem. After a few hours of no luck I had to call it quits for the night, but tonight I went ahead and pulled source and debugged this issue because it seemed like it should have been working. Side note - building and debugging the engine was incredibly easy, thank you for taking the time to make it that way.

Mechanism of the issue From what I can see there are two places in the animation code that lead to this issue.

  1. in AnimationTree::_update_caches (AnimationTree.cpp 659) the initial value of the cached track is zeroed for TYPE_VALUE tracks. This is somewhat code-smelly to me since this happens on the very next line after getting the actual initial value from the animation.

        ERR_CONTINUE_MSG(anim->track_get_key_count(i) == 0, "AnimationTree: '" + String(E) + "', Value Track:  '" + String(path) + "' must have at least one key to cache for blending.");
        track_value->init_value = anim->track_get_key_value(i, 0);
        track_value->init_value.zero(); // <----- Potential problem on this line
    
        // If there is a Reset Animation, it takes precedence by overwriting.
        if (has_reset_anim) {
            int rt = reset_anim->find_track(path, track_type);
            if (rt >= 0 && reset_anim->track_get_key_count(rt) > 0) {
                track_value->init_value = reset_anim->track_get_key_value(rt, 0);
            }
        }
  2. The code in the has_reset_anim conditional is not executed. This leaves track_value->init_value with a value of Variant() aka Nil because that's what zero() does to certain types.
  3. In AnimationTree::_process_graph (AnimationTree.cpp 1076) the value of the track cache is set to init_value which is Nil in this case.
  4. In AnimationTree::_process_graph (AnimationTree.cpp 1483) the final value of the track is acquired by calling Animation::subtract_variant and then Animation::blend_variant. The first call to subtract_variant is fine because it simply returns the direct value from the animation. However, the call to blend_variant uses the Nil value as its first argument, in which case it is directly returned because Nil does not match type with the valid value from the animation. This results in the Nil variant getting stuck in the track because it can't blend with anything.

    if (t->init_value.get_type() == Variant::BOOL) {
        value = Animation::subtract_variant(value.operator real_t(), t->init_value.operator real_t());
        t->value = Animation::blend_variant(t->value.operator real_t(), value.operator real_t(), blend);
    } else {
        value = Animation::subtract_variant(value, t->init_value);
        t->value = Animation::blend_variant(t->value, value, blend);
    }

Workaround Creating a RESET track for the property in question would alleviate this issue, but I think this is still a bug because it defies what I think users consider to be the expected behavior of the animation system, which is that any property with an active animation on it should have that property animated correctly, with or without a reset value.

Potential Fixes The first would be to remove the track_value->init_value.zero(); line. I'm not familiar enough with the engine to know why this was done or whether it needs to stay but zeroing the value we just got from the initial frame doesn't seem useful.

Another fix would be to alter the behavior of Animation::blend_variant to always return the non-Nil value in the case where it is called with one Nil value and one non-Nil value. I imagine this could have broad consequences so I don't know if this is a good idea.

Steps to reproduce

Correct behavior with direct animation:

  1. Open the attached example project
  2. Open repro.tscn
  3. Select the AnimationPlayer node and in the animation tab at the bottom of the screen, preview the continuous_anim animation. Observed: The label text in the middle of the scene interpolates to spell out "Hello Godot" as the animation loops Expected: The string animates as observed

Bugged behavior with blend tree:

  1. Stop the animation preview on the AnimationPlayer
  2. Select the AnimationTree node.
  3. In the inspector, enable the Active checkbox to make the animation tree play Observed: The label text is set to "<null>" and remains that way Expected: The string animates identically to how it does in the above correct case

Minimal reproduction project

GodotAnimationTreeNilBug.zip

TokageItLab commented 4 months ago

Fixed by https://github.com/godotengine/godot/pull/80813 (Deterministic option)