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

[Input] Add haptic support to OpenVR and Oculus runtimes #2169

Merged
merged 21 commits into from
Mar 5, 2024

Conversation

ComputerSmoke
Copy link
Contributor

PR Details

Added a Vibrate method to TouchController which takes a duration in milliseconds and vibrates the controller for that duration. This is implemented for OpenVR and oculus runtimes.

Description

TouchController:
Vibrate(int durationMs) takes a duration in ms, and vibrates the controller for that duration. It returns a Task which completes when the controller vibration ends.

OpenVR implementation:

Every 45 ms, Valve.VR.OpenVR.System.TriggerHapticPulse is called to vibrate the controller until durationMs has passed.

Oculus implementation:

VibrateLeft and VibrateRight are added to OculusOVR.cpp to set the vibration frequency of left and right controllers. OculusOVR exposes a SetVibration function which takes these parameters and a TouchControllerHand, and calls the corresponding VibrateLeft/Right. OculusTouchController's Vibrate method sets the vibration frequency to 1 every 2 seconds until durationMs passes, then disables vibration by setting frequency to 0 if there is no other Vibrate method currently running (as counted by ConcurrentVibratingCallCount).

Related Issue

#2030

Motivation and Context

Haptic feedback is essential to VR games, so Stride should provide a convenient way to make the controllers vibrate.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • [ x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [ x ] My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • [ x ] I have built and run the editor to try this change out.

@ComputerSmoke
Copy link
Contributor Author

@dotnet-policy-service agree

Copy link
Collaborator

@Eideren Eideren left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for the PR !

Comment on lines 413 to 432
public override async Task Vibrate(int durationMs)
{
void SetOvrVibration(bool enable)
{
float frequency = enable ? 1 : 0;
OculusOvr.SetVibration(OvrSession, hand, frequency, 1);
}
ConcurrentVibratingCallCount++;
while (durationMs > 2000)
{
SetOvrVibration(true);
durationMs -= 2000;
await Task.Delay(2000);
}
SetOvrVibration(true);
await Task.Delay(durationMs);
ConcurrentVibratingCallCount--;
if (ConcurrentVibratingCallCount == 0)
SetOvrVibration(false);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not thread safe as far as I can tell; if the user is calling Vibrate from the main thread while a concurrent one is finishing on a task thread this will fail since ConcurrentVibratingCallCount does not have any concurrent access mechanism in place.

Does the vibration automatically turn off after 2000 ms, is that why you have to periodically reset it ?

Is there a reason why frequency and amplitude are fixed values ? It sounds like those could be interesting for the caller to manipulate. For example slowly fading in and out the amplitude for an earthquake-like effect. Although those kinds of usage kind of goes against how the api is designed right now, maybe it would be best to change this into a simple void Vibrate(int frequency, int amplitude) and then have another non-virtual method that generates a task that wraps around this method, something like:

public async Task Vibrate(int durationMs, int frequency = 1, float amplitude = 1)
{
    Vibrate(frequency, amplitude);
    await Task.Delay(durationMs);
    StopVibration();
}
public abstract Task Vibrate(int frequency, float amplitude);
public abstract Task StopVibration();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing!

You're right about the ConcurrentVibratingCallCount increment/deincrement calls not being thread safe. Would it make more sense to acquire a lock, or replace it with a queue?

According to the docs on the ovr_SetControllerVibration called in OculusOVR.cpp, the vibration lasts for at most 2.5 seconds. 2000 milliseconds is a bit under this limit, and could be increased a bit.

According to the docs for the oculus runtime, frequency only accepts the values 0.0, 0.5, and 1.0. It clamps everything else to those values. 0.0 is no vibration, so we'd really just be adding a parameter that's either 0.5 or 1.0. There isn't a direct parallel in the openVR runtime. I was thinking we might be able to emulate it in the openVR runtime using shorter haptic pulses if we decide that the 0.5 frequency option is actually useful, but every call to TriggerHapticPulse there requires a 5ms cooldown. I didn't include the option in the PR because 50% or 100% (and no in-between) frequency option didn't seem useful, but I guess there's no downside to exposing it for oculus in case it is somehow useful to someone.

As for amplitude, the reason I didn't expose it is again because openVR doesn't seem to have a parallel, at least with the Valve.VR.OpenVR.System.TriggerHapticPulse(index, axis, duration) function. There could be another way to do it, but I wasn't able to find any.

Does it make sense to have a vibrate with amplitude option for oculus runtime but not openVR? Apps developed specifically for the oculus store might use that feature, but games going to steam probably want to support both types of controllers. Writing this out, I'm thinking it does make sense to expose these options for the oculus runtime since there's not really a downside.

@ComputerSmoke
Copy link
Contributor Author

I have added the Vibrate() and StopVibration() methods @Eideren suggested. Still no frequency / amplitude parameters because they are not universal to all TouchController types, and since the OculusTouchController class is internal it doesn't make sense to have a vibrate method that takes frequency/amplitude just for the OculusTouchController type.

The cost of having Vibrate() and StopVibration() methods is that the openVR vibration has to call haptic pulse every 5 ms (rather than 60 ms) to make the vibration stop close to when StopVibration is called, without knowing the vibration time when Vibrate is initially called.

These changes also should make things thread safe since the concurrent vibration counter is incremented/deincremented with Interlocked, but check my work there because I'm a noob.

@Eideren
Copy link
Collaborator

Eideren commented Mar 4, 2024

I do think your reasoning makes sense with how limited haptics are currently, but I find it likely that they wouldn't stay that way in the future.
I think supporting those parameters and ignoring them when they are not supported works pretty well in this case, haptics and its features are not essential to operate a video game in the vast majority of cases, and since developers likely don't have the hardware to test every devices, it silently failing is far more preferable to it throwing.
So instead of throwing for Mixed Reality controllers and OpenXR, we could no-op, add a summary over the abstract method that in some api those don't work/work as well, and have some other way to notify the developers that those features are not supported, like a property returning

enum ControllerHaptics
{
	None,
	Limited,
	LimitedFrequency,
	LimitedAmplitude,
	Full,
}

For OpenVR it could enable vibrations when both Frequency and Amplitude is greater than zero and for Oculus you would send the two parameters and let Oculus handle the rest, that way responsibility is on their side if they ever decide to change how their api or hardware work.

Interlocked.Decrement(ref _vibrationCounter);
if (_vibrationCounter <= 0)

This is far better but still doesn't guarantee thread safety, if a thread increments _vibrationCounter right after the decrement (granted, very unlikely but still) the thread would stop any vibration.
Thankfully this class is unlikely to have very high contention, so you can slap a lock around most of the logic and be done with it.
Something like:

private object vibrationLock = new object();
private int vibrationCount;

public async void VibrateFor(int milliseconds)
{
	lock (vibrationLock)
	{
		controllerAPI.Vibrate();
		vibrationCount++;
	}
	await yada yada milliseconds;
	lock (vibrationLock)
	{
		if (--vibrationCount == 0)
			controllerAPI.StopVibrate();
	}
}

The cost of having Vibrate() and StopVibration() methods is that the openVR vibration has to call haptic pulse every 5 ms (rather than 60 ms) to make the vibration stop close to when StopVibration is called, without knowing the vibration time when Vibrate is initially called.

Then best to avoid what I proposed, roll those changes back and let's work with what you setup initially, sorry about that.

@ComputerSmoke
Copy link
Contributor Author

Reverted to the fixed duration vibration, and added haptics with frequency/amplitude options.

When testing I noticed that the oculus runtime doesn't actually support frequency. The docs claim "frequency Vibration frequency. Supported values are: 0.0 (disabled), 0.5 and 1.0. Non valid values will be clamped.", but what actually happens is that the frequency parameter is ignored, even if it is 0. I therefore marked oculus touch controller as limited frequency, and made openVR ignore both frequency and amplitude parameters (still vibrating when 0 is passed) so that "limited support" consistently means the parameter is ignored, even when 0.

@Eideren
Copy link
Collaborator

Eideren commented Mar 5, 2024

Looks good, thanks a bunch @ComputerSmoke !

@Eideren Eideren merged commit 8689798 into stride3d:master Mar 5, 2024
2 checks passed
@Eideren Eideren changed the title Add haptic support to OpenVR and Oculus runtimes [Input] Add haptic support to OpenVR and Oculus runtimes Mar 5, 2024
@Eideren Eideren added enhancement New feature or request and removed area-Input labels Mar 5, 2024
Eideren added a commit to Eideren/xenko that referenced this pull request Apr 9, 2024
* Remove docs, has been moved to Stride docs repo

* [OpenXR] Fixes build on graphics APIs other than D3D11

This build regression was introduced by previous commit d72aef5

* Update samples to Stride 4.2 (stride3d#2132)

* [Samples] Update to 4.2

* [Samples] Remove Newtonsoft.Json dependency in CSharpIntermediate

* [Tests] Fixes random test failures (stride3d#2133)

* [Tests] Fixes random test failures

Some of the tests in Stride.GameStudio.Tests must not run in parallel as they access a shared class which is not thread safe.

* Sets wait time in TestFileVersionManager to 500ms (from 200)

Should make it less likely that unit tests on teamcity fail.

* Repair projectwatcher (stride3d#2106)

* use positive 77

* repair projectwatcher

* remove unused line

* undo -77 change

* undo -77

* order usings

* fix formatting

* remove unused solution

* remove unused async

* use previous cancelation method

* remove extra task

* add check to not throw assembly changes away

* rework distribution of assemblychanges

* remove unused using

* add broadcast back in

* remove assembly broadcast

* add cancelation

* replace cancelation location

* improve if nesting

* improve naming, fix reload on new references

* fix loading chain of assets

* refactor

* [Editor] Refactor initialization of CodeViewModel

---------

Co-authored-by: IXLLEGACYIXL <[email protected]>
Co-authored-by: Nicolas Musset <[email protected]>

* [OpenVR] Adds a minimal API to request and control Passthrough (supported by OpenXR)

https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XR_FB_passthrough

* [OpenXR] Makes device construction internal and improves exception messages of new StartPassthrough method

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

* [Assets] Prevent crash of the assets compiler when an assembly cannot be fully loaded. (stride3d#2144)

Fixes stride3d#2140

* [Build] Remove stylecop (stride3d#2105)

Co-authored-by: IXLLEGACYIXL <[email protected]>

* [Presentation] Rework AssetViewModelAttribute

* [Editor] Cleanup

* [Assets] Mark Editor as obsolete on AssetViewModel

* [Editor] Make Asset property public on all editors

* [Editor] Make IAssetEditorsManager a service

* [Editor] Keep track of asset/editor association in AssetEditorsManager

* [Editor] Add a method to find an opened editor

* [Editor] Change binding logic in editor views

The default DataContext is now the editor instead of the asset

* [Editor] Update sprite editor bindings

* [Editor] Update UI editor bindings

* [Editor] Update entity hierarchy editor bindings

* [Editor] Update script editor bindings

* [Editor] Update graphics compositor editor bindings

* [Editor] Removed pointless constraint on column width

Which is also spamming the console with binding errors.

* [Editor] Remove editor-related properties from AssetViewModel

* [Editor] Rework AssetEditorViewModelAttribute

- add AssetEditorViewAttribute
- rework view creation and initialization

* [Editor] Make AssetEditorViewModel.Asset property virtual

C# has covariant return types since version 9

* [Editor] Rework plugins initialization

* [Editor] Rework AssetPreviewViewModelAttribute & AssetPreviewAttribute

- add AssetPreviewViewAttribute to break the dependency between a preview and its UI-dependent view

* [Editor] Fix initialization of SpriteSheetEditorViewModel returning false

* [Editor] Make Stride.Core.Presentation.Quantum cross-platform

* [Editor] Make Stride.Core.Assets.Quantum cross-platform

* [Editor] Do not load StrideDefaultAssetsPlugin from module.

It currently hardcodes loading the templates from a package which causes some tests to fail.

Partially reverts c98c72e

* feat: Release.yml added for PR categorisation (stride3d#2137)

* feat: Release.yml added for PR categorisation

* Categories updated

* [Editor] Fix scene editor loading screen

* [VR] feat: Add haptic support to OpenVR and Oculus runtimes (stride3d#2169)

Co-authored-by: Eideren <[email protected]>

* [Audio] Audio emitter multiple references to same asset bugfix (stride3d#2176)

Co-authored-by: Eideren <[email protected]>

* Use correct destination path in asset import overwrite dialog

* Use GetFullPath to correct directory seperator for display

* fix: File GraphicsResourceMap.cs without references removed (stride3d#2181)

* feat: Update samples/template to top-level statements (stride3d#2187)

* Update README.md (prerequisites)

* Update README.md (VSIX prerequisites)

* [VSPackage] Revert a few package upgrades so that VSIX builds properly

* [Presentation] Fix issue with binding quantum nodes when associated name is not found (stride3d#2195)

Note: the solution is rather hackish at the moment. To be revisited once we have an Avalonia version.
Should we then introduce a service for setting/retrieving the Unset value

* fix: [Asset] Unified 3D asset importer (on behalf of Noa7/Noah7071) (stride3d#2163)

Co-authored-by: noa7 <[email protected]>
Co-authored-by: noa7707 <[email protected]>
Co-authored-by: Noah7071 <[email protected]>

---------

Co-authored-by: JornosDesktop <[email protected]>
Co-authored-by: Elias Holzer <[email protected]>
Co-authored-by: Nicolas Musset <[email protected]>
Co-authored-by: IXLLEGACYIXL <[email protected]>
Co-authored-by: IXLLEGACYIXL <[email protected]>
Co-authored-by: Nicolas Musset <[email protected]>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Jorn Theunissen <[email protected]>
Co-authored-by: Marian Dziubiak <[email protected]>
Co-authored-by: Vaclav Elias <[email protected]>
Co-authored-by: Addison Schmidt <[email protected]>
Co-authored-by: Eideren <[email protected]>
Co-authored-by: Tim Conner <[email protected]>
Co-authored-by: Jakub Ławreszuk <[email protected]>
Co-authored-by: xen2 <[email protected]>
Co-authored-by: noa7 <[email protected]>
Co-authored-by: noa7707 <[email protected]>
Co-authored-by: Noah7071 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants