Most of the functions in UAnimNotifyLibrary rely on an accurate EventReference.GetCurrentAnimationTime(), however the current animation time is unreliable in certain circumstances in the editor window, where it would be very useful to scrub the animation state, or freeze on certain frames, in order to test functionality.
-
It looks like the CurrentAnimTime only ever gets set via FAnimNotifyEventReference::GatherTickRecordData
-
If FAnimNotifyEventReference::GatherTickRecordData is called with a null InTickRecord.TimeAccumulator, it sets the current time to 0
-
When you scrub the animation editor to frames of animation within the middle of a notify state, the animation instance has its time set via UAnimSingleNodeInstance::SetPositionWithPreviousTime
-
UAnimSingleNodeInstance::SetPositionWithPreviousTime doesn’t set a valid pointer value for the TickRecord.TimeAccumulator
-
The results in the Notify time falling back to an incorrect value of zero being set on the FAnimNotifyEventReference.CurrentAnimTime
-
[Image Removed]
I believe the fix is as simple as this, but I haven’t taken the time yet to verify all potential code paths that could result in this bug are covered.[Image Removed]
Hi, thanks for flagging this one. The issue I see with the suggested fix is that TickRecord.TimeAccumulator is potentially going to be pointing to garbage after SetPositionWithPreviousTime returns. In practice, this probably doesn’t matter since any work with the notify is likely being done inside TriggerAnimNotifies. But it’s possible that someone could grab the notify at some other point in the update and run into problems. We could reset NotifyQueue after the call to TriggerAnimNotifies but that would be a larger behaviour change. I’ll talk with the dev team to get their thoughts and follow up.
That seems to be the usage in most of the places that set TimeAccumulator, they set it from a stack variable. It appears that the value is only copied off within the same stack context, so there doesn’t appear to be an opening for any long lived references to the TickRecord float pointer. The value is copied into the FAnimNotifyEventReference during the same stack context, in FAnimNotifyEventReference::GatherTickRecordData
[Image Removed]
Yeah, you’re right. I had assumed we were copying TickRecord, but we don’t, we just copy the value of TimeAccumulator, so this change is fine. I’ll get this submitted - it’ll go into 5.8. Thanks again for the suggestion.