Incorporate all node weights in additive blending

[mweatherley]

Objective

In the existing implementation, additive blending effectively treats the node with least index specially by basically forcing its weight to be 1.0 regardless of what its computed weight would be (based on the weights in the AnimationGraph and AnimationPlayer).

Arguably this makes some amount of sense, because the "base" animation is often one which was not authored to be used additively, meaning that its sampled values are interpreted absolutely rather than as deltas. However, this also leads to strange behavior with respect to animation masks: if the "base" animation is masked out on some target, then the next node is treated as the "base" animation, despite the fact that it would normally be interpreted additively, and the weight of that animation is thrown away as a result.

This is all kind of weird and revolves around special treatment (if the behavior is even really intentional in the first place). From a mathematical standpoint, there is nothing special about how the "base" animation must be treated other than having a weight of 1.0 under an Add node, which is something that the user can do without relying on some bizarre corner-case behavior of the animation system — this is the only present situation under which weights are discarded.

This PR changes this behavior so that the weight of every node is incorporated. In other words, for an animation graph that looks like this:

┌───────────────┐                                 
│Base clip      ┼──┐                              
│      0.5      │  │                              
└───────────────┘  │                              
┌───────────────┐  │  ┌───────────────┐     ┌────┐
│Additive clip 1┼──┼─►┤Additive blend ┼────►│Root│
│      0.1      │  │  │      1.0      │     └────┘
└───────────────┘  │  └───────────────┘           
┌───────────────┐  │                              
│Additive clip 2┼──┘                              
│      0.2      │                                 
└───────────────┘                                 

Previously, the result would have been

base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2

whereas now it would be

0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2

and in the scenario where base_clip is masked out:

additive_clip_1 + 0.2 * additive_clip_2

vs.

0.1 * additive_clip_1 + 0.2 * additive_clip_2

Solution

For background, the way that the additive blending procedure works is something like this:

• During graph traversal, the node values and weights of the children are pushed onto the evaluator stack. The traversal order guarantees that the item with least node index will be on top.
• Once we reach the Add node itself, we start popping off the stack and into the evaluator's blend_register, which is an accumulator holding up to one weight-value pair:
  • If the blend_register is empty, it is filled using data from the top of the stack.
  • Otherwise, the blend_register is combined with data popped from the stack and updated.

In the example above, the additive blending steps would look like this (with the pre-existing implementation):

1. The blend_register is empty, so we pop (base_clip, 0.5) from the top of the stack and put it in. Now the value of the blend_register is (base_clip, 0.5).
2. The blend_register is non-empty: we pop (additive_clip_1, 0.1) from the top of the stack and combine it additively with the value in the blend_register, forming (base_clip + 0.1 * additive_clip_1, 0.6) in the blend_register (the carried weight value goes unused).
3. The blend_register is non-empty: we pop (additive_clip_2, 0.2) from the top of the stack and combine it additively with the value in the blend_register, forming (base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2, 0.8) in the blend_register.

The solution in this PR changes step 1: the base_clip is multiplied by its weight as it is added to the blend_register in the first place, yielding 0.5 * base_clip + 0.1 * additive_clip_1 + 0.2 * additive_clip_2 as the final result.

Note for reviewers

It might be tempting to look at the code, which contains a segment that looks like this:

if additive {
    current_value = A::blend(
        [
            BlendInput {
                weight: 1.0, // <--
                value: current_value,
                additive: true,
            },
            BlendInput {
                weight: weight_to_blend,
                value: value_to_blend,
                additive: true,
            },
        ]
        .into_iter(),
    );
} 

and conclude that the explicit value of 1.0 is responsible for overwriting the weight of the base animation. This is incorrect.

Rather, this additive blend has to be written this way because it is multiplying the existing value in the blend register by 1 (i.e. not doing anything) before adding the next value to it. Changing this to another quantity (e.g. the existing weight) would cause the value in the blend register to be spuriously multiplied down.

Testing

Tested on animation_masks example. Checked morph_weights example as well.

Migration Guide

I will write a migration guide later if this change is not included in 0.15.

[alice-i-cecile]

@pcwalton @james7132 @mockersf I'm inclined to include this in 0.15, to avoid shipping weird behavior.

[pcwalton]

LGTM, well motivated and seems like a straightforward bug fix. Agree with shipping as part of the release.

[alice-i-cecile]

This is much less confusing behavior, and the changes are simple and clear.