-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
#1734 actions based vr input #1735
#1734 actions based vr input #1735
Conversation
actionSetHandles.put(actionSet,actionSetHandle); | ||
} | ||
|
||
//Todo: this seems to imply that you could have multiple active action sets at once (Although I was not able to get that to work), allow multiple action sets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It annoyed me that I couldn't get multiple action sets working. But then I didn't really understand why you'd want more than one action set, or why you'd want to turn them off. So I haven't worried about it too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way multiple action sets could be used is, for example, if you want to have input per AppState. Or maybe one action set for interacting with the world and another for interacting with the user interface. There definitely are use cases, but I don't think it's a big deal if we don't support it, especially as the OpenVR documentation is a bit vague on how that is supposed to work.
…dern actions based input/vr/AnalogActionState.java All the other apis are vender specific or are alternative of versions OpenVR, so are deprecated - Deprecate everything but lwjgl openVr - Add support for restricting analog, digital and haptics to a particular hand - improve vr javadocs - Add haptics to the new openVR api - Add analogue inputs - Add action based digital controls into jme-vr
909f250
to
120f333
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrote down a couple of documentation nitpicks, but overall from a code perspective looks good to me. I don't have any jme powered VR projects on hand, nor the time to make one from scratch, so I couldn't test if it actually runs though.
* {@link #registerActionManifest} must have been called before using this method. | ||
* | ||
* @param actionName The name of the action. E.g. /actions/main/in/openInventory | ||
* @param restrictToInput the input to restrict the action to. E.g. /user/hand/right. Or null, which means "any input" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add the path for the left hand in here as well, just for the sake of completeness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done
/** | ||
* Check if the given button is down (more generally if the given input type is activated). | ||
* | ||
* Deprecated as should use an actions manifest approach. See {@link #registerActionManifest} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a notice about action based approach only working on OpenVR on all places we tell people to use that instead, so the one in a million people that might still be using some other backend don't get too confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. I've made that clear on the deprecation notices
* Note that registering an actions manifest will deactivate legacy inputs (i.e. methods such as {@link #isButtonDown} | ||
* will no longer work | ||
* | ||
* This option is only relevant to OpenVR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd link this somewhere in this javadoc comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo, that's very useful. I ended up backwards engineering how action manifests worked from the examples I found but thats much more useful, thanks. I've added that
actionSetHandles.put(actionSet,actionSetHandle); | ||
} | ||
|
||
//Todo: this seems to imply that you could have multiple active action sets at once (Although I was not able to get that to work), allow multiple action sets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way multiple action sets could be used is, for example, if you want to have input per AppState. Or maybe one action set for interacting with the world and another for interacting with the user interface. There definitely are use cases, but I don't think it's a big deal if we don't support it, especially as the OpenVR documentation is a bit vague on how that is supposed to work.
…n based API is only for OpenVr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks for the review, @grizeldi.
@richardTingle Have you tested the new code in an application? |
@stephengold yes, I did an install to maven local, created a project (with code much like in my intro to this PR) in intelliJ and ran on an oculus quest 2 (via virtual desktop so it pretended to be a PCVR system) |
Sounds good, then. Thanks for your contributions to the project, @richardTingle ! |
This leaves all the old stuff there (the oculus, osvr & other openvr) but focusses on the LWJGL openvr for the short term future (I guess OpenXR later on as the more long term future). I have ended up with a bit of a leaky abstraction where VRInputAPI wants to talk about openVr specific stuff like action manifests but I couldn't avoid that without requiring users to cast to LWJGLOpenVRInput which feels equally as bad (especially as LWJGLOpenVRInput is probably the only one that's going to be usable till OpenXR comes along)
This allows Jmonkey VR to use the actions manifest style VR (which is the non deprecated openVr api for mapping button presses to "stuff" in game).
An example action manifest might look like
With a default bindings looking like
Then it would be used like:
And within the update loop like
Assuming people are happy with this I intend to expand the wiki to document how to write action manifests etc