Skip to content

Detached controllers implementation #2818

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

yurec0977
Copy link

@yurec0977 yurec0977 commented May 5, 2025

We have a VR project — a machine gun simulator — where we use a real physical model of a machine gun. Our goal is to track where the user is aiming with the real machine gun and transfer that data into the virtual world. At the same time, we also need to track the user’s hands so that the player can see their virtual hands and accurately grab the machine gun handles.

To achieve this in Unity3D, we had to treat the VR controllers as trackers, and then extract tracking data from them in Unity accordingly.

It is clear pull request instead previous one #2794 which has some master merge issues

Yurii Seredovych added 2 commits May 5, 2025 15:15
This gives possibility to track real world object and hands in the same time. As example to track aiming of stationary placed real world gun by attached controller to it. And in the same time still have hands skeletones in game.
Use controllers as any fake tracker on server side
@zmerp
Copy link
Member

zmerp commented May 6, 2025

You can still fix the other PR, you just need to rebase your branch. Just run git rebase master making sure you have your branch checked out and you fetched the latest changed from upstream. If you want to continue on this PR please copy the info from the top post of the previous PR into this one.

@zmerp
Copy link
Member

zmerp commented May 6, 2025

You still need to fix the formatting and some lints so please follow up on that

@yurec0977
Copy link
Author

yurec0977 commented May 6, 2025

Is there any way to see what exactly is wrong in cpp files? For rust scripts console gives me nice instructions for problems? but for cpp files just fail without instructions(((

P.S. I've found some formatting issues in my changes, but seems there still a lot of issues not related to my changes : Some mappings in Path.cpp looks not pass format check.
Creating some body providers are not ok.

If you want I could fix some of that problem, but not sure if it is good idea to do this fixes in not PR related places

@zmerp
Copy link
Member

zmerp commented May 7, 2025

Please run cargo xtask format. This is our custom command to automatically format the whole codebase, both Rust and c++. Sorry I haven't mentioned it before; we should say this in the PR template

@yurec0977
Copy link
Author

So strange I've done cargo xtask format. And localy cargo xtask check-format is success. But here it failed

@zmerp
Copy link
Member

zmerp commented May 7, 2025

xtask format uses clang-format internally. Maybe it's not finding the .clang-format file. are you running the command from the repository root?

@yurec0977
Copy link
Author

yurec0977 commented May 7, 2025

Yes I used it from the root. And file which failed here was modified by format command. And as I mentioned localy "check-format" success after format command

@zmerp
Copy link
Member

zmerp commented May 8, 2025

Look in the source code, run the clang format command yourself checking that it picks up the config file

@yurec0977
Copy link
Author

Could you please double-check locally what's going wrong? I'm having trouble understanding the issue on my side.

Running cargo xtask format formats the files as expected, so it seems like the config file is being picked up correctly.
Also, cargo xtask check-format passes successfully after formatting — which makes me think everything is working as intended.

@zmerp zmerp force-pushed the Detached-controllers-implementation branch from d0b307f to 604981f Compare May 8, 2025 09:24
@zmerp
Copy link
Member

zmerp commented May 8, 2025

Passes the check for me after I ran cargo xtask format. Your system is configured wrong somehow

@zmerp
Copy link
Member

zmerp commented May 8, 2025

@yurec0977 sorry to tell you this so late but I remembered I already had a detached controllers branch, "poll-detached". I can work on integrating that into this PR

@zmerp
Copy link
Member

zmerp commented May 9, 2025

@yurec0977 Could you help checking that this PR still works after my changes?

@zmerp zmerp force-pushed the Detached-controllers-implementation branch from 8351431 to 955bcca Compare May 9, 2025 04:34
@yurec0977
Copy link
Author

Of course, I'll check it today.

Comment on lines +222 to +230
if let Some(motion) = &ffi_left_detached_controller_motion {
motion
} else {
ptr::null()
},
if let Some(motion) = &ffi_right_detached_controller_motion {
motion
} else {
ptr::null()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like it can just be a .unwrap_or()

Copy link
Author

Choose a reason for hiding this comment

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

I am not Rust or c++ expert. Just take as example set for controllerMotion and handSkeleton in hand data.

Comment on lines 103 to 106
pub use_left_controller_as_fake_tracker: bool,
pub use_right_controller_as_fake_tracker: bool,
pub left_controller_as_fake_tracker_binding: i32,
pub right_controller_as_fake_tracker_binding: i32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

4 fields for this seems like a lot, maybe the bools can be merged as essentially being an off enum option, tho idk if that's actually a good idea

Copy link
Author

Choose a reason for hiding this comment

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

Sounds nice. Removed bool variables

Comment on lines +251 to +274

uint64_t GetFakeTrackerBindingID(int settingsBinding) {
switch (settingsBinding) {
case 0:
return BODY_CHEST_ID;
case 1:
return BODY_HIPS_ID;
case 2:
return BODY_LEFT_ELBOW_ID;
case 3:
return BODY_RIGHT_ELBOW_ID;
case 4:
return BODY_LEFT_KNEE_ID;
case 5:
return BODY_LEFT_FOOT_ID;
case 6:
return BODY_RIGHT_KNEE_ID;
case 7:
return BODY_RIGHT_FOOT_ID;

default:
return (uint64_t)0;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should really be handled by setting the actual enum values in rust and c++ and then just using a cast

Copy link
Author

Choose a reason for hiding this comment

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

As I mentioned earlier I am not Rust or C++ expert, so have no idea, how to do it. Could you please tell me if there is any example in project?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I made the mistake of forgetting that those aren't actually enums but rather ids so what I suggested was indeed bogus, sorry for the confusion. But what I'd prefer is to just do the conversion on the rust side since the ids are also accessible there and directly put the id into the openvr config.

@zmerp since these are hashes is that fine? The openvr config is always regenerated so it shouldn't be an issue but idk.

Tho that would also require unmerging the bools and ids, but that doesn't matter, the openvr config struct is already such a mess that it doesn't really matter.

Copy link
Member

Choose a reason for hiding this comment

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

If we have hash IDs then there shouldn't be the need to convert to ordered numbers. @yurec0977 send back the ID through openvr_config if necessary, this function is not needed

Copy link
Author

Choose a reason for hiding this comment

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

Seems that config doesn't support uint64_t. When I rework it to use hash IDs and try to get it from config, build streamer failed.
m_rightControllerAsFakeTrackerBinding
= config.get("right_controller_as_fake_tracker_binding").get<uint64_t>();

Copy link
Author

Choose a reason for hiding this comment

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

@zmerp Could you help me with it? Meybe there is some example how to do it?

@yurec0977
Copy link
Author

@yurec0977 Could you help checking that this PR still works after my changes?

@zmerp It works as expected.

Comment on lines +251 to +274

uint64_t GetFakeTrackerBindingID(int settingsBinding) {
switch (settingsBinding) {
case 0:
return BODY_CHEST_ID;
case 1:
return BODY_HIPS_ID;
case 2:
return BODY_LEFT_ELBOW_ID;
case 3:
return BODY_RIGHT_ELBOW_ID;
case 4:
return BODY_LEFT_KNEE_ID;
case 5:
return BODY_LEFT_FOOT_ID;
case 6:
return BODY_RIGHT_KNEE_ID;
case 7:
return BODY_RIGHT_FOOT_ID;

default:
return (uint64_t)0;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I made the mistake of forgetting that those aren't actually enums but rather ids so what I suggested was indeed bogus, sorry for the confusion. But what I'd prefer is to just do the conversion on the rust side since the ids are also accessible there and directly put the id into the openvr config.

@zmerp since these are hashes is that fine? The openvr config is always regenerated so it shouldn't be an issue but idk.

Tho that would also require unmerging the bools and ids, but that doesn't matter, the openvr config struct is already such a mess that it doesn't really matter.

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

Successfully merging this pull request may close these issues.

3 participants