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

Implement method of exchanging base hand animations #231

Merged
merged 1 commit into from
Nov 13, 2022

Conversation

BastiaanOlij
Copy link
Member

@BastiaanOlij BastiaanOlij commented Oct 26, 2022

This PR does some cleanup removing the old hand models in favor of the new hand models that @DigitalN8m4r3 added in, moving some things around and introduce the ability to change the base poses for the hand animations.

Cleanup

The asset folder in the plugin was meant to just hold assets (model files and graphics). The hand logic has already moved into its own folder as its a full implementation. The player body and poke scenes have been moved into a player folder.

Open and Closed hand animations

So the trick we've always been using is to blend between an open and closed hand animation depending on trigger and grip inputs. There is now an enhancement to this where you can select different animations as your open and closed hand.

@BastiaanOlij BastiaanOlij added the enhancement New feature or request label Oct 26, 2022
@BastiaanOlij BastiaanOlij added this to the 3.1.0 milestone Oct 26, 2022
@BastiaanOlij BastiaanOlij self-assigned this Oct 26, 2022
@DigitalN8m4r3
Copy link
Contributor

How do you Plan on exposing this to the the pickable Object? and vice versa as it is nice to have the ability to change the open and closed handpose bit without an interlink to grabbed objects, its no change

@Malcolmnixon
Copy link
Collaborator

The XRToolsFunctionPickup knows the controller, and could probably find the XRToolsHand under it.

The XRToolsFuncrionPickup could be expanded to look for open/closed animations attached to the packable (or its PickupCenter) and apply them to the hand when it becomes the closest.

@BastiaanOlij
Copy link
Member Author

Pretty much what @Malcolmnixon says, though I'm not sure how much we should do automatically and how much we need to leave up to the game developer as we can't really predict the many ways this could be used.

In the end, when an object is picked up you can react to that and assign the correct animation, just need a nice way to reset it when you let go

@DigitalN8m4r3
Copy link
Contributor

,...just need a nice way to reset it when you let go

From my trys previously with using blendvalues it resets just fine, no fuzz there but then again i was using animationtree blendvalues.

If we could save put the current blendvalues from a animationtree as .anim files that would give us rly Something, as so far lookin at the above u still need to make the animation pose, probably in blender and am gonna lean out the window here and say
Yes we don't need to do evrything for the potential dev User but why shouldnt we make the Best choice when godot xr could benefit from this as a xr dev platform. Making things easier = makin godot xr more attractive and competetive

@BastiaanOlij
Copy link
Member Author

@DigitalN8m4r3 indeed, the reset is just about determining the condition that you're now no longer holding something and thus need to return to the original pose. When every possible pose is in the blend tree like in your solution, that is easy. In my solution it's easy to detect that you're no longer holding something but you've lost the pose that was used before you changed it to the pose relating to the object you are holding.

So some more thought needs to go into this and I suspect that different games will require different solutions. Right now this PR limits itself to making it possible to swapping the poses out.

As for creating the poses, fully agree. Blender last night was a nightmare, while creating the poses themselves is straight forward, the UI around dope sheets is very confusing and it is very easy to make mistakes there doesn't seem to be an easy way to come back from.
Also I noticed that Blender seems to save GLTF using whatever current pose the hand is in as the rest pose, and if the rest pose doesn't match up with the one used to create the original hand we're using, the new animations are not compatible.
Finally there is the issue with legacy names. Godot used to convert the names of the bones to camel case and that is what we're using.
Turning the legacy conversion off all of a sudden we have the correct bone names, but it seems to break the blend tree logic. I couldn't figure this out so I left them on legacy.

In Godot 3 there is no viable way to create the animations by posing the hand, but in Godot 4 you can!

@DigitalN8m4r3
Copy link
Contributor

i dont feel different games/different needs.
From my pov it is limiting yourself and the enduser to use animations and havin to go thru loops such as makin those with Blender or savin those out using handtracking.
At the end this pr is about what the title says and so there is no need to downtalk this, thx for your work and it is a good start.

On a sidenote, your code is already a start when it comes to using a animationtree instead...
all that needs to be done is

Change from animation pose to animation tree, force the openfield to use a tree that got no blend values changed and the closed to use the animationtree from the pickable if present and if not fallback

Now this should happen on the fly as in loading in the resource and replacing with the current animationtree

am sure thats possible, just cant wrap my head around connecting this and loading it dynamicly

@Malcolmnixon
Copy link
Collaborator

I'm part-way through a hand-pose helper tool which attempts to build a pose-animation by recording the angles from the Quest 2 hand-tracker.

Currently it gives severely broken fingers, but I think that's just a rotation by 90 degrees issue. If it works ill polish the tool and put it up, but I'll also assemble a moderate collection of pre-made poses.

@Malcolmnixon
Copy link
Collaborator

I used the Godot 3 animation editor (https://shmolyneaux.itch.io/godot-3-skeletal-animation-add-on) recommended by @DigitalN8m4r3 to create a set of animation files and tried to set them; however it looks like there some serious cross-talk between hands and scenes - its like the script is editing the same global AnimationPlayer and AnimationTree. I think there's shared resources involved here.

The following is some of the hand poses to play with: hand poses.zip

@BastiaanOlij
Copy link
Member Author

I used the Godot 3 animation editor (https://shmolyneaux.itch.io/godot-3-skeletal-animation-add-on) recommended by @DigitalN8m4r3 to create a set of animation files and tried to set them; however it looks like there some serious cross-talk between hands and scenes - its like the script is editing the same global AnimationPlayer and AnimationTree. I think there's shared resources involved here.

Sounds like we're missing a "resource is local to scene" tickbox somewhere. Godot indeed has a habit to share resources amongst instances of scripts meaning that if you modify one, it modifies for all instances (as it's the same thing used).
Good behavior most of the times, a PITA when you don't :)

@BastiaanOlij
Copy link
Member Author

@Malcolmnixon looks like that local to scene ensures the resource is made unique, but when the resource contains subresources, those are not made unique.

I've added a deep copy of the blend tree, its only a little bit of data, and it seems to fix the issue.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review November 4, 2022 02:26
@BastiaanOlij BastiaanOlij marked this pull request as draft November 8, 2022 00:50
@BastiaanOlij
Copy link
Member Author

Changed this back to draft as I have some rework to do after the new hand models were introduced. Will get to that probably later in the week.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review November 10, 2022 11:37
@BastiaanOlij
Copy link
Member Author

Rebased this so the logic is now based on the new hand models. As the old hand models aren't compatible with the new ones I figured it's best to remove them and prevent confusion around why anim files can't be mixed and matched.

The open/close logic has now been adapted for the new hand logic.

@DigitalN8m4r3
Copy link
Contributor

Works well, gonna submit a pr to the 4.0dev branch with the new hands and then this can be ported to 4.0 as well

@Malcolmnixon
Copy link
Collaborator

Malcolmnixon commented Nov 11, 2022

I think you might want to make the XRToolsHand a tool script - everything looks compatible and I checked it out. It nicely allows the hand poses (and material changes) to be shown live. That will also help my next pull request with the grab-point tool script to show the hand in the correct pose.

I'm not sure if XRToolsPhysicsHand also needs to be made a tool script as it extends from XRToolsHand - I'm still not 100% up on the implications of tool scripts and lifetimes.

@BastiaanOlij
Copy link
Member Author

Yeah when you extend a tool script, the new script must be a tool script as well for things to keep working.

@BastiaanOlij
Copy link
Member Author

@Malcolmnixon indeed making it a tool script worked well, just needed to disable the process so we're not trying to handle controller input. So back to review :)

Copy link
Collaborator

@Malcolmnixon Malcolmnixon left a comment

Choose a reason for hiding this comment

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

This works very nicely. I'll get the grab-point stuff in on top of this soon.

@BastiaanOlij BastiaanOlij merged commit 36a3848 into GodotVR:master Nov 13, 2022
@BastiaanOlij BastiaanOlij deleted the cleanup_hands branch November 13, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants