-
Notifications
You must be signed in to change notification settings - Fork 209
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
2532 app create state machine for animations #2537
Conversation
…//github.com/webaverse/app into 2532-app-create-state-machine-for-animations
avatars/StateMachine.js
Outdated
// add object to state machine | ||
registerObj(name, obj) { | ||
this.tracking.has(name) || this.tracking.set(name, { | ||
id: crypto.randomUUID(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have makeId
in utils:
Line 609 in 51acb44
export function makeId(length) { |
It could be changed to crypto.randomUUID()
but the codebase should have one place that defines the shape of IDs (since that may need to be changed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm easy either way, I'll have the SM use makeId
and we can have it use UUID's in another ticket if we want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moving to makeId
is fine for now.
character-controller.js
Outdated
this.getActionsList().forEach((name) => { | ||
this.avatar.tracker.registerState({ | ||
name, | ||
animationFn: ()=>{console.log(`${name} func ran`);} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For debugging? Probably shouldn't ship this in PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for debugging, will remove
The state engine needs thinking about how we are blending states (lerp, multiply, or something else), in which order, and for which bone subset. The order and bone blend functions both matter a lot, and are not easily defined declaratively. Is the plan just to copy the execution logic from Avatar's |
If you want this you could just float this change yourself. I use the same trick but by committing this it actually breaks clean global assumptions for everyone. Can we remove? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, some questions.
I'm planning on creating an animation factory which will handle how blending is handled and other animation specific code. To keep things moving swiftly I'm setting things up the new system to follow the same general execution logic we're currently using, and once the new system is handling that I will continue to expand ordering. The state machine #2533 is the animation factory ticket, it doesn't reference bone subsets specifically but I'll make it explicit |
this is not actually in this PR, just a note for QA if it's helpful for them when testing |
@@ -408,6 +409,9 @@ class Avatar { | |||
}; | |||
} | |||
|
|||
const stateName = "playerAvatar" | |||
StateMachine.registerObj(options.name ?? stateName, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't used yet, but we cannot depend on options.name
being unique, so this will collide and overwrite.
What was the intent here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why is this mutating its argument this
rather than returning a tracker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registering objects first checks to see if it's already being tracked on line 55 of StateMachine.js. If it is, it won't reregister it, it will just take the object being passed and provide it with the tracker.
The state mechanism probably needs a redesign. This PR is fine but it's not adding any functionality, and the state engine tracking is bugged (name collisions resulting in shared state). When there is functionality to add we can re-open. |
closes #2532
Initial state machine which will be used for animations. There is already a state system in place for player actions, which this will work along side. Future updates to this state machine will allow for an action to trigger animations with multiple states which will be managed here.
This PR currently has a minimal actual implementation, Avatars register themselves for tracking, and states are registered for the player avatar with dummy functions, but it's not actually controlling anything right now(although it could). Submitting this now because I want access to this as I decouple the animation function handling from the render loop.
State machine can currently:
To help with testing, you can add the following to avatars.ja at line 1132, which will provide access to the state machine and avatar via the console.
you can test triggering the dummy functions by running the following in the console
Also, I'd recommend hiding white space changes when reviewing