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 XR Face Tracking support #88312

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

Malcolmnixon
Copy link
Contributor

@Malcolmnixon Malcolmnixon commented Feb 14, 2024

This PR implements godotengine/godot-proposals#9078 by:

  • Adding an XRFaceTracker Object (containing face-tracking blend shape weights)
  • Adding XRFaceTracker3D Node (to drive a face mesh)

The XRFaceTracker type defines a standard Godot definition of face-tracking data based on the Unified Expressions standard. Different providers (OpenXR-Meta, OpenXR-HTC, VMC, MoCap, etc.) can map their custom data formats into this standard, and then consumers such as XRFaceTracker3D (or custom game logic) can act on this standardized data.

The following demonstration video shows a face being driven by an OpenXR XRFaceTracker.

2024-02-18.21-21-04.mp4

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.

Thanks!

Overall, this looks really nice to me :-)

Does XRFaceTrackingProvider::update and XRFaceTrackingProvider::_update need to exist, or should the updating just be done by the process() of the OpenXR extension.

Whether or not we'll need the update() method depends on if we have any timing issues, ie will the code that updates the face tracking data run before or after we need to access it for rendering. If it runs after, that means face tracking data will be one frame delayed.

So, I'd probably lean towards keeping the update() method. It's possible we may not need it in the case of OpenXR (the process on the OpenXR extension may be sufficient), but since this is API agnostic and we may end up with WebXR or Apple Vision implementations at some point, I think it's safer to keep it around.

scene/3d/xr_face_tracker.h Outdated Show resolved Hide resolved
servers/xr_server.cpp Outdated Show resolved Hide resolved
@Malcolmnixon Malcolmnixon marked this pull request as ready for review February 17, 2024 01:54
@Malcolmnixon Malcolmnixon requested review from a team as code owners February 17, 2024 01:54
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.

Feedback from discussions on contributors chat.

  • Suggest to rename the data object XRFaceTracker and drop the _update method. This brings it in line with XRPositionalTracker approach for controllers.
  • Suggest to rename the node to XRFaceTracker3D to be in line with other 3D nodes and set it apart from the data object.
  • Remove the 'Provider' suffix from the methods added to XRServer

@Malcolmnixon Malcolmnixon force-pushed the face-tracker-provider branch 3 times, most recently from 47f6752 to 03d941f Compare February 17, 2024 04:55
@fire fire requested a review from a team February 17, 2024 06:46
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.

Looks great! Maybe update the original description of the PR to match the latest decisions.

servers/xr_server.cpp Outdated Show resolved Hide resolved
scene/3d/xr_face_tracker_3d.h Outdated Show resolved Hide resolved
servers/xr_server.cpp Outdated Show resolved Hide resolved
servers/xr_server.cpp Outdated Show resolved Hide resolved
servers/xr_server.cpp Outdated Show resolved Hide resolved
servers/xr_server.cpp Outdated Show resolved Hide resolved
@dsnopek
Copy link
Contributor

dsnopek commented Feb 17, 2024

Suggest to rename the data object XRFaceTracker and drop the _update method. This brings it in line with XRPositionalTracker approach for controllers.

Does this assume that face tracker data will be updated by XRInterface::process() or earlier? That's the assumption with positional tracker data.

