RFC: Improve performance of AnimationPlayer.

[therealbnut]

Hi,

I've noticed there's a lot of repeated lookups in AnimationPlayer, and wanted to explore reducing those. This PR is just an RFC for whether I should proceed cleaning up and verifying these changes.

CC @james7132 @superdump who are working on similar things.

On my computer this PR speeds up many_foxes by 2.262011ms, which increases the FPS by 57%.

this branch:
   frame_time :    6.149402ms (avg 5.641781ms)
   fps        :  177.083135   (avg 189.863891)
   frame_count: 4167.000000

main:
   frame_time :    8.411413ms (avg 8.359114ms)
   fps        :  119.594450   (avg 120.259692)
   frame_count: 2009.000000

Objective

• This PR aims to improve the performance of the AnimationPlayer.

Solution

• Flatten the curve vectors to avoid repeated searches.
• Assume the search result index is similar to previous results.
• Memoize Entity lookups for EntityPath.

---

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

• What changed as a result of this PR?
• If applicable, organize changes under "Added", "Changed", or "Fixed" sub-headings
• Stick to one or two sentences. If more detail is needed for a particular change, consider adding it to the "Solution" section
  • If you can't summarize the work, your change may be unreasonably large / unrelated. Consider splitting your PR to make it easier to review and merge!

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

• If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
• Simply adding new functionality is not a breaking change.
• Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.

[james7132]

Please see #6785 as there is sizable overlap with this PR, as well as bevyengine/rfcs#51 for a pathway towards more complex FK controls. I'll give this a more thorough review soon.

[james7132]

Quick cursory readthrough: if the intent is to cache the results of the EntityPath lookup, the animation results are not going to be correct if an intermediate bone is either renamed or there is some tangible change to the hierarchy (deparenting, reparenting, despawns). This is likely undesirable behavior.

[therealbnut]

if an intermediate bone is either renamed or there is some tangible change to the hierarchy (deparenting, reparenting, despawns). This is likely undesirable behavior.

Thanks for taking a look @james7132, yeah I was thinking about that - If this sort of change in general is desirable I want to explore adding new component AnimationCurvePlayer for each Entity pointed to by a path, so their curves are stored on the relevant object. Each curve player could walk up to its parent AnimationPlayer to get the elapsed time, or maybe have a shared weak reference to its state. By having a player for each path it avoids any issues with renames etc.

I’d like to ideally move things that seem glfw specific into that loader, and focus on the Bevy side of things being format agnostic and flexible.

[therealbnut]

@james7132 thanks for the link to the RFC too, I’ll have a proper read through later. Although I think the AnimationCurvePlayer idea may work well with that, if the curve player is generic over the property being animated.

For example, you add a CurveAnimated<Translation> to the entity with the Transform you want to translate. That can optionally (?) have an associated AnimationPlayer which can drive it.

I need to think more about how an AnimationClip will work though, but I think it should be able to be resolved in the glfw loader.

[james7132]

I want to explore adding new component AnimationCurvePlayer for each Entity pointed to by a path, so their curves are stored on the relevant object

The original plan for the aforementioned RFC was to use a the following struct:

#[derive(Component)]
struct BoneBinding {
   animator: Entity,
   bone_id: usize
}

Which would be added in a similar way as you mentioned. However, maintaining that relationship is very hard and change detection on the hierarchy is non-trivial. If we can find a performant and correct way of doing this, that'd be ideal.

[james7132]

@therealbnut The PR to parallelize forward kinematics has been merged, do you think you can rebase this PR? I'd love to see if it's still a performance win.

[therealbnut]

@james7132 it'll take me a while to compare and see which bits can be transferred over, I'll probably have to start over rather than rebase. I'm not sure when I'll get to it, but I'll take a look when I get a chance.

[bas-ie]

Backlog cleanup: closing due to inactivity, and the aforementioned #6785 did get merged in the end.