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.

Sign up for GitHub

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

Jump to bottom

Proposal for the OPPO vendor extension #146

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Proposal for the OPPO vendor extension #146

merged 2 commits into from
Jun 9, 2023

Conversation

hjiang36
Copy link
Contributor

Recreate pull request for OPPO vendor extension proposal to rebase commits to latest releases / staging.
The original pull request was in #144

@rpavlik @Wallbraker It seems like there is a new release last week and I rebase the unmerged part of work from pr144 to here. Let me know if there is something I could do to move this forward.

Thanks a lot.

@rpavlik-bot rpavlik-bot added the synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab label Mar 29, 2023
@rpavlik-bot
Copy link
Collaborator

An issue (number 1963) has been filed to correspond to this pull request in the internal Khronos GitLab (Khronos members only: KHR:openxr/openxr#1963 ), to facilitate working group processes.

This GitHub pull request will continue to be the main site of discussion.

@rpavlik
Copy link
Contributor

rpavlik commented Mar 30, 2023 •
edited
Loading

There are people on the working group who are concerned with your suggested naming (of the component with just controller), that you may regret calling it that (without decoration or a trademark) when you come to make your second controller design. We do not have formal rules against it at this time but probably will soon.

We are also making sure that interaction profiles added in vendor extensions (the actual profile path itself) is also decorated with your vendor ID, so that we can remove it to perform any renames needed when/if it is promoted to KHR or core.

Also, in the 1.0.27 release we introduced describing interaction profiles additionally in the XML. The spec text is not (yet) generated from that part of the XML, but please add it since tooling is being worked on that uses it. (There is some checking of those entries in ./checkSchematron.sh)

(It is OK to force-push over your old branch, you do not need to open a new MR if you rebase or amend commits)

@hjiang36
Copy link
Contributor Author

@rpavlik Thanks for the comments. I updated the pull request with decorated controller name and xml entries. Please check and let me know if there is anything else needed. Thanks

@hjiang36
Copy link
Contributor Author

@rpavlik Thanks a lot for the detailed review and suggestions. I accepted all suggestions and added some description about the heartrate sensor. Please check and let me know if there is anything to be updated. Thanks a lot

@hjiang36
Copy link
Contributor Author

hjiang36 commented Apr 7, 2023

@rpavlik I justed modified then entries per your suggestion. If you could help review again and let me know if there is anything else to be updated, it would be greatly appreciated. Thanks

@hjiang36
Copy link
Contributor Author

@rpavlik I'm wondering if there is any update on this PR. Please let us know if there is anything we should do to move this forward. thanks a lot.

@hjiang36 hjiang36 requested a review from rpavlik May 10, 2023 21:45
@hjiang36 @rpavlik
Proposal for the OPPO vendor extension
d7d028c 
* Decorated name for controller and adding xml entry
* Add description about heartbeat on controller
* Add /input/thumbstick path
* Remove battery path
* Add reference to interaction profile in extension
* Apply PrettyRegistryXML
* Accept suggestions

Co-authored-by: Ryan A. Pavlik <ryan.pavlik@collabora.com>
rpavlik
rpavlik requested changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@rpavlik rpavlik left a comment

Choose a reason for hiding this comment

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

Looks like I missed one correction. I squashed your existing commits into a single one, and added an XML formatting cleanup. Once the license header is added, I can merge this.

specification/sources/chapters/extensions/oppo/oppo_controller_interaction.adoc Outdated Show resolved Hide resolved
@hjiang36 @rpavlik
Update specification/sources/chapters/extensions/oppo/oppo_controller…
472e200 
…_interaction.adoc

Co-authored-by: Ryan A. Pavlik <ryan.pavlik@gmail.com>
@rpavlik rpavlik self-requested a review June 9, 2023 16:10
rpavlik
rpavlik approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@rpavlik rpavlik left a comment

Choose a reason for hiding this comment

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

Thank you!

@rpavlik rpavlik merged commit 2cdb57d into KhronosGroup:staging Jun 9, 2023
@rpavlik
Copy link
Contributor

rpavlik commented Jul 11, 2023

Released and published in 1.0.28!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fantastxu fantastxu fantastxu left review comments

@utzcoz utzcoz utzcoz left review comments

@rpavlik rpavlik rpavlik approved these changes

Assignees
No one assigned
Labels
synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hjiang36 @rpavlik-bot @rpavlik @fantastxu @utzcoz