Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Mixer Animation to bitecs #5938

Merged
merged 1 commit into from
May 11, 2023
Merged

Add Mixer Animation to bitecs #5938

merged 1 commit into from
May 11, 2023

Conversation

takahirox
Copy link
Contributor

@takahirox takahirox commented Feb 10, 2023

This PR adds animation handled with AnimationMixer to bitecs.

The changes are

  • Add MixerAnimatable component which should be added to an entity whose Object3D has AnimationClips in object.animations and manages AnimationMixer for the Object3D.
  • Add MixerAnimatableInitialize component which triggers MixerAnimatable component initialization that will be done in a system. AnimationMixer needs to be created with Object3D so it needs to be initialized after Object3D is initialized.
  • Add LoopAnimation component which manages AnimationActions composed from AnimationClips assigned to Object3D.
  • Add LoopAnimationInitialize component which triggers LoopAnimation component initialization that will be done in a system. AnimationActions needs to be created from AnimcationClips with AnimationMixer so it needs to be initialized after MixerAnimatable is initialized.
  • Add mixerAnimatableSystem which initializes and cleans up MixerAnimatable component and calls AnimationMixer.update() for animatable entities every tick.
  • Add loopAnimationSystem which initializes and cleans up LoopAnimation component.
  • Add MixerAnimatable and LoopAnimation inflators that set up MixerAnimatableInitialize and LoopAnimationInitialize. (MixerAnimatable inflator is a default inflator.)
  • Register the inflators to JSX inflators
  • Add a special path in model inflator for inflating MixerAnimatableInitialize and LoopAnimationInitialize components because Hubs animation-mixer and loop-animation components don't greatly fit to the bitECS MixerAnimatable and LoopAnimation components and systems.

Note that this PR attempts to move the current functionality and capability of the old animation-mixer and loop-animation components. Advanced features (eg: Network animation) should be in another commit.

Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty close but I think we should move some stuff out of inflators into enter queries. I think what you have now will technically work for animated models without loop-animation components (they will play their first animation) but will not correctly handle models with actual loop-animation components (since the root object wont yet have an Object3DTag on it when calling the inflators).

Its definitely very clear that inflators are quite a confusing concept and we need to better figure out how to describe what they should (and should not) be used for.

I do think we unfortunately do need to keep supporting all 3 methods of targeting animations since they are used in existing scenes...

Made a few other suggestions, but nothing major.

};

export function inflateMixerAnimatable(world: HubsWorld, eid: number, params: MixerAnimatableParams) : number {
// TODO: Is it possible to guarantee that
Copy link
Contributor

@netpro2k netpro2k Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in my comment on inflateLoopAnimation, its not safe to access eid2obj in an inflator, so this code probably belongs in an enter query instead. This inflator probably just becomes a default inflator since MixerAnimatable probably doesn't end up needing any properties (since animations can be grabbed off the model).

// TODO: Do we need to keep supporting the following two params?
// DEPRECATED: Use activeClipIndex instead since animation
// names are not unique
clip?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Blender currently uses clip a comma separated list of names. Spoke uses activeClipIndices. I don't see anything using activeClipIndex but I bet there are old Spoke scenes that use it. We probably need to support all 3 for now :(

Lets discuss which one we want to be the canonical representation (probably activeClipIndicies) and then handle converting the others to that as early as possible in the flow, so the rest of the code does not need to think about it.

We have discussed in other places moving this sort of legacy migration all to an earlier point in GLTF loading after we completely remove gltf-model-plus, so we should add a TODO about that part of the code here.

};

export function inflateLoopAnimation(world: HubsWorld, eid: number, params: LoopAnimationParams) : number {
// Is it possible to guarantee that LoopAnimation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not. It's invalid to try and get the Object3D in an inflator. An inflator may be the thing that ends up creating the Object3D for an entity. Needing to get the object is a good sign that this code should be in an enter query instead.

inflators should just set up the component data and (if they are a component managing the Object3D for an entity) create the Object3D and call addObject3DComponent. We definitely need to make sure to call this out clearly in the docs since I suspect it will be a repeated point of confusion.

@@ -245,6 +246,7 @@ export function mainTick(xrFrame: XRFrame, renderer: WebGLRenderer, scene: Scene
hubsSystems.gainSystem.tick();
hubsSystems.nameTagSystem.tick();
simpleWaterSystem(world);
mixerAnimationSystem(world, dt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can grab dt off of world.time.delta

// MixerAnimatable component or Scene/App?

export function mixerAnimationSystem(world: HubsWorld, delta: number) : void {
mixerAnimatableQuery(world).forEach(eid => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this loop I think we can add an enteryQuery here for MixerAnimatable where we will actually set up the mixer. Then we can have an enter query for [MixerAnimatable,LoopAnimation] to set up the clip for the looping animation and add it to the mixer.
Then by the time we hit this loop everything is guaranteed to be set up.

mediaLoader?: MediaLoaderParams;
mixerAnimatable?: MixerAnimatableParams;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we will end up needing an inflator for this or if we need to expose it to JSX. I think it's only valid to be inserted internally as part of a model component, but maybe we will find a use for manually making one in JSX as well... Not sure.

@@ -139,6 +141,20 @@ export function inflateModel(world: HubsWorld, rootEid: number, { model }: Model
}
});

// TODO: Not sure if here is the best place and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable spot to set up the MixerAnimatable component and set up a default LoopAnimation component if one does not exist already.


// Not sure here is the best place to clean up
// these two components. We might need to revisit.
exitedLoopAnimationQuery(world).forEach(eid => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of minor, but I think we want to run this before we tick the animation mixers this frame otherwise we will run an additional frame of that animation.

});

exitedMixerAnimatableQuery(world).forEach(eid => {
cleanupMixerAnimatable(eid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: No need for the wrapping function here and above, though I think most of the logic in there will be moving into this system anyway, so this code may just end up getting inlined instead of being with the inflator.

@@ -150,7 +150,12 @@ MediaPDF.map = new Map();
export const MediaVideo = defineComponent({
autoPlay: Types.ui8
});
export const AnimationMixer = defineComponent();
export const MixerAnimatable = defineComponent({});
// Map<EntityId, {mixer: AnimationMixer: animations: Array<AnimationClip>}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure exactly the syntax but typescript can pickup on jsdoc comments for type info, so we might be able to use that here. Similar to what we did here https://github.com/mozilla/hubs/blob/c78cec579eb422521e06cf1459469860277a47cd/src/components/gltf-model-plus.js#L818-L823

@takahirox
Copy link
Contributor Author

takahirox commented Mar 18, 2023

Updated to reflect the review comments.

I don't really like some of the designs in the PR, but want to focus on making it work as it worked before for now because it is at very last minutes in this cycle. I want to revisit and refactor in the next or later cycle.

@takahirox
Copy link
Contributor Author

takahirox commented Apr 10, 2023

I started to think that the first design is not intuitive and is buggy. I rewrote the implementation. Please refer to the PR comment for the changes.

@keianhzo keianhzo self-requested a review April 11, 2023 12:30
Copy link
Contributor

@keianhzo keianhzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some comments, let me know what you think.

Comment on lines +12 to +20
initializeEnterQuery(world).forEach(eid => {
addComponent(world, MixerAnimatable, eid);

const object = world.eid2obj.get(eid)!;
const mixer = new AnimationMixer(object);
MixerAnimatableData.set(eid, mixer);

removeComponent(world, MixerAnimatableInitialize, eid);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If adding MixerAnimatable is something that we have to always do for any component that has animations, wouldn't it be simple to add the mixer in inflateModel and save this query?

Something like:

if (model.animations !== undefined && model.animations.length > 0) {
    const mixer = new AnimationMixer(model);
    MixerAnimatableData.set(rootEid, mixer);
    addComponent(world, MixerAnimatable, rootEid);
    if (!findChildWithComponent(world, LoopAnimation, rootEid)) {
      inflateLoopAnimation(world, rootEid, {});
    }
  }

Copy link
Contributor Author

@takahirox takahirox Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a mix feeling about it.

Pros: Yes, the implementation in model will be simpler. And the two step initializations (MixerAnimatableInitialize -> MixerAnimatable) for model will not be needed.
Cons: Object (AnimationMixer in this case) creation and clean up are spreaded out, creation is here while clean up is in mixerAnimatableSystem. I think it would be more readable and maintainable if both are placed in a single place. And MixerAnimatable initializations can be inconsistent between glTF and JSX. I think it would be more readable and maintainable if they are consistent.

Personally I prefer consistency and that both creation and clean up are placed in a sinble place, so I want to go as is unless there is any critical problems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense, I think it's better to prioritize consistency.

@@ -0,0 +1,87 @@
import { addComponent, defineQuery, enterQuery, exitQuery, hasComponent } from "bitecs";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that some non used imports were left

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks.

import { MixerAnimatable, MixerAnimatableData, LoopAnimation, LoopAnimationData, Object3DTag } from "../bit-components";
import { HubsWorld } from "../app";

const loopAnimationQuery = defineQuery([MixerAnimatable, LoopAnimation, Object3DTag]);
Copy link
Contributor

@keianhzo keianhzo Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works good for JSX entities like the LoadingObject where the MixerAnimatable and LoopAnimation components are both added to the root object where the animations are but when a glTF model is loaded the MixerAnimatableInitialize (and hence the MixerAnimatable component) is added to the glTF root node but the LoopAnimation component can be in a child node so loopAnimationQuery doesn't iterate over the right LoopAnimation component.

My understanding is that we need to add a MixerAnimatable and create a mixer at the root of any gltf scene with animations and every child in that scene with a LoopAnimation component will look for a parent with a MixerAnimatable and get the mixer and animations and then create AnimationActions based on its LoopAnimation parameters.

My other comment referred to this, when a model is loaded in inflateModel if there are any animations we can already create a mixer and link the animations, something like:

if (model.animations !== undefined && model.animations.length > 0) {
    const mixer = new AnimationMixer(model);
    MixerAnimatableData.set(rootEid, {
      mixer,
      animations: model.animations
    });
    addComponent(world, MixerAnimatable, rootEid);
    if (!findChildWithComponent(world, LoopAnimation, rootEid)) {
      inflateLoopAnimation(world, rootEid, {});
    }
  }

and then during the LoopAnimation query enter loop retrieve the mixer and animations from closest parent:

const mixerEid = findAncestorWithComponent(world, MixerAnimatable, eid);
const { mixer, animations } = MixerAnimatableData.get(mixerEid)!;

Copy link
Contributor Author

@takahirox takahirox Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but when a glTF model is loaded the MixerAnimatableInitialize (and hence the MixerAnimatable component) is added to the glTF root node but the LoopAnimation component can be in a child node so loopAnimationQuery doesn't iterate over the right LoopAnimation component.

Let me clarify some things before diving into more. I first want to confirm the specification.

  1. Is loop-animation Hubs component a glTF node level extension, not a glTF scene level extension?
  2. Can there be multiple loop-animations in glTF nodes in a single glTF, for example gltf.nodes[0], gltf.nodes[1], and gltf.nodes[2] have loop-animation component?
  3. If 2. is yes, can loop-animation components have different component parameters from each other?
  4. If 3. is yes, can loop-animation component parameters that are contradict from each other be valid, for example gltf.nodes[0].loop-animation = {activeClipIndex: 0, paused: false}, gltf.nodes[1].loop-animation = {activeClipIndex: 0, paused: true}?

Copy link
Contributor Author

@takahirox takahirox Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked through the old code and you seem to be right. It seems that AnimationMixer needs to be found from ancestors and root entity needs to be aware of LoopAnimations in its children. I don't really like this design, but we need to make the new code compatible with the old code for now. I will update the code.


I want to note concerns of this design for future improvement.

I don't really want to let an entity have a dependency with other entities (ancestors or children). It can make the system design less readable and maintainable. For example query mechanism won't work.

const query = defineQuery([Foo, Bar]);
export function system(world: HubsWorld) {
  query(world).forEach(eid => { 
    const a = Foo[eid].a;
    const b = Bar[eid].b;
    ...
  }); 
}

needs to be replaced with

const query = defineQuery([Foo]);
export function system(world: HubsWorld) {
  enterQuery(world).forEach(eid => { 
    const mixerEid = findAncestorWithComponent(world, Bar, eid);
    if (!mixerEid) return;
    const a = Foo.a[eid];
    const b = Bar.b[mixerEid];
    ...
  }); 
}

And if someone changes the scene hierarchy, it may affect the system. To avoid problems devs need to be aware of special restrictions. It can make the design and implementation non-intuitive.


Perhaps Hubs loop-animation component should be defined at root level. loop-animation can specify which clip(s) should be played. An animation clip (= a glTF animation) can compose of multiple target nodes. So, defining a loop-animation at a node level is weird to me. Root level definition is more natural.

And MixerAnimatable and LoopAnimation components can be set to a root eid, then the concerns written above can be avoided.

loop-animation will need to take an array to control multiple clips separately.

"loop-animation": [
  {
    "activeClipIndices": [0, 1],
    "timeScale": 1.0
  },
  {
    "activeClipIndices": [2],
    "timeScale": 0.5
  }
]

Validation can also be easier. Eg: regarding contradict definitions as invalid, for example root.loop-animation[0] = {activeClipIndices: [0], paused: false}, root.loop-animation[1] = {activeClipIndices: [0], paused: true}

Copy link
Contributor Author

@takahirox takahirox Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably even with the current Hubs loop-animation component specification, we may let LoopAnimation take an array of params, collect hubs-component params in child glTF nodes, and set to root entity to avoid to have a dependency from an entity with another entity.

const params = [];

model.traverse(obj => {
  const param = obj.userData.gltfExtensions?.MOZ_hubs_components?['loop-animation'];
  if (param) {
    params.push(param);
  }
});

addComponent(world, LoopAnimation, rootEid);
LoopAnimation.params.set(rootEid, params);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have talked about this privately but for future reference about how the loop-animation component works.

  1. Is loop-animation Hubs component a glTF node level extension, not a glTF scene level extension?

As of this writing the loop-animation component can the attached to any mesh node. In Blender we expose the animation actions/nla tracks of that mesh for selection so each mesh with a loop-animation will only reference it's own animations and it can't reference animations in other meshes. Same applies for Spoke, only meshes with animations will show the Loop Animation component options and list that mesh animations.

  1. Can there be multiple loop-animations in glTF nodes in a single glTF, for example gltf.nodes[0], gltf.nodes[1], and gltf.nodes[2] have loop-animation component?

Yes, any mesh node can have one.

  1. If 2. is yes, can loop-animation components have different component parameters from each other?

Yes, each loop-animation component can have their own independent parameters.

  1. If 3. is yes, can loop-animation component parameters that are contradict from each other be valid, for example gltf.nodes[0].loop-animation = {activeClipIndex: 0, paused: false}, gltf.nodes[1].loop-animation = {activeClipIndex: 0, paused: true}?

No, each loop-animation component will only reference it's own meshes animations and each mesh node can on ly have one loop-animation component so conflict is not possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. My impression is Hubs animation system assumes a little bit too much implied rules in glTF, for example it assumes that an animation clip consists of a ceratain root glTF node and its child nodes but there is no such a limitation in the glTF animation specification.

I think it would be better to let loop-animation component be aligned with glTF spec more and remove such implied rules. Probably the component specification will be more straightforward and less problematic, for example when processing a Hubs glTF assets with external glTF tools.

If we want to keep the animation representation in Blender, for example binding a loop-animation copmponent to a certain node, we may add info needed in glTF extra field.

It isn't the best time to revisit the loop-animation component spec now. We may revisit the spec later.

@takahirox takahirox changed the title Move Mixer Animation to bitecs Add Mixer Animation to bitecs Apr 11, 2023
@takahirox
Copy link
Contributor Author

As @keianhzo mentioned, the previous implementation didn't handle Hubs loop-animation component in glTF correctly. I think it may be better to revisit the Hubs component specification but I don't think it's the best time to do that now. We should focus on migration and making the new entity system compatible with the old ones.

I updated the implementation to handle it. Refer to the PR for the update.

@keianhzo keianhzo self-requested a review April 13, 2023 10:11
Copy link
Contributor

@keianhzo keianhzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand better the pros/cons of this approach Vs keeping the LoopAnimation component in each entity and rely on an ancestor having a mixer. Any entity that has a LoopAnimation component should have an ancestor with a mixer as they are actually completely coupled so I think it would be fair to rely on that relationship. I feel we are adding a few more hops here to avoid that.

The concept of inflator seems to be diverging here from what we are doing for other components. I would expect it to add a LoopAnimation component but it's not doing that, the component is added in the system. Maybe we could rename it to something like inflateLoopAnimationInitialize to avoid confusion?

Most of my concerns are design related and reflect just my current my preference so I don't want to block as many of this will be discussed eventually and this is working correctly.

Comment on lines 32 to 41
const DEFAULTS: Required<LoopAnimationParams> = [
{
activeClipIndex: 0,
clip: "",
activeClipIndices: [],
paused: false,
startOffset: 0,
timeScale: 1.0
} as Required<ElementParams>
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be reduced to?

const DEFAULTS: LoopAnimationParams = [ELEMENT_DEFAULTS];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Updated, thanks.

This commit adds animation handled with AnimationMixer to bitecs.

The changes are
* Add MixerAnimatable component which should be added to an entity
  whose Object3D has AnimationClips in object.animations and
  manages AnimationMixer for the Object3D.
* Add MixerAnimatableInitialize component which triggers
  MixerAnimatable component initialization that will be done in a
  system. AnimationMixer needs to be created with Object3D so it
  needs to be initialized after Object3D is initialized.
* Add LoopAnimation component which manages AnimationActions
  composed from AnimationClips assigned to Object3D.
* Add LoopAnimationInitialize component which triggers
  LoopAnimation component initialization that will be done in a
  system. AnimationActions needs to be created from AnimcationClips
  with AnimationMixer so it needs to be initialized after
  MixerAnimatable is initialized.
* Add mixerAnimatableSystem which initializes and cleans up
  MixerAnimatable component and calls AnimationMixer.update()
  for animatable entities every tick.
* Add loopAnimationSystem which initializes and cleans up
  LoopAnimation component.
* Add mixerAnimatable and loopAnimation inflators that
  set up MixerAnimatableInitialize and LoopAnimationInitialize.
  (mixerAnimatable inflator is a default inflator.)
* Register the inflators to JSX inflators
* Add a special path in model inflator for inflating
  MixerAnimatableInitialize and LoopAnimationInitialize components
  because Hubs animation-mixer and loop-animation components
  don't greatly fit to the bitECS MixerAnimatable and
  LoopAnimation components and systems.

Note that this commit attempts to move the current functionality
and capability of the old animation-mixer and loop-animation
components. Advanced features (eg: Network animation) should be
in another commit.
@takahirox
Copy link
Contributor Author

takahirox commented Apr 13, 2023

Any entity that has a LoopAnimation component should have an ancestor with a mixer as they are actually completely coupled so I think it would be fair to rely on that relationship.

Some thoughts.

  1. No dependency from an entity with others in a system is better for sure in terms of readability, maintainability, and so on.
  2. I don't think the current the Hubs loop-animation component specification is good. It should be moved to root or scene level because it will be more natural and aligned with glTF spec and less problematic. Then LoopAnimation bitECS component will be able to be set to the root entity without any problem.
  3. But we have to support the current Hubs loop-animation component for now. We may need hack anywhere anyways. Basically we need to choose which one, my implementation or having dependency with other entities. I prefer mine because hack is placed to be only in model inflator. And I don't really want a system to be complex for the component specification we may revisit later.

Regarding the naming, ok inflateLoopAnimationInitialize is less confusing. Updated.

@takahirox
Copy link
Contributor Author

This PR needs to be unlocked by the reviewers to get merged.

@keianhzo
Copy link
Contributor

keianhzo commented May 9, 2023

@takahirox unlocked

Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now seems to handle both caes but I left some feedback into how things are structured. The additional "Initialize" components feel quite strange to me, and I am not too sure what they are buying us. Also suggested possibly cleaner ways of exposing the Map data for the components. I am slightly uneasy about how the LoopAnimation components have been consolidated, but I think that's fine since that probably the direction we wanted to eventually go anyway.

const mixerExitQuery = exitQuery(mixerQuery);

export function mixerAnimatableSystem(world: HubsWorld): void {
initializeEnterQuery(world).forEach(eid => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused why we need 2 components here (MixerAnimatableInitialize and MixerAnimatable). Can't we just create the Mixer on enter query for MixerAnimatable? That's what I did in the quick stub of this I am using in my behavior graph branch and it seems to work well https://github.com/mozilla/hubs/blob/behavior-graphs-spike/src/bit-systems/behavior-graph.ts#L82-L101. Since we have strong control over exectuion order we can guarantee that the mixer is set up before any other code using it runs (both in this system and the loop-animation system).

I would also prefer to keep the map on the MixerAnimatable component itself instead of having it floating seperatly. It makes it feel more like any other component property: https://github.com/mozilla/hubs/blob/behavior-graphs-spike/src/bit-components.js#L185-L190. With the type comment I have there it also typechecks just fine too.

mixer.update(world.time.delta / 1000.0);
});

mixerExitQuery(world).forEach(eid => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it particularly matters in this case, but I tend to do enter query, exit query, and then the general work of the system.

/**
* @type {Map<EntityId, AnimationMixer}>}
*/
export const MixerAnimatableData = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer we start putting component properties on this on the component and just using type annotations, like https://github.com/mozilla/hubs/blob/behavior-graphs-spike/src/bit-components.js#L185-L190

const loopAnimationExitQuery = exitQuery(loopAnimationQuery);

// Question: Who should have AnimationMixer?
// MixerAnimatable component or Scene/App?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to have "on" a MixerAnimatable component . Then when loop-animation components are added we find the nearest ancestor that is MixerAnimatable and use its mixer.

timeScale?: number;
};

export type LoopAnimationParams = ElementParams[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was quite confused why this is an array, but I see in model inflator that you are attempting to consolidate all the loop-animation components in a model into a single one. I think that makes some sense. It's the component we would like to have. Though if we are going to do that anyway, I think the component data should be consolidated earlier too. Maybe it should just be an array of clips or actions (since that's what you end up doing in the system anyway). That way we can move the messy handling of the legacy properties all up front and not have the rest of the code need to think about it.

@@ -139,6 +153,16 @@ export function inflateModel(world: HubsWorld, rootEid: number, { model }: Model
}
});

// Hubs loop-animation component is defined at glTF node level.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I would have bothered trying to consolidate these at this stage. Writing an inflator for the GLTF data that added a LoopAnimation component, who's data is an array of clips, and then having a system that find's the ancestor mixer to play those clips on could have worked. I suppose this gets us closer to where we would eventually like to be with it but adding more special cases to ignoredComponents is definitely something we should really try and minimize. Animations in three are tricky though and quite tied to GLTF loading and the threejs hierarchy, so I can see why it might be justified.

loadModel(loadingObjectSrc, null, true).then(gltf => {
disposeNode(loadingObject);
loadingObject = gltf.scene;
});

// TODO: Do we really need to clone the loadingObject every time?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have more than one so we definitely need multiple. We could render it as instanced geometry which might be a good idea, probably even better than pooling.

const object = world.eid2obj.get(eid)!;
const mixer = MixerAnimatableData.get(eid)!;

addComponent(world, LoopAnimation, eid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something since you ended up doing it twice. What does the additional LoopAnimationInitialize component get us vs doing this in an enter query for LoopAnimation itself?

@takahirox takahirox merged commit 4fa9078 into master May 11, 2023
9 of 11 checks passed
@takahirox takahirox deleted the BitECSAnimation branch May 11, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants