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

Reworked geometric algebra. Accesses Basis via get_column. #219

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -314,17 +314,22 @@ void OpenXRFbBodyTrackingExtensionWrapper::_on_process() {
// with how models are designed, rather than the Meta positions of
// the tips of the clavicles.
if (locations.isActive) {
// Construct a root under the hips pointing forwards
// IE, +z aligns with hips & user's real-world forward.
// (Remaining, however, on the XRorigin / Global XZ plane; root's basis rotated around Y to fit)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is great! However, I think we could remove all the other comments added in this PR - they don't really add much that isn't already in this comment, or clear from the line of code they are commenting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dsnopek . But would you believe it?... Was aiming to update the PR this evening (UK time) after an afternoon working on my project itself... only to discover during that afternoon session that - as I see it - the initial assertion "The Root joint carries no data, so instead place it on the ground under the hips pointing forwards" is erroneous. Or, more accurately, zeroing out the position of root seems inconsistent / illogical when the reference play area is set to LOCAL (aka XR_PLAY_AREA_SITTING).

The new Root orientation (pointing real-world forward, and parallel to real-world ground) is good, but I believe the original position should actually be maintained. This way, when switched to LOCAL, Root doesn't magically teleport up to Head, but maintains its location in tracker-space: being found where the ground would be, between the two feet. [Exactly where all the good stuff, avatar control, and skeleton modifications happen to be in my project!]

A quick-and-dirty hack locally to comment out the 'slide', then rebuild, seems to confirm this indeed works better for all play area modes. Is that also your understanding of how Root should behave, and therefore the functionality I should tidy up and include in my new commit for review?

TL;DR - New Root orientation good. But origin should be copied over from the original Tracker itself?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps. It'd be great if @Malcolmnixon could give his opinion - it's his original code that based the root position on the hips, rather than using the root tracker.


// Get the hips transform
Transform3D hips = xr_body_tracker->get_joint_transform(XRBodyTracker::JOINT_HIPS);

// Construct the root under the hips pointing forwards
Vector3 root_y = Vector3(0.0, 1.0, 0.0);
Vector3 root_z = -hips.basis[Vector3::AXIS_X].cross(root_y);
Vector3 root_x = root_y.cross(root_z);
Vector3 hips_left = hips.basis.get_column(Vector3::AXIS_X);
// flatten hips' left (x basis) on to real-world / global ground plane. Normalize.
Vector3 root_x = (hips_left.slide(Vector3(0.0, 1.0, 0.0))).normalized();
Vector3 root_z = root_x.cross(root_y);
// flatten hips' origin on to ground plane.
Vector3 root_o = hips.origin.slide(Vector3(0.0, 1.0, 0.0));
// set the actual root transform
Transform3D root = Transform3D(root_x, root_y, root_z, root_o).orthonormalized();
xr_body_tracker->set_joint_transform(XRBodyTracker::JOINT_ROOT, root);
xr_body_tracker->set_pose("default", root, Vector3(), Vector3(), XRPose::XR_TRACKING_CONFIDENCE_HIGH);

// Distance in meters to push the shoulder joints back from the
// clavicle-position to be in-line with the upper arm joints as
Expand All @@ -333,7 +338,7 @@ void OpenXRFbBodyTrackingExtensionWrapper::_on_process() {

// Deduce the shoulder offset from the upper chest transform
Transform3D upper_chest = xr_body_tracker->get_joint_transform(XRBodyTracker::JOINT_UPPER_CHEST);
Vector3 shoulder_offset = upper_chest.basis[2] * shoulder_z_offset;
Vector3 shoulder_offset = upper_chest.basis.get_column(Vector3::AXIS_Z) * shoulder_z_offset;

// Correct the left shoulder
Transform3D left_shoulder = xr_body_tracker->get_joint_transform(XRBodyTracker::JOINT_LEFT_SHOULDER);
Expand All @@ -344,6 +349,10 @@ void OpenXRFbBodyTrackingExtensionWrapper::_on_process() {
Transform3D right_shoulder = xr_body_tracker->get_joint_transform(XRBodyTracker::JOINT_RIGHT_SHOULDER);
right_shoulder.origin += shoulder_offset;
xr_body_tracker->set_joint_transform(XRBodyTracker::JOINT_RIGHT_SHOULDER, right_shoulder);

// Set tracker pose, velocities, confidence.
// Good to go.
xr_body_tracker->set_pose("default", root, Vector3(), Vector3(), XRPose::XR_TRACKING_CONFIDENCE_HIGH);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I don't like this line being moved to the end. I think it was better back up where the code calculates root. Also, like I said above, these comments don't really say anything that isn't clear from this line of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy that. I was kinda having to figure everything out from first principles - purely to rectify the broken SHOULDER joints I was experiencing - so things are / were a bit verbose, yes. Agreed.

}

// Register the XRBodyTracker if necessary
Expand Down
Loading