Skip to content

Make aframe work with WebVR APIs 1.0 #1423

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
Merged
merged 1 commit into from
May 11, 2016
Merged

Make aframe work with WebVR APIs 1.0 #1423

merged 1 commit into from
May 11, 2016

Conversation

@dmarcos
Copy link
Member

@dmarcos dmarcos commented Apr 29, 2016

I made the necessary changes to adapt aframe to the new API spec.

  • I added two flags to the webvr-polyfill to disable the UI and turn-your-phone instructions. The package.json points to my fork while I submit and merge a PR on boris repo.
  • I tested on mobile (Safari iOS / Chrome for Android) and desktop Windows on Firefox (no webvr) and latest WebVR enabled Chromium builds on Rift and Vive. Everything seems to work.
dmarcos
dmarcos reviewed Apr 29, 2016
View reviewed changes
src/style/aframe.css
top: 0;
}

:-webkit-full-screen {
Copy link
Member Author

@dmarcos dmarcos Apr 29, 2016

Choose a reason for hiding this comment

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

in chrome mobile the user agent stylesheet set it to white.
Copy link
Member

@ngokevin ngokevin May 10, 2016

Choose a reason for hiding this comment

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

Worth a comment
@dmarcos dmarcos mentioned this pull request Apr 29, 2016
Closed
cvan
cvan reviewed Apr 29, 2016
View reviewed changes
src/core/scene/fullscreen.js
switch (event.data.type) {
case 'fullscreen': {
switch (event.data.data) {
case 'enter':
Copy link
Contributor

@cvan cvan Apr 29, 2016

Choose a reason for hiding this comment

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

I think we should port this logic: some postMessage handler for entering and exiting using new API
@cvan
Copy link
Contributor

cvan commented Apr 29, 2016

we should add keyboard shortcuts per https://github.com/cvan/aframe/commit/af2cfa39fe8bc72cc1875009f810f7e3c8560f13#diff-b4f74a9a2c02669787c5210f0cbed271R20
cvan
cvan reviewed Apr 29, 2016
View reviewed changes
src/core/scene/a-scene.js Outdated
}
if (!vrDisplay) { return; }
vrDisplay.requestPresent([{source: this.renderer.domElement}]);
this.addState('vr-mode');
Copy link
Contributor

@cvan cvan Apr 29, 2016

Choose a reason for hiding this comment

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

we should not blindly assume that this succeeded. we should change state and emit events only when the promise resolves à la https://github.com/cvan/aframe/commit/af2cfa39fe8bc72cc1875009f810f7e3c8560f13#diff-eb9bb1084863a2f65678ea67d6e7cf5eR135
@cvan
Copy link
Contributor

cvan commented Apr 29, 2016

thanks for doing this. I really appreciate it. I don't have my CV1 in front of me atm but I'll test as soon as I get a chance.
@cvan
Copy link
Contributor

cvan commented Apr 29, 2016

and just make sure we are using the latest VREffect and VRControls. also see this: mrdoob/three.js#8568 (comment) it's important
@dmarcos dmarcos force-pushed the webvr1 branch 4 times, most recently from 5e80595 to b1f69bc Compare May 8, 2016 00:36
cvan
cvan reviewed May 8, 2016
View reviewed changes
src/components/look-controls.js Outdated
currentHMDPosition = this.calculateHMDPosition();
deltaHMDPosition.copy(currentHMDPosition).sub(previousHMDPosition);
previousHMDPosition.copy(currentHMDPosition);
// Do nothing if we have not moved
Copy link
Contributor

@cvan cvan May 8, 2016

Choose a reason for hiding this comment

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

punctuation.
ngokevin
ngokevin reviewed May 10, 2016
View reviewed changes
tests/components/scene/vr-mode-ui.test.js Outdated
this.entityEl = entityFactory();
var el = this.el = this.entityEl.parentNode;
var resolvePromise = function () {
return new Promise(function (resolve) {
Copy link
Member

@ngokevin ngokevin May 10, 2016

Choose a reason for hiding this comment

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

Promise.resolve() will return a resolved promise
ngokevin
ngokevin reviewed May 10, 2016
View reviewed changes
src/components/scene/canvas.js Outdated
canvas.classList.add('a-canvas');
canvas.style.height = data.height + '%';
canvas.style.width = data.width + '%';
canvas.style.height = data.height + 'px';
Copy link
Member

@ngokevin ngokevin May 10, 2016

Choose a reason for hiding this comment

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

% vs px?
@dmarcos dmarcos force-pushed the webvr1 branch 2 times, most recently from f6ea8c8 to 96955c8 Compare May 10, 2016 23:35
@ngokevin ngokevin mentioned this pull request May 11, 2016
Closed
@dmarcos dmarcos force-pushed the webvr1 branch 2 times, most recently from e372fd7 to 4061edd Compare May 11, 2016 00:28
@dmarcos
Copy link
Member Author

dmarcos commented May 11, 2016

This is ready to merge.
ngokevin
ngokevin reviewed May 11, 2016
View reviewed changes
src/style/aframe.css Outdated
display: -webkit-flex;
font-family: sans-serif, monospace;
font-size: 13px;
font-size: 13px;}
Copy link
Member

@ngokevin ngokevin May 11, 2016

Choose a reason for hiding this comment

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

oops brace?
@dmarcos dmarcos force-pushed the webvr1 branch 2 times, most recently from 10000c0 to e5dd14d Compare May 11, 2016 01:20
@dmarcos dmarcos merged commit 1279ac5 into aframevr:master May 11, 2016
@ngokevin ngokevin mentioned this pull request May 11, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review

3 participants

@dmarcos @cvan @ngokevin