Reworked geometric algebra. Accesses Basis via get_column. #219
Conversation
|
Tested as working here for 4.3-stable and 4.4-dev1 on both Quest2 and Quest3. Neater code - from a slightly more knowledgeable position and tidied up dev env on my side - than the now discarded #209. Should pass clang* etc too this time around. *Puts the magic into autoMAGIC that does, as far as my wee storyteller [Film / XR] brain is concerned. |
There was a problem hiding this comment.
Thanks!
The math changes look correct to me. The basis[i] referring to rows in C++ (versus columns in GDScript) is something that always gets me too. :-) Otherwise, the only other change is constructing the Z-axis of the root basis from the X and Y, as opposed to constructing the X-axis from Y and Z -- this also makes sense to me.
I tested on the Quest 3, and it seemed to be working great!
I only have a couple of notes on the comments and the moving of one line explained below.
| // 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // Set tracker pose, velocities, confidence. | ||
| // Good to go. | ||
| xr_body_tracker->set_pose("default", root, Vector3(), Vector3(), XRPose::XR_TRACKING_CONFIDENCE_HIGH); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The-Cyber-Captain
force-pushed
the
master
branch
from
3d12f8b to
1c56797
Compare
There was a problem hiding this comment.
Thanks!
The code clean up looks mostly good to me! (There's still one new comment - called out below - that I don't think adds anything and could be removed)
But I'm not sure about the change in the root origin calculation. I think it'd be best to hear what @Malcolmnixon thinks about that
| Transform3D root = Transform3D(root_x, root_y, root_z, root_o).orthonormalized(); | ||
| xr_body_tracker->set_joint_transform(XRBodyTracker::JOINT_ROOT, root); | ||
| // Set tracker pose, velocities, confidence. |
There was a problem hiding this comment.
I don't think we really need this comment either - it's not really saying anything that the line of code itself says just as well.
… to 'correct' root and shoulder-tracking bugs respectively.
Root tracker does now point 'forward'. IE +z aligns with hips & user's real-world 'forward' (whilst remaining along, and on, the XRorigin / Global XZ plane). Shoulder trackers also properly pulled back (the previously indicated 7cm) towards upper chest, more aligned with T-pose modelling.
Having now delved a bit deeper into GDE wrappers, variants exposure, etc, I'm going to speculate that basis' unit vectors are, indeed, intended to be accessed via get_column out here - as we're firmly in C++ land still - and that's what initially tripped @m4gr3d up way back when? I've done a quick search of the files in GodotVR repo and couldn't find any others which access 'basis[]' beyond this file which would need addressing.
Primarily fixes #205, & fixes #218, though leaves open discussion still re: