-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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 root motion to Skeleton3D to resolve conflicts between Animation and SkeletonModifier which may modify root bone transform #90361
Conversation
e2f2416
to
1905456
Compare
Thanks! I'll test it as soon as I can. However, this should also update |
I didn't realise we changed |
As for the change itself. I haven't followed the whole discussion leading up to these decisions, so I'm not coming from an informed standpoint. But I do fear very much that we've changed the system in a way that only applies to animations. With what were doing in XR, we're dealing with live tracking data. We have two scenarios that need to be supported. The first is using the data as is, this is especially important for AR type applications where we want the hand to be placed exactly where the tracking system says the hand is. In the original approach (and I'm using the original names we decided on here) we would setup up our node tree like this:
The logic would place the The second scenario is where we stray away from the position of the hand. We either stop the hand from moving through a physis object when the player punches through a wall, or we limit the hand motion based on IK for the avatar the user is using, or there is some other mechanism where we stray away from the hand position. A setup for that first example would be:
The We decided to change over to the skeleton modifier approach because we were duplicating a lot of logic here, this was all possible before the change that prompted all this, and it seems the necessities of animating skeletons outside of animations have not been taken into account in the decision making process. I hope the above gives some clarity in the use cases we're dealing with, and the question on whether these changes will mean we need to stop basing our work on other systems in Godot, or whether we can combine these in properly. It seems to me there are still things missing. |
The XR nodes (OpenXRHand, XRHandModifier3D, and XRBodyModifier3D) had two modes of operation depending on the game:
The Direct Drive is the kiddie-training-wheels version people tend to start with, but abandon it for Indirect Drive the moment they want advanced features such as hand-collision, held-object-collision, two-handed-grabbing, etc. There are a significant number of articles, videos, and games which rely on Indirect Drive approaches; and I'm concerned we may have broken a fair number of them. |
@TokageItLab If you want an environment where you can play with XR then all you need is a webcam. This video shows setting up a basic avatar in Godot, and driving it with XR Animator using a webcam. You'll note in this training video that the scene tree has the XRFaceModifier3D and XRBodyModifier3D separated from the avatar because I swap out the avatar and don't want to delete the modifier nodes: In other videos I've published I implement dynamically downloading the avatars over the internet and switching them at runtime. |
The following is a test of upgrading the Godot XR VMC Tracker repository demo project: Currently the XRBodyModifier3D nodes contain two things that want to be positioned:
It's possible to fix the warnings by moving the XRBodyModifier3D and the marker under the avatars skeleton - although this does make for a rather complex node-tree. With this updated node structure we get the following behavior:
GodotXRVMCTracker.Bug.mp4 |
The following is a video testing a few different avatars (AvaturnMe, Mixamo, ReadyPlayerMe, and VRM) with Godot MainLine and this patch. It looks like the SkeletonModifier3D changes also broke the VRM 3D Avatars asset based on how the anime characters hair and dress are being torn off the body. Untitled.Project.mp4UPDATE: The other thing to note is that while the VRM Avatar (with the messed up meshes) can move with this patch; the other three avatars are still stationary. I suspect it's because only the VRM avatar has a "Root" bone - The other three avatars have "Hips" as the root bone. The XRBodyModifier3D used to apply "root motion" to the avatar via the Node3D transform because it's not guaranteed that there's a root bone. |
How would it work if the The corruption of VRM avatars should be caused by VRMSpringBone's use of bone_pose_override. Since bone_pose_override was left in place for compatibility, it should work if only bone_pose_override is used, but it is expected to break if SkeletonModifier is used at the same time. We plan to rework VRMSpringBone as a SkeletonModifier. @BastiaanOlij First of all it was quite problematic that some potential modifier Nodes were running set_bone_pose() outside of the animation system in the past. At the very least it should have used set_bone_global_pose_override(). Because it does not have a mechanism like reset_on_save, it can easily rewrite the actual values of the .tscn. Also the process order issue, as I pointed out in SkeletonModifierPR, but unfortunately, this may not have been taken into account when the design of XRNode was started. With SkeletonModifier's PR, the value is now not reflected in the actual scene as long as set_bone_pose() is executed in SkeletonModifier::_process_modification(). This PR attempts to do the same for the root motion, but since the root motion needs to be reflected in the actual scene, so Gizmo has a virtual world to allow previewing. Also, as @lyuma and someone have expressed concern, it may be possible to make the However, in any case, now the future may modify the bones (and Skeleton's transform) must be done as SkeletonModifier which is a direct child of Skeleton, and if they are currently developing something that does not conform to this, they should redesign it to conform to SkeletonModifier's design. |
The root problem of this works both ways, as I think those focused on the requirements of the animation system have a blind spot in knowing the implications of real time tracking, so do we on the XR side have a blind spot in understanding the needs and requirements for the animation system. I did not come into this conversation until everything was merged, so I'm still playing catchup to the discussions had on your PR. My main concern here is that while XR nodes were altered, and their behaviour altered, this was done without proper consultation with knowledge holders and thus we didn't come up with a solution that works for everyone involved. From what I've been told, the change that broke the camels back, were only done at the end though possibly planned for awhile. Now that this breakage is leaving several people in a position that they can't continue their work until things are fixed, we're starting the conversation that should have been had. Also something that puzzles me. How would I know that There is also an issue that this logic, while now being used in a broader sense now that we have extended this logic to do body and face tracking, and made the system available outside of OpenXR, that this originates from Godot 3 days and has been ported over, so there are decisions made that predate some of the newer functionality in the animation system. I think we have a broader problem here that we're not good at commenting code/adding design information to our documentation and explaining more about why we do what we do, instead of what it does. Only the latter can be distilled from reading the code, while the first is what is missing. That's a broader problem and we in the XR team are just as guilty of this, as there would be very little in the code that would inform you that these changes would cause the issues they have. |
No, the one with the suffix "override" was safe. The value of pose_override was to override the pose without changing the actual bone pose property. So it is not change .tscn. However, this design was quite hack and also did not resolve the processing order of the bone deformation. Since the user only wanted to use the global pose, it was confusing without knowing what it was supposed to do just as even you are. This is why we just recently put a deprecated flag on override to use process modification. These issues and proper usage of override were never documented, so only a few people who developed IK and other modifiers had detailed knowledge of them. It would have been unfortunate that this was not shared with the XR team. Godot 4 deprecated them, but in Godot 3, in particular, an attempt was made to implement a ModificationStack, to which a local pose override was also added, making the code a nightmare. I am organizing the documentation of the animation API for 4.3 as most APIs have finally begun to enter a stable phase recently, and it is unfortunate that XRModifier is also being developed for 4.3 so that it conflicts with it. Anyway, I have wrote the general SkeletonAPI process flow and how to get/set a modified pose in the Modifier PR description. If they have any questions about the implementation, please ask in the Animation channel of RocketChat or on the v-sekai server of Discord. |
@TokageItLab as with everything, it all has a history to get where we need to go :) The important thing is that we're getting the right people together now to talk about the issues, and hopefully to create a better understanding within both teams what each others requirements are. My main concern remains the move that The nice thing about having it as the root was that we had a node that is positioned in the right place to add other mechanisms too (including, like I previously mentioned being able to effect this location through physics) while the skeleton remained updated in local space. This also allows us to enforce the XR nodes to be a child of But I also get having it as a child node and it being more descriptive and removing the need for an arbitrary property. There is an alternative option of not updating, or optionally updating the root motion of the skeleton, and moving the placement code to an XRAnchor3D node. That will make it less monkey proof for XR users however.. |
The modifier has to be a child, but the node itself doesn't. what I am working on doing in vrm is that the vrm_secondary node can be anywhere, and it manages its own internal SkeletonModifier3d child of Skeleton3d For some nodes that might be ok, but I suspect for the high level body tracking nodes that the user will want to update some things after the body tracking takes effect, so the only internal mode that can be used is FRONT, but that leaves the user little ability to control the update order granularity or tweak the weight of the modifier (in case they want to smoothly shift from body tracking to animation). Though this can be mitigated by exposing your own weight property in the XR nodes and forwarding the weight to the internal modifier node |
We will not go into detail implementation until we know which direction to go, but I have made some changes based on lyuma's feedback. For now, I changed the "Apply Root Motion" property to a "Root Motion Target" property that allows specifying a Node3D such as a Skeleton parent as the target to which the root motion is applied. Also, the "root_motion_processed" signal now sends |
b6e1782
to
998b1d8
Compare
The latest patch seems to work quite nicely - especially with the Godot VRM patch to make many-bone-IK work with SkeletonModifier3D. RootMotionPatch.mp4By default only the VRM character can move due to being the only one with a Root bone. The video above enabled "Use Root Motion" on all four XRBodyModifier3D nodes (including the VRM character). |
We do seem to have lost XRServer.world_scale control for the driven avatar. Pre SkeletonModifier3D this would be included in the XRBodyModifier3D transform resulting in all child objects including the avatar being scaled: // Transform to the tracking data root pose. This also applies the XR world-scale to allow
// scaling the avatars mesh and skeleton appropriately (if they are child nodes).
set_transform(
transforms[XRBodyTracker::JOINT_ROOT] * ws); This scale which is applied to all XR-driven systems (including headset IPD) is required for:
UPDATE: In discussing with Bastiaan we may be moving towards deprecating |
There's a problem with how root motion is applied. The following video shows a mocap recording of a user running back and forth; however the avatar keeps running away forever: 2024-04-09.22-21-26.mp4This error also highlights the fact that the target nodes position is updated: node->set_position(node->get_position() + root_motion_position); As such even after the transform order is fixed we may end up with numerical integration errors which will result in character drift over time. |
I am still not clear on this, but are you saying that there are other modules that need to use world scale? If so, I think the right way would be make each module to use something like XRServer::get_world_scale() to retrieve it from singleton.
This is the reason why I say in above I am not sure if my calculations are correct. Perhaps the order of applying rotation and translation is different in the glTF animation and OpenXR tracking data, and this needs to be corrected.
My guess is that the drift is not due to tracker shake, but due to the timing of the application of world scale or something in XRModifier that has caused an error with the actual values (means the drift must be a simple miscalculation on my part due to not knowing what values the OpenXR tracker data is). If all those calculations are consistent and the drift occurs, then I think there needs to be some sort of threshold in the XRModifier. |
232e8b8
to
c0bb42f
Compare
I have tried to fix the calculation with the problems you have pointed out.
However, I still don't know if this is correct, so I may need to have @Malcolmnixon take over as soon as we have a clear direction for about root motion implementation. |
In discussing with @BastiaanOlij we're going to hold off on XRServer.world_scale and possibly deprecate it.
I'll see what I can come up with. I think it's a missing (or extra applied) rotation - the mocap-actor is always walking forwards (in the direction they're facing) and I think that's being translated to the skeleton root always moving in a positive direction.
It's more that we're effectively doing |
This was just a test to show the tracking data is valid and there is a simple way to apply it which achieves working results. It's interesting you say "is not relative and should not be done" because relative isn't really important for this type of work - we can get multiple sources of tracking information (headset, controllers, hands, body, trackers strapped on the body, trackers strapped onto props, etc.) that are all in the same XR coordinate space. We actually want to position all of these objects in the same space - their local transforms being the exact values coming out of the tracking data. If we want to move the entire space around we'd have all these nodes childed to some "stage" Node3D and then move the Node3D around dragging everything else - this stage would have to be synchronized with the XROrigin3D if we're doing VR so that the camera moves with the body. |
If the root motion of another Modifier or AnimationMixer is relative (at least AnimationMixer has historically been relative) and only the XRModifier sets an absolute value, it will completely override the other Modifiers and break FK/IK blending. For example, if you have an animation that moves from the origin, if you adopt an absolute value for the root motion, the node will move to the origin every time the animation is played, and the root motion will not do what it is supposed to do. So the root motion must be relative, not just a deformation applied to the root node. In theory, it is simply a matter of calculating the delta of the frames before and after the tracker and adding or multiplying them. If world scale is obsolete and the accumulation of decimal precision errors is to be ignored, then the only rest issue here now is how to calculate the delta of the root bone rotation value. |
0ca1906
to
7b41b8a
Compare
@Malcolmnixon I thought there seemed to be a problem with the conversion about the xform of translations, so I changed it. How is it now? skeleton->set_root_motion_position((current_q.inverse() * node->get_quaternion() * delta_q).xform(transforms[XRBodyTracker::JOINT_ROOT].origin - prev_root_bone_position)); to skeleton->set_root_motion_position(current_q.xform(transforms[XRBodyTracker::JOINT_ROOT].origin - prev_root_bone_position)); If it doesn't work, I would like you to try to calculate the direction in which the translations will be correct. |
7b41b8a
to
14e4489
Compare
14e4489
to
72c59df
Compare
I fix XRBodyModifier direction of translation as @Malcolmnixon told me about |
Quote @TokageItLab in the above example, the SkeletonIK3D nodes require a target for IK tracking. What node path would be used in that example? We can't require Skeleton3D to be a direct child of XROrigin. Indirect child may be possible but it would carry the implication that the character's position is the center of my play area which is both arbitrary and restrictive. Overall, this requirement would lead to major compat breakage across asset import and networking. Originally posted by @lyuma in #90645 (comment) |
Is there a reason not to add the @TokageItLab: To respond to something you wrote on the other issue:
The To relate that to your example:
The point of
Due to my lack of experience with trying to use it in this context, I don't know the best way to replace that with root motion. Maybe we could keep the hands or avatar outside the Alternatively, maybe the hands and avatar stay as children of the To a large degree, I think we're going to need to implement this, and then take some time to play with it and experiment, in order to find the best way to actually use it. |
@TokageItLab It seems my comment above was cross-posted with a new comment from you on the other issue :-) To attempt to respond to part of that:
By "XR Modules", do you mean the However, the
I think there are valid approaches that don't have the skeleton as a child of the
Hm, I'm not sure I understand this one. The tracking data that comes from There is a use case for forcing the player to stay in the same place: in some games if you attempt to use your real world motion to walk into a virtual wall, it will apply the inverse motion to the tracking origin, so that you can't walk into the virtual wall in virtual space. But in most cases, you wouldn't want to move the
Yep!
I'm not totally sure about the use case where the
Hm, again, I don't think we want the skeleton moving |
After discussing this with @lyuma at this stage, I think it is possible to postpone this PR over to 4.4 without rushing it for now. First, the purpose of this PR was to remove it so that XRModifier would not implement the wrong move application to core. If that is true, then I understand that after the PR of #90645, the XR Module has no role in moving the Node and only provides the value to the user. I see no problem with that since the value can then be modified and used at the user's option. There is no need to rush this PR. I want to make this most clear:
I don't know if HandModifier can have the movement for that position. Or it could be possible to extract the movement value from the XR camera as well. So, the idea I have now is to add a new SkeletonModifier to extract the Root Motion, which would have the name XRRootMotionModifier. It would be able to select which Tracker or Camera to extract the Root Motion from. Although it would replace this PR. |
@dsnopek 2, 2-1, 2-3, 2-4 are all the same, saying that it is not wanted to make Skeleton a child of XROrigin. Then, I do not understand why 2-2 and the rest are different. Both have the same relative and world coordinates, right? After moving XROrigin, is there some internal transformation that initializes XROrigin's coordinates to 0 for the next frame? --If it is doing such internal coordinate processing, it would make sense that there would be a parent-child relationship, but this would violate XRModule's policy of not moving Nodes, so which is never happen. My understanding is that the reference point for tracking is kept in the reference_frame of the XRServer at the time of calibration, so only the relative coordinates between it and XROrigin are important. At this point, the parent-child relationship between the Skeleton and XROrigin is not important, only that the coordinates match, is it? Edited: |
Yes, I think you could have the skeleton and the My comments on 2.1, 2.3 and 2.4 are about having the However, if the
No, there isn't internal coordinate processing as you're describing it. But its position and rotation are used to map the real world coordinates to the virtual coordinates. So, moving the I'll attempt to explain it in a simplified way: the tracking data says the player's head is at (2.5m, 2m, -3m) in the real world. By placing the We don't generally move the Does that make sense? |
@dsnopek Let me ask an additional question: does XROrigin use Global coordinates? In other words, does tracking work when the XROrigin's parent Node (like PlayerContainer) is moved? This implies that questions 2-3 in the list of questions will work or not. |
Yes |
We have numerous XR Nodes that move based on tracking data, and are often used to contain (for moving) other nodes. Developers will place these under XROrigin3D then attach nodes under them with the assumption that the nodes and all their children will move around in the 3D game-space as the player moves:
That is correct. In fact for multi-player network games it's may be better to treat the entire XROrigin3D part of the scene-tree as the local players "input system" which drives (via script or RemoteTransform3D) network-shared objects kept in a separate part of the scene-tree (synchronized with MultiplayerSynchronizer / MultiplayerSpawner).
These questions seem to imply that player physical movement causes the XROrigin3D to move. There are instead two ways to compose the XR scenes: Games are free to pick either approach based on the preferences of the development team and the problems being solved; and they result in wildly different scene hierarchies and behavior for the XROrigin3D node, and also affect the answers to these four questions. |
@TokageItLab Just moving this reaction here as David requested
Ah my bad, yes indeed the MeshInstance is under the Skeleton. My mistake for quickly putting this together from memory :) I think the scene tree you drew up here will get close to what we need.
That goes very much against the principles of Godot, Godot has always taken an approach to hide as much complexity as possible from those developing with Godot. It was one of the main design criteria Juan required when we added XR support to Godot, the less users needed to know about how XR works, the better. This does not just apply to XR but is also permutated in much of the rendering design where all the complexity is hidden away. So I am somewhat surprised by that statement. I'm hoping we just misunderstand the intention of this here.
This to me is a horrible idea, the nodes should do a good job in being directly usable (within reason), not require complex setup tools. Yes when logic is highly dependent on the type of games being made, the nodes should only do so much and the rest should be in code. We need to be pragmatic for sure, there are definitely situations this simply isn't possible (XR has a few). But in the end GDScript is meant as a control language, and as much functionality should be handled by nodes and/or composition, not by layers of code written by tools or by the user which requires more understanding of the user, and runs the risk of breaking with every version of Godot that nudges the direction and makes the user code outdated. |
@BastiaanOlij #90645 (comment) was said as an argument against the past XRNode's strong reliance on Node parent-child relationships, and opposed to XROrigin forcing a Skeleton to be its child. Skeletons can have quite a variety of NodeTree forms, so it was necessary to avoid XRModule forcing them to do so. Now that I understand that they can be siblings, as I confirmed with @dsnopek and @Malcolmnixon above, so now the problem with this should be eliminated; If you want to add a new controller without destroying the existing project's SceneTree, it should be a sibling or child of the controlling object. If you want Nodes to take many different forms, then the solution of limiting the forms of the NodeTree that users can take by enforcing parent-child attachment is not good way, and if you believe past OpenXR Node design is better, your argument has a discrepancy. Now that the relationship between XROrigin and Skeleton is separate and free (as long as the global coordinates match), we can build a much wider variety of SceneTrees.
I still believe it is useful to setup XR for some use cases. If you understand the roles of all the Nodes well, it is not too difficult to set up manually, but if we have a large number of modules, it will be difficult for beginners to make use of them. This is a just sample preset, and moreover, the Editor does not determine the behavior of every Node. Well although, it may be enough to distribute a sample project. |
I'm late to the party here as I've only recently begun digging into Godot's animation system. I apologize if this has already been debated or if this is the wrong place, but it is related to the topic here. I'm wondering if there has been any discussion or investigation of having SkeletonModifier3D implemented as AnimationNodes instead of scene nodes? This would allow for custom manipulation and blending of poses and root motion alongside animation. I'm not sure if this would help to alleviate any of the problems being discussed here aside from simplifying the scene tree, but there are a couple other scenarios I imagine would have similar difficulties. For example, what if you had various states for a character that required different IK setups that you needed to transition between? What pattern is advised for transitioning between them using scene nodes? I could be wrong, but it appears you'd have to add a custom blending solution per use case. |
@BaseMale SkeletonModifier always has an Influence property that adjusts the amount of blending, so you can simply key the Influence value into the Animation and blend in the such case where you want to change the amount of IK blending depending on the AnimationTree state. We agreed that this type of post-processing bone effect, only serial blending should be sufficient for the use case; In other words, it cannot blend in parallel as the equal way as AnimationNode, so it does not support such as blending different IKs that reference different poses (means to create multiple poses with different IKs before blending them), but it is very niche and complex use cases. |
Memo: Probably this will be implemented in a form like XRRootMotionModifier, but different from BodyModifier. The reason is that it would be necessary to be able to choose whether the tracker for body movement is by BodyTracker or by HMD. Also, if root motion is not needed, the user just opts out of adding that modifier. |
I began to think that this should be implemented as RootMotionProcesser extends SkeletonModifier. Including the gizmo in the Skeleton would make it cumbersome to control its display. So, adding a SkeletonModifier only when RootMotion is used would solve this problem, and would allow other modifiers to handle root bone moves at arbitrary times. We will rework it later as a new PR. |
Closes godotengine/godot-proposals#9470
It is still experimental. It is debatable how to get Gizmo out and whether this is a good way to move Skeleton.
Skeleton3D only exposes the setter
set_root_motion()
to the core. This is because it is impossible to process the root motion by looking only at the Skeleton3D root bone since it must be processed by an AnimationMixer external to Skeleton3D in order to take the root motion of the loop animation into account.It needs to be processed in the deferred update process of skeleton as well as pose. This means that if you want to manually apply root motion to any Node via get_root_motion(), you must subscribe to the
root_motion_processed
signal (isroot_motion_fixed
better naming?).If the
Apply Root Motion
option is enabled, the root motion is added to the Skeleton3D transform at runtime, see Class reference.The root motion of the AnimationMixer and the root motion of the Skeleton3D are different values, because the rotation of the accumulator and the actual object must be taken into account, as done in #72931. In other words, Skeleton3D's root motion is an easily applicable value.
@BastiaanOlij @Malcolmnixon @dsnopek See if this is helpful and useful for XRBodyModifier's root transformation.
If the use root motion option is disabled, it should move the root bone, and if enabled, it should set delta as the root motion to the skeleton.
However, I do not have the XR debugging environment, so I do not know if the calculations are correct. Especially, I don't know which is processed first in the OpenXR tracker, the rotation or the translation (whether the root translation takes the rotation into account or not).