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

Fix XRHandModifier3D scaling #89130

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

Malcolmnixon
Copy link
Contributor

The XRHandModifier3D scaling needs to be fixed to correctly apply the Skeleton3D.motion_scale and XRServer.world_scale - this is similar to the work in #89103.

Attempting to set XRServer.world_scale before this PR would not uniformly scale the hands with the rest of the player, but would instead only stretch the bones resulting in distorted the hands with spindly fingers:

image

This PR changes the scaling as follows:

  • Joint tracking data is scaled using the Skeleton3D.motion_scale to be applied to the skeleton
  • The XRHandModifier3D is positioned at the tracked palm location and scaled with the XRServer.world_scale

The following video demonstrates the results:

XR.Hand.Scaling.mp4

There's also a test project at https://github.com/Malcolmnixon/GodotXROpenXRTracker to play with the results.

@Malcolmnixon Malcolmnixon requested a review from a team as a code owner March 4, 2024 04:25
@Malcolmnixon Malcolmnixon requested review from dsnopek and AThousandShips and removed request for a team March 4, 2024 04:25
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

lgtm, but we do need to update the class documentation to have information about these.

Especially the motion scale needs some documentation about the use case, it will always stretch the mesh in weird ways but I can see that it will have its uses.

The world scale for me is the important one which we should have in the original release. Here we should include a paragraph in the class documentation informing the user the node will set proper scaling based on world scale so they know that is expected behaviour.

@dsnopek
Copy link
Contributor

dsnopek commented Mar 4, 2024

Thanks!

Scaling the node transform by world scale makes sense to me - that looks good, and feels like something we should probably always have been doing.

I'm confused about applying the skeleton's motion scale, though, largely because I don't really understand what that property is supposed to do. It sort of sounds like it's supposed to scale the amount of movement, but that would seem to imply scaling not just the position but also the rotation? The changes in this PR basically just space the joints out more - is that really sufficient to correctly scale the amount of movement? And if the the bone update mode is set to "Rotation Only", the motion scale will not do anything at all - is that the expected behavior?

@Malcolmnixon
Copy link
Contributor Author

I'm confused about applying the skeleton's motion scale, though, largely because I don't really understand what that property is supposed to do.

The motion_scale property is documented simply as "Multiplies the 3D position track animation." and indeed that's how it's used in animation_mixer.cpp.

My guess is that it's for scaling the animation track to match the size of the skeleton. Consider some jogging animation and you want to play it on three NPCs - a dwarf, a human, and a giant. The motion_scale can be used to adjust the jogging animation positions to align with the size of the character.

We wouldn't want to motion_scale to be applied to angular rotations, as this would cause more or less joint-rotation rather than just scaling the animation.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

My guess is that it's for scaling the animation track to match the size of the skeleton. Consider some jogging animation and you want to play it on three NPCs - a dwarf, a human, and a giant. The motion_scale can be used to adjust the jogging animation positions to align with the size of the character.

Ah, ok, that makes sense! Using this interpretation of motion_scale, this PR looks good to me :-)

@Malcolmnixon Malcolmnixon force-pushed the xr-hand-scaling branch 2 times, most recently from df8758b to 515d3d4 Compare March 4, 2024 23:23
@Malcolmnixon Malcolmnixon requested a review from a team as a code owner March 4, 2024 23:23
@Malcolmnixon
Copy link
Contributor Author

lgtm, but we do need to update the class documentation to have information about these.

I believe I've captured the position and scaling information in the node documentation. Could you review?

scene/3d/xr_hand_modifier_3d.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

lgtm

@akien-mga akien-mga merged commit fcb0adf into godotengine:master Mar 6, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants