KybernetikGames / animancer

Documentation for the Animancer Unity Plugin.
63 stars 8 forks source link

End Events not respecting the Time set in the Inspector #305

Closed KybernetikGames closed 9 months ago

KybernetikGames commented 1 year ago

Originally posted by @LukeBolly in https://github.com/KybernetikGames/animancer/issues/261#issuecomment-1686469249

I'm trying to set the time that the EndEvent plays at using the inspector, I've set it to 0.5x here image

I'm still setting the end event through code State.Events.OnEnd = () => FinishOverride(result.State, animConfig.PlaybackLayer, onEndAction);

It looks like the EndEvent uses the end time of the full length animation regardless of where I set End Time in the inspector, if I log the Transition.Length I get the correct 0.6. However the EndEvent plays at a NormalizedEndTime of 1, not the 0.5x like it shows in the inspector.

Any idea how I can get the End Event to fire at the End Time set in the inspector?

KybernetikGames commented 1 year ago

@LukeBolly I've moved your issue to a new thread since it doesn't sound related to the one you posted in.

End Events should definitely respect the End Time set in the Inspector.

Take a look at the Runtime Events at the bottom of your screenshot. That should help determine whether there's an issue in the event handler system or the time you set isn't actually making it to that system at runtime.

Also, if that Transition is being used by multiple characters then they're most likely sharing the same events so they would be triggering each other's end events which could look like it's triggering at the wrong time.

ADH-LukeBollam commented 1 year ago

The inspector is showing correctly: image

But the code:

Action action = () => FinishOverride(result.State, animConfig.PlaybackLayer, onEndEvent);
result.State.Events.OnEnd = action;
Debug.Log($"end time is at {result.State.NormalizedEndTime}");

shows me a NormalizedEndTime at 1.0, so it's not being applied correctly when assigning an onEnd callback. I've been able to work around it by calculating the endTime from the ClipTransition and assigning a whole AnimancerEvent instead. I would have liked to use a ITransitionDetailed in my configuration though but it doesn't have an accessible Length property:

var normalizedEndTime = selectedAnimation.NormalizedStartTime + selectedAnimation.Length / selectedAnimation.Clip.length;
Action action = () => FinishOverride(result.State, animConfig.PlaybackLayer, onEndEvent);
var ee = new AnimancerEvent(normalizedEndTime, action);
result.State.Events.EndEvent = ee;

Also, if that Transition is being used by multiple characters then they're most likely sharing the same events so they would be triggering each other's end events which could look like it's triggering at the wrong time.

My understanding from the other thread was that if I didn't assign any events in the inspector, Animancer would instead grab a new event sequence from the object pool when I added one in code at runtime so that it wouldn't be shared. Does setting the End Time count as assigning an event and break this? FYI I've only been testing with a single character in the scene so this won't have affected any of my tests regardless.

Also I forgot to add, I'm on version 7.4.2.

KybernetikGames commented 1 year ago

But the code ... shows me a NormalizedEndTime at 1.0, so it's not being applied correctly when assigning an onEnd callback.

The OnEnd property definitely only sets the callback so if you log the end time before that it should already be 1, meaning the issue must be somewhere between there and where you're playing the transition.

Does setting the End Time count as assigning an event and break this?

Yes, the end time and callback are stored with the other events so anything other than the default value would cause it to have its own non-pooled events.

On a similar note, if result is your transition then result.State will contain the last state that was played on any character sharing that transition so you would likely see issues from that when you start having multiple characters.

KybernetikGames commented 1 year ago

I would have liked to use a ITransitionDetailed in my configuration though but it doesn't have an accessible Length property

ITransitionDetailed has a MaximumDuration because the actual length can vary in mixers depending on which of their children are active.

But you also have access to the state at that point so you can just get its length.

ADH-LukeBollam commented 12 months ago

The OnEnd property definitely only sets the callback so if you log the end time before that it should already be 1, meaning the issue must be somewhere between there and where you're playing the transition.

This is pretty much the code, there's a calculation to add the EndTime back because it will just be 1 otherwise. Let me know if you see anything that might be causing the EndTime to not be respected.

        CustomTransition selectedAnimation = animConfig.Clips[UnityEngine.Random.Range(0, animConfig.Clips.Count)];

        if (fadeDuration != null)
        {
            result.State = _animancer.Layers[(int)animConfig.PlaybackLayer].Play(selectedAnimation, fadeDuration.Value);
        }
        else
        {
            result.State = _animancer.Layers[(int)animConfig.PlaybackLayer].Play(selectedAnimation);
        }

        // set animation duration for non-looping animations
        if (targetAnimationLength != null && !selectedAnimation.IsLooping)
        {
            var normalAnimationLength = selectedAnimation.Length;
            var playbackSpeed = normalAnimationLength / targetAnimationLength.Value;
            result.State.Speed = playbackSpeed;
            result.PlayDuration = targetAnimationLength.Value;
        }
        else
        {
            result.PlayDuration = selectedAnimation.Length;
        }

        // dont fire end event for endless repeating animations
        if (!selectedAnimation.IsLooping)
        {
            var normalizedEndTime = selectedAnimation.NormalizedStartTime + selectedAnimation.Length / selectedAnimation.Clip.length;
            Action action = () => FinishOverride(result.State, animConfig.PlaybackLayer, onEndEvent);
            var ee = new AnimancerEvent(normalizedEndTime, action);
            result.State.Events.EndEvent = ee;
        }

        result.SkillRotationDeg = selectedAnimation.SkillRotationDeg;

        return result;
