-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
SNAP App: support for Thumbstick of the right controller to capture picture and gif in HMD only. #1090
Closed
Closed
SNAP App: support for Thumbstick of the right controller to capture picture and gif in HMD only. #1090
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
There should be some sort of fallback for an unrecognized controller. At least log something, so that there's some clue as to what is wrong, but preferably an user-visible error message.
Also, the XBox controller has been used for VR, so it's also a possibility.
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.
3 things about it...
1- Fact: I simply bring the code from other camera app supporting the right thumb stick. This means that the same issue exists in those app too.
2- Xbox Controller: I don't have the hardware so I wouldn't be able to test it. I have honestly not consider support larger than that for that simple reason. If you have the "controllerType" string and which Controller.Standard.### it uses, then yes, I can add it.
3- Unrecognized controller: Currently, if a controller is not recognized, it simply do nothing. (No error, just not supported). The existence of this feature is not even written anywhere. This is not on the UI (a bit intentional). So people are currently find out accidentally or by learning from someone else. If a user has an unsupported controller, it won't tell him that the feature that he has never heard about before, is not working for him. He will be then as happy as all of us before this new implementation. In my opinion, It would be a bit mistake to advise directly the user that his controller is not taking change of this feature. It just cause the deception even before the need appears... It will just look clunkier as system for that user after that.
Instead, I would add a notice in the UI, but only if the controller is supporting it.
I didn't went there cause the cost in time was a bit too much for actually just a "shortcut".
Which means: changes in the Ui and what it needs between the app and the UI to hide/show the notice.
I can do it. Let me know what you think about this approach.
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 think that adding a notice in the UI for supported controllers would be a really good idea.
Could you also add a TODO comment in the code telling to update this once OpenXR is merged?