We need some way (even if it's just convention) to know that we're updating the tracker data before it'll be used. For "remote" face tracking data, we could even register an XRInterface since the API does allow for multiple interfaces to be active as noted in the comment in XRServer::_process():

void XRServer::_process() {
	// called from our main game loop before we handle physics and game logic
	// note that we can have multiple interfaces active if we have interfaces that purely handle tracking

	// process all active interfaces
	for (int i = 0; i < interfaces.size(); i++) {
		if (!interfaces[i].is_valid()) {
			// ignore, not a valid reference
		} else if (interfaces[i]->is_initialized()) {
			interfaces.write[i]->process();
		};
	};
};

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 17, 2024
@Malcolmnixon
Copy link
Contributor Author

I modified XRServer.get_face_tracker_paths() : PackedStringArray to instead be XRServer.get_face_trackers() : Dictionary. This aligns better with the existing XRServer.get_trackers() : Dictionary method used for positional trackers.

Similar to the other method, this one returns a dictionary of StringName to XRFaceTracker allowing the caller to check for existence, enumerate, and get the tracker instances all using the result of this function.

@Malcolmnixon
Copy link
Contributor Author

Malcolmnixon commented Feb 17, 2024

I have a question about naming conventions. This PR introduces two classes:

  • XRFaceTracker - a RefCounted data object containing tracked face blend shape weights
  • XRFaceTracker3D - a Node used to drive a MeshInstance3D face from the blend shape weights

The important point is that XRFaceTracker3D is NOT a Node3D as it needs no spatial location. It has the 3D name because:

  1. It drives a MeshInstance3D
  2. Removing the 3D would cause a name-collision with the XRFaceTracker object

Is this acceptable, or is it a violation of a naming standard where the '3D' postfix of a node implies it inherits from Node3D?

@fire
Copy link
Member

fire commented Feb 17, 2024

Is this the first time a '3D' postfix of a node does not inherits from Node3D? That suggest maybe a different name would be better.

@dsnopek
Copy link
Contributor

dsnopek commented Feb 17, 2024

Maybe:

  • XRFaceTracker for the RefCounted data object, because it's tracking data
  • XRFace for the Node, because it's not tracking, it's rendering the face?

doc/classes/XRFaceTracker.xml Outdated Show resolved Hide resolved
Dilates the left eye pupil.
</constant>
<constant name="FT_EYE_CONSTRICT_RIGHT" value="16" enum="BlendShapeEntry">
Constricts the right eye pupil.
Copy link
Contributor

Choose a reason for hiding this comment

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

May be more accurate but I did have to look up what it meant as a non-native English speaker.

Suggested change
Constricts the right eye pupil.
Contracts the right eye pupil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The names and descriptions come from the Unified Expressions standard. They actually put pictures showing the side-effect on meshes because facial distortions and the associated terminology are a specialized subject.

That website does have hover-over popups for each blend-shape with corresponding anatomical information and enhanced descriptions. Perhaps using the extended descriptions may be more useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being more detailed here, since there are no images would definitely be useful. But I also do believe, wherever the description comes from, it can be "toned down" a bit for most readers.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Mickeon I'm kinda with Malcolm on this one. The descriptions come directly from the standard and we refer to the standard. I don't think we should be re-inventing the wheel here.

People who work on this stuff will be looking at the unified expression standard and documentation around this, or be already familiar with the standard. If we suddenly deviate that is only going to lead to extra confusion.

Getting people up and running who aren't familiar here, there is a lot more information to disseminate. including a lot of stuff around modeling and how to setup the blend shapes in the first place. That is more tutorial grounds and could use a nice step by step write down in the manual, or a good video tutorial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The standard is maintained in Github at https://github.com/VRCFaceTracking/docs and the author has requested feedback and PR improvements. Possibly feeding improvements through the standard might be the better approach as Godot is adopting this standard.

I am concerned about creating too many URL links to the standard as it would put us at the mercy of URL path changes; and I'm not sure we have the right to copy the images.

Constricts the right eye pupil.
</constant>
<constant name="FT_EYE_CONSTRICT_LEFT" value="17" enum="BlendShapeEntry">
Constricts the left eye pupil.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Constricts the left eye pupil.
Contracts the left eye pupil.

Comment on lines +234 to +245
<constant name="FT_MOUTH_UPPER_RIGHT" value="66" enum="BlendShapeEntry">
Moves upper lip right.
</constant>
<constant name="FT_MOUTH_UPPER_LEFT" value="67" enum="BlendShapeEntry">
Moves upper lip left.
</constant>
<constant name="FT_MOUTH_LOWER_RIGHT" value="68" enum="BlendShapeEntry">
Moves lower lip right.
</constant>
<constant name="FT_MOUTH_LOWER_LEFT" value="69" enum="BlendShapeEntry">
Moves lower lip left.
</constant>
Copy link
Contributor

Choose a reason for hiding this comment

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

These do not feel explanatory enough. Not a VR expert and I'm not sure what they are referring to, indeed.

Left lip corner pulls diagonally up and out.
</constant>
<constant name="FT_MOUTH_CORNER_SLANT_RIGHT" value="72" enum="BlendShapeEntry">
Right corner lip slants up.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe "tilt" is a simpler yet equivalent word? "lean"?

Suggested change
Right corner lip slants up.
Right corner lip tilts up.

Tongue arches up then down inside the mouth.
</constant>
<constant name="FT_TONGUE_CURL_UP" value="93" enum="BlendShapeEntry">
Tongue arches down then up inside the mouth.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Tongue arches down then up inside the mouth.
Tongue bends down then up inside the mouth.

Inner mouth throat closes.
</constant>
<constant name="FT_THROAT_SWALLOW" value="99" enum="BlendShapeEntry">
The Adam's apple visibly swallows.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah so that's how it's called. This PR is turning into a human biology lesson for me.

Suggested change
The Adam's apple visibly swallows.
The neck's Adam's apple visibly swallows.

Dilates both pupils.
</constant>
<constant name="FT_EYE_CONSTRICT" value="106" enum="BlendShapeEntry">
Constricts both pupils.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Constricts both pupils.
Contracts both pupils.

Mouth stretches.
</constant>
<constant name="FT_MOUTH_DIMPLE" value="140" enum="BlendShapeEntry">
Lip corners dimple.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure on what this means exactly at a glance.

doc/classes/XRFaceTracker.xml Outdated Show resolved Hide resolved
doc/classes/XRServer.xml Outdated Show resolved Hide resolved
doc/classes/XRFaceTracker3D.xml Outdated Show resolved Hide resolved
@Malcolmnixon
Copy link
Contributor Author

After discussing with @BastiaanOlij we decided to rename XRFaceTracker3D to XRFaceModifier3D to better align with the upcoming SkeletonModifier3D. Additionally it now extends from Node3D so developers can use this node in to hold the mesh if desired.

@akien-mga akien-mga merged commit aa7ac13 into godotengine:master Feb 19, 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.

8 participants