KybernetikGames commented 12 months ago

Nothing looks obviously wrong there. If you want to find out what the problem is you'll need to log the values you're getting to figure out where they're coming from. Log the state's end time right after it plays. Log the transition's end time. Make sure that code is actually referring to the right transition, etc. The value obviously has to be coming from somewhere.

ADH-LukeBollam commented 11 months ago

@KybernetikGames I think this might be a bug, the end time is definitely not being copied into the state, I'll attach some more images so you can see if there is anything wrong with my set up.

  1. I set an EndTime in the inspector, but I don't need any callbacks so I leave that blank (I'm watching for the end by polling now to avoid issues with the shared events). image

  2. When I grab the transition to play it, here's what the transition (selectedAnimation) looks like when debugging. It all looks fine so far.

result.State = _animancer.Layers[(int)animConfig.PlaybackLayer].Play(selectedAnimation);

image

  1. This is where it goes wrong, here is what the returned state (result.State) looks like image

Note it doesn't have the EndEvent NormalizedTime at all and the Length is back to the default animation length. This is me inspecting the resulting state immediately after playing. I haven't made any modifications to it.

And in case it matters, here is what my DetailedTransition class looks like:

public class DetailedTransition : ClipTransition
{
    [field: SerializeField] public float Weight { get; private set; } = 1;
    [field: SerializeField] public float SkillRotationDeg { get; private set; }

    public override void Apply(AnimancerState state)
    {
        base.Apply(state);

        // Fade the layer in to target weight
        if (state.Layer.Weight != 0)
        {
            state.Layer.StartFade(Weight, FadeDuration);
        }
    }
}

And in case it's relevant too, I run this code to initialise all the animation states to avoid some visual bugs (I can't remember what they were, maybe something to do with flickering with an Animation Rig?)


        _animancer.Playable.KeepChildrenConnected = true;

        foreach (var a in _animationSet.Actions)
        {
            foreach (var c in a.Value.Clips)
            {
                _animancer.Layers[(int)a.Value.PlaybackLayer].GetOrCreateState(c);
            }
        }
KybernetikGames commented 11 months ago

The fact that the state.Length shows the full animation length is by design, if you want to account for the end event you can use the state.Duration instead. Having the transition.Length account for the end time is definitely confusing though, I'll rename it to Duration to match the states for the next version.

I still can't see any way the end time could get changed like that, so I can only recommend putting in more logs to narrow down exactly where the issue is occurring:

If you still aren't able to find the issue, feel free to send me a minimal reproduction project (ideally with only a single script to demonstrate the issue and an animation or two) and I'll take a look at it.

KybernetikGames commented 11 months ago

Could the transition's Weight be 0? Fading out the layer would cause it to Stop all its states and clear their events, though I can't see how everything else would still seem to be working properly if that happened.

ADH-LukeBollam commented 11 months ago

Okay, I've narrowed it down to the fading code in the transition but it's not like you said. It appears running StartFade() to any weight is enough to break the events being copied.

See this log of events with this transition Apply() override:

    public override void Apply(AnimancerState state)
    {
        base.Apply(state);

        Debug.Log($"fading from {state.Layer.Weight} to {Weight}");
        state.Layer.StartFade(Weight, FadeDuration);
    }

image

If I remove the StartFade() from the Apply() call, the events are copied correctly. image

ADH-LukeBollam commented 11 months ago

Ah, I guess this is the source of my problems: image

The Layer calls OnStartFade of all of the States, which will automatically wipe the events even if the fade target isn't 0

KybernetikGames commented 11 months ago

I don't know how I missed it yesterday but yeah that will be it.

Try putting your StartFade before Apply so it clears the events before assigning the transition's events to the state.

If that doesn't work, just set AnimancerState.AutomaticallyClearEvents = false; before StartFade then back to true afterwards.

I'm trying to think of a way I could detect and warn when something like this happens, but all I can come up with would be to assert that the state matches the transition after calling Apply but that just seems silly when none of the included transition types could have this issue. Can you think of anything else I could have done?

ADH-LukeBollam commented 11 months ago

Well for me I'm using the StartFade() method to fade IN a layer, but it looks like state.OnStartFade makes an assumption that you would only ever fade a state out and fade it to 0 which is why it always removes the events. That's obviously not true for me, but maybe I'm butchering the intended use-case of this method. This is what I needed from AnimancerState.OnStartFade():

        protected internal override void OnStartFade(float targetWeight)
        {
            if (targetWeight == 0 && AutomaticallyClearEvents)
                EventDispatcher.TryClear(_EventDispatcher);
        }
KybernetikGames commented 11 months ago

The reason I didn't implement it like that is in case you play the same clip that was already playing, meaning it would reuse the same state and still have the old events on it which would be inconsistent with the way Play works the rest of the time.

That won't be an issue when using transitions though so you might never have a problem with it, other than needing to remember to make the same change again if you ever update Animancer.