Controller integration #125

Conzar · 2017-08-09T22:38:33Z

This PR adds controller integration with the following features:

Button Mapping
Axis Mapping
Touchpad Touch/untouch mapping
Trigger Mapping
Battery Mapping

Joysticks are not implemented.

to get steamvr to recognize the controllers.

But, controller buttons are sorted. I don't think the axis is correct on the trackpad though. There is also a Button_A that I wasn't sure what to map to.

godbyk

I've made a first pass on this. It looks pretty good! I have a few questions and a couple thoughts on how we might be able to reduce the code a bit without overcomplicating things. Let me know what you think.

godbyk · 2017-08-09T22:41:47Z

.gitignore

@@ -30,3 +30,5 @@
 # Build directory
 build

+nbproject


These folders should only exist in the build tree and the build tree should be outside of the source tree or contained within a subdirectory (e.g., build) so as not to interfere with the source tree.

These are present in my repository. I use netbeans and netbeans automatically creates nbproject. I also create the build directory within the project directory. This is why I added them to .gitignore.

godbyk · 2017-08-09T22:42:13Z

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "openvr"]
 	path = openvr
-	url = https://github.com/ValveSoftware/openvr.git
+	url = https://github.com/ValveSoftware/openvr


The .git extension on the URL is what Github gives me. Did that not work for you?

I had some issues with submodule when I first created my fork. I had to delete it. I added it back with the git submodule add. I will change this back to .git though as I think its probably something on my end that wasn't correct.

godbyk · 2017-08-10T00:10:43Z

