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

Splits profiles-registry package into separate registry and assets packages #78

Merged
merged 4 commits into from Sep 14, 2019

Conversation

NellWaliczek
Copy link
Member

(Apologies for dropping another massive PR. This should be the last of the significant structural changes. For clarity, I'm expecting we'll still need to spend a bunch of time digging into the schema and content of the registry package, so I'm likely to request that feedback be filed as separate issues in order to land the structural changes)

The new registry package describes the data that must be exposed
consistently by all User Agents. For a specific piece of hardware, such
as an Oculus Touch controller, all User Agents must report the same
ordering of strings in XRInputSource.profiles. All User Agents must also
consistently map physical hardware components, such as thumbsticks, into
identical indexes within the Gamepad.buttons and Gamepad.axes arrays.
The schemas and JSON profile descriptions can be extended in the future
to unify additional XRInputSource behaviors and values.

The new assets package contains 3D asset files and JSON files with the
information necessary to animate the 3D assets in response to backing
hardware's state changes. The JSON files conform to a schema which
enumerates the relevant node names within the asset and describes visual
responses that should be applied to the 3D asset as Gamepad.buttons and
Gamepad.axes values change. The build step of the assets package merges
its profile JSON files with the matching profile JSON files in the
registry. These merged JSON files and assets can then be used directly
in the javascript library published by the motion-controllers package,
or by developers using their own asset loading and animation library.

The motion-controllers library remains 3D engine agnostic, but has been
signifcantly simplified to directly ingest the merged profile JSON files
published by the assets repository. Small changes were also made to the
viewer packages to adjust accordingly.

…ckages

The new registry package describes the data that must be exposed
consistently by all User Agents. For a specific piece of hardware, such
as an Oculus Touch controller, all User Agents must report the same
ordering of strings in XRInputSource.profiles. All User Agents must also
consistently map physical hardware components, such as thumbsticks, into
identical indexes within the Gamepad.buttons and Gamepad.axes arrays.
The schemas and JSON profile descriptions can be extended in the future
to unify additional XRInputSource behaviors and values.

The new assets package contains 3D asset files and JSON files with the
information necessary to animate the 3D assets in response to backing
hardware's state changes. The JSON files conform to a schema which
enumerates the relevant node names within the asset and describes visual
responses that should be applied to the 3D asset as Gamepad.buttons and
Gamepad.axes values change. The build step of the assets package merges
its profile JSON files with the matching profile JSON files in the
registry. These merged JSON files and assets can then be used directly
in the javascript library published by the motion-controllers package,
or by developers using their own asset loading and animation library.

The motion-controllers library remains 3D engine agnostic, but has been
signifcantly simplified to directly ingest the merged profile JSON files
published by the assets repository. Small changes were also made to the
viewer packages to adjust accordingly.
Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

Obviously not a detailed line-by-line review, but looks good enough to me to land as a foundation for the (smaller) PRs that will follow. Left a few comments that I'd appreciate seeing addressed prior to merging, and I'm interested in the outcome of Alex and Jacob's discussions.

OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF
OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
TODO Figure out what licence should be here
Copy link
Member

Choose a reason for hiding this comment

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

Good question! Gut reaction would be to say it should follow the spec work, which uses the W3C Software and Document License but given my experience with open source libs in the past that may drive people to not use it because it's not one of the well known licenses that their lawyers have given blanket approval for. Wonder if there's policy or precedent within the W3C for that?

(Side note: Because they grew out of personal code the WebXR samples are using an MIT license already, so if for whatever reason that's not cool with the W3C we should figure it out fast.)

@@ -1,21 +1 @@
MIT License

Copyright (c) 2019 Amazon
Copy link
Member

Choose a reason for hiding this comment

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

Whatever the answer to "Which license is it anyways" comes back as, the answer to "Who is it attributed to" is "The Immersive Web Working Group". 😉

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2019 Amazon
Copy link
Member

Choose a reason for hiding this comment

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

See above. Are the Lawyers likely to give you trouble if this is attributed to the Immersive Web Working Group?

packages/assets/profiles/samsung/samsung-gearvr.json Outdated Show resolved Hide resolved
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2019 Amazon
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

packages/viewer/README.md Outdated Show resolved Hide resolved
packages/registry/profiles/samsung/samsung-gearvr.json Outdated Show resolved Hide resolved
@NellWaliczek
Copy link
Member Author

I'm still digging on the license issue and hope to have an answer shortly. Meanwhile, I've addressed the remainder of the comments. I also realized that I'd totally broken the local-files version of the viewer, so I've fixed it.

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

Reaffirming my LGTM with the latest changes.

@NellWaliczek NellWaliczek merged commit e25988e into master Sep 14, 2019
@NellWaliczek NellWaliczek deleted the registry-split branch September 20, 2019 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants