lonetech / nolo-osvr

OSVR plugin for LYRobotix Nolo VR tracker system
Apache License 2.0
12 stars 8 forks source link

Trigger #14

Closed Conzar closed 6 years ago

Conzar commented 6 years ago

The following PR includes the following:

This PR will fully work with my fork. I am working on a PR for that fork so eventually it will get into mainline SteamVR-OSVR.

nanospork commented 6 years ago

@Conzar: I believe the "rest" value in the json for the triggers should be 0, not 1. Should fix that and update PR before @lonetech merges.

@lonetech Justification: The analog mapping for the trigger is in addition to the button mapping, not a replacement. While the hardware still does not appear to output an actual analog value, by including an analog value we allow developers the consistency of mapping controls to one, since I believe most competitors use analog triggers. This is especially helpful with the SteamVR-OSVR plugin, since SteamVR only accepts an analog trigger value.

Normalizing the axes between -1 and 1 also helps ensure consistency in the future and, in particular, is helpful for the SteamVR-OSVR plugin. It is possible that we could return to using the raw values in the future if OSVR clients can access the "min/max/rest' on their own, but it seems for touchpads, joysticks, and triggers, it actually makes sense to just standardize on all devices reporting a normalized value.

The NUM_BUTTONS and NUM_AXIS vars just make future development on the plugin easier.

lonetech commented 6 years ago

This contains a long list of improvements, which I am really happy to see! In particular I'm embarrassed to have left the off by one for all the buttons in there.

Just one comment before merging, though: the touchpad doesn't rest at zero. The value 255 is used to indicate no touch (unlike 0 which also happens when there's no report yet). This is distinct from joysticks which mechanically return to center. As such, it would seem appropriate for rest to be whatever 255 maps to.

nanospork commented 6 years ago

Hmmm. Okay, I understand that approach but it seems counterintuitive to developers, especially since other devices like the Oculus Touch controllers use a joystick that does recenter, and devs might be expecting that behavior. I would argue that requesting the devs access "controller/hand/trackpad/touch" explicitly to determine whether the trackpad is being touched makes more sense. It sounds like SteamVR does both?

We also already scaled the output to -1 to 1 so the special value of 255 doesn't really apply anymore :/

Conzar commented 6 years ago

@lonetech the analog:traits currently are not used by osvr so the rest:1 for the trigger doesn't effect. I think maybe we should just remove the traits as to not confuse anyone. Thoughts?

nanospork commented 6 years ago

What exactly are they there for, if OSVR doesn't use them?

Also I realize now that the XY are mapped to a semantic path under "trackpad" so my earlier point about developer confusion should be moot. However, if for example a dev creates a game that requires a joystick on the controller and somebody wants to create an alias for the trackpad to enable them to play that game, I think not recentering could be problematic. In other words, I don't think there is a downside to emulating recentering, which as I understand it means we would check for raw input == 255 and set output = 0 (since we've normalized -1 to 1).

nanospork commented 6 years ago

If the traits exist only for developer reference, that seems problematic. We don't want game devs to have to look at a specific device's traits to set up their controls. We've already started working around that by normalizing. If the resting position listed in traits simply tells devs where the resting position is and doesn't influence the behavior, then I think we should change the behavior to rest at 0.

lonetech commented 6 years ago

I think that's a much wider discussion for OSVR level. It's quite possible a touchpad shouldn't have a rest attribute at all, and that nothing reads it, but the answers (and planning discusson) belong in the wider community. If a consensus is found, we can remap 255 then.

nanospork commented 6 years ago

I'll see if I can find the appropriate channel to start that discussion then.

nanospork commented 6 years ago

@Lonetech: I just got done playing a game (Serious Sam TSE VR) that uses the trackpads for movement, and the fact that a non-centered value was reported caused my character to spin wildly until I went into options and enabled the setting that required the touchpad button to be pressed. Unfortunately keeping the button pressed while using the trackpad puts a lot of stress on the hardware (Vive controllers have been known to be damaged by this behavior) and Serious Sam is one of few games that even give the option.

I honestly can't think of a situation where reporting the edge value like this actually makes sense.

I mean, perhaps the SteamVR-OSVR driver should check for it and then just not report any touch, but that seems like a hardware-specific quirk we would be programming for, and I'm not even sure it would work. And in native OSVR, individual app developers would have to handle it, again if it even worked.

I understand the hardware may report 255 to indicate no touch, but it really doesn't seem to make sense to have the driver do the same. The driver should report the most "neutral" value, since the axis must always be assigned a value anyway.

lonetech commented 6 years ago

It's not an edge value; it's a sentinel value, indicating no touch. The absence of the touch bit also indicates no touch. Are you seeing touch set when the value is 255? We can stop setting the value for the analog axis, as setting axis state itself is a separate function in OSVR, but the issue is that the axis value is invalid at that point in time.

Have I missed somewhere in the OSVR specs that a touchpad is defined to not update its axis when untouched?

https://github.com/ValveSoftware/openvr/wiki/IVRSystem::GetControllerState indicates there's a mask ulButtonTouched, a type indicator k_eControllerAxis_TrackPad and of course controller axis. It doesn't indicate any particular value the axis should have when there is no touch.

We're a bit off topic here, and this doesn't change the fact that setting a "rest" value of 0 for the touchpad made no sense either. 255 doesn't mean the edge but effectively NaN in the protocol.