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

Vr tracker support #213

Merged
merged 14 commits into from Jan 25, 2019
Merged

Vr tracker support #213

merged 14 commits into from Jan 25, 2019

Conversation

@joreg
Copy link
Contributor

joreg commented Sep 24, 2018

  • added a list of TrackedItems to Xenko.VirtualReality.VRDevice
  • for now only supported by OpenVR, the other APIs return new TrackedItem[0]
  • it returns all devices, including HMD, Controllers, GenericTrackers, LightHouses,..
  • added DeviceClass enum to file DeviceEnums.cs (previously named DeviceState.cs). the enum is taken from OpenVR
  • a TrackedItem also returns its SerialNumber so we can distinguish several devices of the same class
  • a TrackedItem also returns its BatteryPercentage
  • SetTrackingSpace on VRDevice now allows to choose between Seated, Standing and Raw
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Sep 24, 2018

CLA assistant check
All committers have signed the CLA.

@joreg joreg mentioned this pull request Sep 24, 2018
Copy link
Collaborator

Kryptos-FR left a comment

A few remarks on a pure technical/language side. I'll let @xen2 do the proper review.

sources/engine/Xenko.VirtualReality/TrackedDevice.cs Outdated Show resolved Hide resolved
// Distributed under the MIT license. See the LICENSE.md file in the project root for more information.
namespace Xenko.VirtualReality
{
public enum DeviceState

This comment has been minimized.

Copy link
@Kryptos-FR

Kryptos-FR Sep 24, 2018

Collaborator

Why moving the enum into a different file?

This comment has been minimized.

Copy link
@joreg

joreg Oct 1, 2018

Author Contributor

before, it was one enum in a file with the same name as the enum (DeviceState). since i added a second enum concerning the Device i thought, instead of adding a second file, i'd put them both in the same file and renaming the file to a more generic name, ie. DeviceEnums.cs.

please let me know if you prefer one file per enum and i'll change it accordingly.

This comment has been minimized.

Copy link
@xen2

xen2 Jan 19, 2019

Member

Don't mind so much with so small enums, but so far most of the code is one file per enum, so might be better to keep consistency.

@@ -43,6 +43,8 @@ public class DummyDevice : VRDevice

public override TouchController RightHand => null;

public override TrackedDevice[] TrackedDevices => null;

This comment has been minimized.

Copy link
@Kryptos-FR

Kryptos-FR Sep 24, 2018

Collaborator

I personally prefer collection properties to return empty rather than null.

This comment has been minimized.

Copy link
@joreg

joreg Oct 1, 2018

Author Contributor

done. now returns new ..[0]

@@ -150,6 +150,27 @@ public void Update()
}
}

public class TrackedDevice

This comment has been minimized.

Copy link
@Kryptos-FR

Kryptos-FR Sep 24, 2018

Collaborator

Not a fan of having classes with the same name in different namespaces. Can cause confusion and makes it harder to find/navigate the code.

This comment has been minimized.

Copy link
@joreg

joreg Oct 1, 2018

Author Contributor

renamed the Xenko.VirtualReality one to TrackedItem so the one in OpenVR can keep the name of its original api.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Oct 7, 2018

I assume this is ready for review? I will take a look and merge it soon.

…uctor since it doesn't change at runtime

(cherry picked from commit b6a8f4912637b7f89b0017da07df3ae6efc53a57)
@joreg

This comment has been minimized.

Copy link
Contributor Author

joreg commented Oct 7, 2018

@tebjan i put the SerialNumber and DeviceClass getters in Update instead of the constructor because i wasn't sure if they couldn't actually change at runtime: see, the TrackedDevice is only an index in to OpenVRs list of devices. to my understandings controllers and trackers can be added/removed (ie. paired/unpaired) at runtime which i'd assume can change their order in the list. therefore i thought putting those in update is the saver option.

if we are certain it can not happen, then of course it should go to the constructor. but then not only SerialNumber but also DeviceClass.

@xen2 otherwise yes, please.

@tebjan

This comment has been minimized.

Copy link
Contributor

tebjan commented Oct 7, 2018

@joreg ah you are right, if the index of the same physical device changes during runtime, it would create confusion. we could test that with the vive trackers and the controllers.

@joreg

This comment has been minimized.

Copy link
Contributor Author

joreg commented Oct 10, 2018

@xen2 i just checked, and indeed, devices can be added at runtime, so i moved the serialnumber check back to update. interestingly though it seems they cannot be removed at runtime. once a slot is occupied it is occupied for runtime, even if the device is turned off.

also i allowed myself to expose another feature we need: VRDevice.SetTrackingSpace()

@tebjan

This comment has been minimized.

Copy link
Contributor

tebjan commented Oct 10, 2018

added a slight improvement by re-using the same string builder in every frame.

serialNumberStringBuilder.Clear();
Valve.VR.OpenVR.System.GetStringTrackedDeviceProperty((uint)TrackerIndex, ETrackedDeviceProperty.Prop_SerialNumber_String, serialNumberStringBuilder, StringBuilderSize, ref error);
if (error == ETrackedPropertyError.TrackedProp_Success)
SerialNumber = serialNumberStringBuilder.ToString();

This comment has been minimized.

Copy link
@xen2

xen2 Oct 11, 2018

Member

Does that mean it will allocate every frame? (something we are trying to avoid)

This comment has been minimized.

Copy link
@joreg

joreg Oct 11, 2018

Author Contributor

what do you mean by "allocate every frame"? what those lines do, is accessing the serialnumber property of the trackeddevice with the index TrackerIndex. i don't see anything getting allocated there.

This comment has been minimized.

Copy link
@Kryptos-FR

Kryptos-FR Oct 11, 2018

Collaborator

StringBuilder.ToString() will allocate a new string at every update.

This comment has been minimized.

Copy link
@joreg

joreg Oct 11, 2018

Author Contributor

ok, any idea to improve that?

This comment has been minimized.

Copy link
@tebjan

tebjan Oct 25, 2018

Contributor

maybe we could check somehow whether the device slot has changed? that and create is the only moment when the serial number needs reading...

This comment has been minimized.

Copy link
@tebjan

tebjan Oct 25, 2018

Contributor

another idea would be to place the code into the getter of the property or a method of the TrackedDevice, then it would only be read and allocated if someone asks for it...

{
var error = ETrackedPropertyError.TrackedProp_Success;
var result = new System.Text.StringBuilder((int)64);
Valve.VR.OpenVR.System.GetStringTrackedDeviceProperty((uint)TrackerIndex, ETrackedDeviceProperty.Prop_SerialNumber_String, result, 64, ref error);

This comment has been minimized.

Copy link
@tebjan

tebjan Oct 25, 2018

Contributor

this is expensive and only needs to be called once. you can check whether the SerialNumber is null and only then update it.

serialNumberStringBuilder.Clear();
Valve.VR.OpenVR.System.GetStringTrackedDeviceProperty((uint)TrackerIndex, ETrackedDeviceProperty.Prop_SerialNumber_String, serialNumberStringBuilder, StringBuilderSize, ref error);
if (error == ETrackedPropertyError.TrackedProp_Success)
SerialNumber = serialNumberStringBuilder.ToString();

This comment has been minimized.

Copy link
@tebjan

tebjan Oct 25, 2018

Contributor

maybe we could check somehow whether the device slot has changed? that and create is the only moment when the serial number needs reading...

serialNumberStringBuilder.Clear();
Valve.VR.OpenVR.System.GetStringTrackedDeviceProperty((uint)TrackerIndex, ETrackedDeviceProperty.Prop_SerialNumber_String, serialNumberStringBuilder, StringBuilderSize, ref error);
if (error == ETrackedPropertyError.TrackedProp_Success)
SerialNumber = serialNumberStringBuilder.ToString();

This comment has been minimized.

Copy link
@tebjan

tebjan Oct 25, 2018

Contributor

another idea would be to place the code into the getter of the property or a method of the TrackedDevice, then it would only be read and allocated if someone asks for it...

@joreg

This comment has been minimized.

Copy link
Contributor Author

joreg commented Oct 25, 2018

ouright, will move SerialNumber, DeviceClass and Batteries into getters.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Oct 26, 2018

Seems like there is some line ending issues in most edited files (I see the warning when opening https://github.com/vvvv/xenko/edit/VRTrackerSupport/sources/engine/Xenko.VirtualReality/OpenVR/OpenVR.cs)

your CRLF setting should be set to "auto" (core.autocrlf=true)

@joreg

This comment has been minimized.

Copy link
Contributor Author

joreg commented Oct 26, 2018

indeed, i've just moved to a new PC where this wasn't set correctly. i checked all files this PR modifies and only found an inconsistency in this one mentioned file though..

@xen2 xen2 force-pushed the xenko3d:master branch from 88dcb74 to 1535057 Nov 19, 2018
@joreg

This comment has been minimized.

Copy link
Contributor Author

joreg commented Nov 29, 2018

hei @xen2 do you see any further issues with this?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 18, 2019

@joreg sorry for the delay...
Let's merge this for 3.1.

@xen2 xen2 self-assigned this Jan 18, 2019
Controller, //The device is a controller
GenericTracker, //The device is a tracker
TrackingReference, //The device is a camera, Lighthouse base station, or other device that supplies tracking ground truth.
DisplayRedirect //Accessories that aren't necessarily tracked themselves, but may redirect video output from other tracked devices

This comment has been minimized.

Copy link
@xen2

xen2 Jan 19, 2019

Member

Can you put those comments following the /// <summary>My summary here.</summary> xml convention?
Since you bothered adding them, better make them visible to API users as well 😄

…l files and added xml comments
@xen2 xen2 merged commit 7e41209 into xenko3d:master Jan 25, 2019
3 checks passed
3 checks passed
Build Launcher (Package) - merge TeamCity build finished
Details
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 25, 2019

LGTM, merging
Thanks!

xen2 added a commit that referenced this pull request Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.