src/ServerDriver_OSVR.cpp


    for (auto& tracked_device : trackedDevices_) {
+        OSVR_LOG(debug) << "ServerDriver_OSVR - for loop - before TrackedDeviceAdded\n";


We can probably remove this log entry.

godbyk · 2017-08-10T00:10:48Z

src/ServerDriver_OSVR.cpp

        vr::VRServerDriverHost()->TrackedDeviceAdded(tracked_device->getId(), tracked_device->getDeviceClass(), tracked_device.get());
+        OSVR_LOG(debug) << "ServerDriver_OSVR - for loop - after TrackedDeviceAdded\n";


We can probably remove this log entry.

godbyk · 2017-08-10T00:15:47Z

src/ServerDriver_OSVR.cpp

@@ -86,9 +87,13 @@ vr::EVRInitError ServerDriver_OSVR::Init(vr::IVRDriverContext* driver_context)

    trackedDevices_.emplace_back(std::make_unique<OSVRTrackedHMD>(*(context_.get())));
    trackedDevices_.emplace_back(std::make_unique<OSVRTrackingReference>(*(context_.get())));
+    trackedDevices_.emplace_back(std::make_unique<OSVRTrackedController>(*(context_.get()), 0)); // left hand


Instead of passing in 0 and 1 for the controller index, could we pass in vr::TrackedControllerRole_LeftHand and vr::TrackedControllerRole_RightHand instead? (They eval to 1 and 2 instead, so other code would have to be tweaked.)

I haven't reviewed the usage of the controller index yet, so this may be a bad idea.

OSVRTrackedDevice(context, vr::TrackedDeviceClass_Controller, "OSVR controller"+std::to_string(controller_index)), controllerIndex_(controller_index)
That is the constructor that uses the index. For OSVRTrackedDevice, its passed a string so the name probably doesn't matter.

There is a conditional statement that uses the value of the controllerIndex_ but certainly, that would be easily to change to RightHand. I will make the change now and check if that works.

Done and pushed

godbyk · 2017-08-10T00:25:59Z

src/OSVRTrackedController.h

+#define INCLUDED_OSVRTrackedController_h_GUID_128E3B29_F5FC_4221_9B38_14E3F402E645
+
+
+#define NUM_BUTTONS 64


Use vr:: k_EButton_Max instead of NUM_BUTTONS?

hum, when I change it to k_EButton_Max & k_unControllerStateAxisCount, after exiting steamvr, Steam crashes!!! I suspect there might be something up in this code
for (int iter_axis = 0; iter_axis < NUM_AXIS; iter_axis++)
and/or
for (int iter_button = 0; iter_button < NUM_BUTTONS; iter_button++) {
I replaced NUM_AXIS and NUM_BUTTONS with the enum values.

godbyk · 2017-08-10T00:26:44Z

src/OSVRTrackedController.h

+    static void controllerJoystickXCallback(void* userdata, const OSVR_TimeValue* timestamp, const OSVR_AnalogReport* report);
+    static void controllerJoystickYCallback(void* userdata, const OSVR_TimeValue* timestamp, const OSVR_AnalogReport* report);
+    static void controllerXAxisCallback(void* userdata, const OSVR_TimeValue* timestamp, const OSVR_AnalogReport* report);
+    static void controllerYAxisCallback(void* userdata, const OSVR_TimeValue* timestamp, const OSVR_AnalogReport* report);


Are any of these callbacks similar enough that we could consolidate them? (I haven't read through their implementations yet.)

yep, I left controllerYAxisCallback & controllerXAxisCallback and removed the Joystick callbacks.

godbyk · 2017-08-10T00:44:53Z

src/OSVRTrackedController.cpp

+    //vr::VRProperties()->SetBoolProperty(propertyContainer_, vr::Prop_DeviceIsWireless_Bool, false);
+    vr::VRProperties()->SetInt32Property(propertyContainer_,  vr::Prop_DeviceClass_Int32,       static_cast<int32_t>(deviceClass_));
+    /*
+    vr::VRProperties()->SetInt32Property(propertyContainer_,  vr::Prop_Axis0Type_Int32,         static_cast<int32_t>(analogInterface_[0].axisType));


I think we need to set the axis types so games can tell which ones are the touchpad, which are the triggers, etc.

I agree and its done in the registerTrigger, registerTouchpad, etc functions. See

in registerTrackpad
analogInterface_[id].axisIndex = id; analogInterface_[id].axisType = vr::EVRControllerAxisType::k_eControllerAxis_TrackPad; analogInterface_[id].parentController = this; analogInterface_[id].analogInterfaceX.registerCallback(&OSVRTrackedController::controllerXAxisCallback, &analogInterface_[id]); propertyContainer_ = vr::VRProperties()->TrackedDeviceToPropertyContainer(this->objectId_); vr::VRProperties()->SetInt32Property(propertyContainer_, vr::Prop_Axis0Type_Int32, static_cast<int32_t>(analogInterface_[id].axisType));

godbyk · 2017-08-10T00:50:57Z

src/OSVRTrackedController.cpp

+ * @param path the complete path to the button.
+ */
+void OSVRTrackedController::registerButton(int id, std::string path, vr::EVRButtonId button_id){
+    buttonInterface_[id].buttonInterface = context_.getInterface(path);


Ensure id is within bounds of array so we don't try to access an element that doesn't exist (and segfault).

I have added checks and committed.

godbyk · 2017-08-10T00:52:39Z

src/OSVRTrackedController.cpp

+ * @param id the id of the button which is used for indexing an array.
+ * @param path the complete path to the button.
+ */
+void OSVRTrackedController::registerButtonTouch(int id, std::string path, vr::EVRButtonId button_id){


registerButton() and registerButtonTouch() look to be the same but for the callback. Could we pass the callback function in as a parameter and just have a single registerButton() method?

Probably, I had a go at it but I am not skilled enough in C++ to do this. Sorry.

Conzar · 2017-11-19T22:13:49Z

I just want to bump this PR to see if there is anything else that I need to do to get it accepted. Thank you.

godbyk and others added 18 commits July 4, 2016 23:52

Refactored to pull common code into OSVRTrackedDevice base class.

1ced330

Removed obsolete code.

a4fc526

Initial import of controller support.

5a4a62f

Merged controller with master in preperation for integration work.

6ed2bfa

Initial Commit - merged and updated so that it compiles.

61332c8

Ignoring cmake

1e110c1

Working on integration.

5dfd217

Working on developing.

814097f

Added print statement on deactivate.

0565ef5

Working with Kevin on fixing problems with constructor

f72ba13

to get steamvr to recognize the controllers.

Added back openvr submodule and cleaned up for latest version.

9b1e0b6

Still lots of comments and needs to be cleaned up.

124fd8e

But, controller buttons are sorted. I don't think the axis is correct on the trackpad though. There is also a Button_A that I wasn't sure what to map to.

Added touch press and unpress for trackpad!

16564da

Fixed trigger!

80fca11

Added battery interaction.

ad059b4

Fixed the battery status.

c1be95c

Cleaned up, readying for PR.

c78f592

Removed comments.

da14a85

godbyk requested changes Aug 10, 2017

View reviewed changes

Conzar added 7 commits August 11, 2017 16:29

changed openvr path back.

bd45e00

Using enum to differentiate between left and right controller.

0e1eb86

Removed unnecessary log entry.

4f46a40

Added back in commented out (due to version mismatch).

61c5bba

Removed unused function.

62fd7b7

String optimization

2cd70f4

Added error checking for array bounds.

3e3b4b4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controller integration #125

Controller integration #125

Conzar commented Aug 9, 2017

godbyk left a comment

godbyk Aug 9, 2017

Conzar Aug 10, 2017

godbyk Aug 9, 2017

Conzar Aug 11, 2017

godbyk Aug 10, 2017

Conzar Aug 11, 2017

godbyk Aug 10, 2017

Conzar Aug 11, 2017

godbyk Aug 10, 2017

Conzar Aug 11, 2017

Conzar Aug 11, 2017

godbyk Aug 10, 2017

Conzar Aug 11, 2017

godbyk Aug 10, 2017

Conzar Aug 11, 2017

godbyk Aug 10, 2017

Conzar Aug 11, 2017 •

edited

godbyk Aug 10, 2017

Conzar Aug 11, 2017

godbyk Aug 10, 2017

Conzar Aug 11, 2017

Conzar commented Nov 19, 2017


		for (auto& tracked_device : trackedDevices_) {
		OSVR_LOG(debug) << "ServerDriver_OSVR - for loop - before TrackedDeviceAdded\n";

		vr::VRServerDriverHost()->TrackedDeviceAdded(tracked_device->getId(), tracked_device->getDeviceClass(), tracked_device.get());
		OSVR_LOG(debug) << "ServerDriver_OSVR - for loop - after TrackedDeviceAdded\n";

		#define INCLUDED_OSVRTrackedController_h_GUID_128E3B29_F5FC_4221_9B38_14E3F402E645


		#define NUM_BUTTONS 64

Controller integration #125

Are you sure you want to change the base?

Controller integration #125

Conversation

Conzar commented Aug 9, 2017

godbyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conzar Aug 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conzar commented Nov 19, 2017

Conzar Aug 11, 2017 •

edited