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

WebVR implementation #941

Merged
merged 1 commit into from Mar 25, 2019

Conversation

Projects
None yet
5 participants
@georgios-uber
Copy link
Contributor

commented Mar 1, 2019

No description provided.

@georgios-uber georgios-uber self-assigned this Mar 1, 2019

@georgios-uber georgios-uber requested review from Pessimistress and ibgreen Mar 1, 2019

@ibgreen ibgreen requested a review from tsherif Mar 2, 2019

@ibgreen

ibgreen approved these changes Mar 2, 2019

Copy link
Contributor

left a comment

FYI - You can now mark a PR as Draft when you created it (New PR button has a dropdown) so your don't need to write WIP in the header.

Show resolved Hide resolved examples/gltf/app.js Outdated
Show resolved Hide resolved examples/gltf/app.js Outdated
Show resolved Hide resolved examples/gltf/package.json Outdated
Show resolved Hide resolved examples/gltf/app.js Outdated
.translate([0, 0, -this.translate])
.rotateX(pitch)
.rotateY(roll);
const uView = new Matrix4(Array.from(this.frameData[`${eye}ViewMatrix`]));

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 2, 2019

Member

It looks like the frameData matrices are already typed arrays: https://developer.mozilla.org/en-US/docs/Web/API/VRFrameData/leftViewMatrix

So you can get rid of the copies you're making here.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 3, 2019

Author Contributor

If I don't make copies the whole browser crashes! Not sure why.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 4, 2019

Member

Wow, that sounds like a serious browser bug. You should file a report, here for Chrome or here for Firefox. There are security implications to being able to crash the browser from JS, so they'll definitely want to hear about it.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 4, 2019

Author Contributor

It did not crash just became super-slow. Suspect it's a memory leak.
I'm using an extension for WebVR simulation, so it could be the extension and not the browser.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 4, 2019

Contributor

Please add a TODO comment in the code explaining why we leave these in. We need to go back and investigate the situation later.


const uProjection = new Matrix4().perspective({fov: radians(40), aspect, near: 0.1, far: 9000});
// const uProjection = new Matrix4().perspective({fov: radians(40), aspect, near: 0.1, far: 9000});
const uProjection = new Matrix4(Array.from(this.frameData[`${eye}ProjectionMatrix`]));

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 2, 2019

Member

Same as above. Don't need to make the copies.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 3, 2019

Author Contributor

Same as above.

@georgios-uber georgios-uber force-pushed the web-vr-test branch 2 times, most recently from 86bed4d to cb7813a Mar 3, 2019

@georgios-uber georgios-uber changed the title **WIP** WebVR Test WebVR prototype Mar 9, 2019

@georgios-uber georgios-uber requested a review from tsherif Mar 9, 2019

@georgios-uber georgios-uber changed the title WebVR prototype WebVR prototype implementation Mar 9, 2019

@georgios-uber georgios-uber changed the title WebVR prototype implementation WebVR implementation Mar 9, 2019

@georgios-uber georgios-uber force-pushed the web-vr-test branch 3 times, most recently from 9e6c3d7 to 8afc267 Mar 9, 2019

@ibgreen
Copy link
Contributor

left a comment

  • Some doc updates in animation-loop.md?
  • Some article about how to use VR?
}

const displays = await navigator.getVRDisplays();
if (displays && displays.length) {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 11, 2019

Contributor

Maybe return false here to reduce indentation.

@@ -242,6 +303,57 @@ export default class AnimationLoop {
return this.props.onFinalize(...args);
}

async enableWebVR() {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 11, 2019

Contributor

Let's use underscore and document these as experimental methods.


this.vrDisplay = displays[0];
this.vrPresenting = false;
this.vrButton = createEnterVRButton({

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 11, 2019

Contributor

Nit; I still think it would be nice to be able to trigger split rendering without VR controller, to make it easy to test/debug this mode when you don't have devices handy.

// TODO: Consider resizing canvas to match vrDisplay.getEyeParameters()

this.vrDisplay = displays[0];
this.vrPresenting = false;

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 11, 2019

Contributor

Nit; vrPresenting => vrActive?

frameData: new VRFrameData()
};
this.vrPresenting = true;
this.vrButton.style.display = 'none';

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 11, 2019

Contributor

No button to exit or is that provided by default?

Show resolved Hide resolved modules/core/src/core/animation-loop.js Outdated
}

_animationFrameDevice() {
return this.vrPresenting ? this.vrDisplay : window;

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 11, 2019

Contributor

Beware: Take care to avoid referencing window in case we are running under Node.js

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 13, 2019

Author Contributor

hmmmm....

Show resolved Hide resolved modules/core/src/utils/webvr.js Outdated
Show resolved Hide resolved modules/core/src/core/animation-loop.js Outdated
Show resolved Hide resolved modules/core/src/utils/webvr.js Outdated
Show resolved Hide resolved modules/core/src/webgl/utils/request-animation-frame.js Outdated
@tsherif

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@georgios-uber @ibgreen maybe an approach would be to create an AnimationLoopVR subclass of AnimationLoop and have the custom code go in there?

@ibgreen

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@georgios-uber @ibgreen maybe an approach would be to create an AnimationLoopVR subclass of AnimationLoop and have the custom code go in there?

Yes that is an option. For experimental code we can do what we want.

I did want it to be easy to VR enable all existing luma.gl examples. It will require some changes to the demo code as they will have to start handling the VR matrices as arguments in onRender, so one could also upgrade all examples to use the new VRAnimationLoop.

That said, it might look slightly nicer in the demo code if we don't modify all demos to use an experimental animation loop class, but just inject an "underscore" parameter to support this.

@tsherif

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

I just don't want to have a lot of VR-specific logic cluttering up the AnimationLoop class for an uncommon use case.

What if we had a function that applies necessary tooling to the AnimationLoop?

   	if (webvr) {
		animationLoop = createVRAnimationLoop(animationLoop);
	}

This should work with a parameter too, e.g. in the AnimationLoop constructor just return the tooled animationLoop if the parameter is present.

@ibgreen

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

This should work with a parameter too, e.g. in the AnimationLoop constructor just return the tooled animationLoop if the parameter is present.

I think we should avoid bundling the "tooled" AnimationLoop when we don't use it (via tree-shaking).

This means that the base AnimationLoop class can't know of it, because then it will automatically be bundled.

So the application needs to import it and supply/inject it. And in that case it might just as well instantiate it directly, since now the application knows there is a VRAnimationLoop?

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I agree in making VRAnimationLoop. Let me work on that.

@georgios-uber georgios-uber requested review from ibgreen and tsherif Mar 12, 2019

@georgios-uber georgios-uber added this to the V 7.0.0 milestone Mar 12, 2019

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

class VRAnimationLoop extends AnimationLoop {...}
Is that ok @ibgreen @tsherif

@Pessimistress
Copy link
Contributor

left a comment

I recommend creating an experimental VRAnimationLoop class instead of injecting code into the core AnimationLoop. 1) This is not needed by most users and 2) we do not have a consistent process to ensure that it continues to work.

@tsherif

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@georgios-uber that sounds good. And I'm thinking it should be in its own module, not in core.

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Its own module? What name? I think this is too much.

@ibgreen

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

class VRAnimationLoop extends AnimationLoop {...} Is that ok?

  • Yes put in separate class and file and export it from core with underscore export {default as _VRAnimationLoop} from './src/vr-animation-loop that should streamline approval.
  • Once landed and working we can discuss a tighter integration.

@georgios-uber georgios-uber force-pushed the web-vr-test branch 3 times, most recently from 436cbd1 to f83c11d Mar 13, 2019

@georgios-uber georgios-uber requested a review from Pessimistress Mar 13, 2019

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

class VRAnimationLoop extends AnimationLoop {...} Is that ok?

  • Yes put in separate class and file and export it from core with underscore export {default as _VRAnimationLoop} from './src/vr-animation-loop that should streamline approval.
  • Once landed and working we can discuss a tighter integration.

DONE.

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@ibgreen please advise on module name

@ibgreen

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@ibgreen please advise on module name

See #981

@georgios-uber georgios-uber force-pushed the web-vr-test branch 2 times, most recently from 5a6a48d to 7f3b3c5 Mar 14, 2019

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

This is now moved to addons. @Pessimistress Can I land?

@ibgreen

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

This is not moved to addons.

You mean: this is now moved to addons!

We are trying to get test coverage up, the coveralls integration is not working, but please add at least some trivial test to help prevent our test coverage from dropping.

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Sure

@georgios-uber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Ping ping.

@ibgreen
Copy link
Contributor

left a comment

The code at this point is looking good.

We need docs and tests for the new class.

For tests I still think it would be a nice idea to have a "mock" WebVRDisplay that renders with two slightly offset viewMatrices (or even the same view matrix twice).

That would both allow you to test this in e.g. headless mode and also demo the split screen effect without having hardware attached.

@@ -268,7 +270,7 @@ export class DemoApp {
};
}

onInitialize({gl, canvas}) {
onInitialize({gl, canvas, _loop: animationLoop}) {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Nit; I think you want to put long helper methods like initializeEventHandling last in the class to make it easier for readers to "scan".

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Nit: Any chance you can use async/await instead of then below?

@@ -268,7 +270,7 @@ export class DemoApp {
};
}

onInitialize({gl, canvas}) {
onInitialize({gl, canvas, _loop: animationLoop}) {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Where is _loop used? Cut?

@@ -371,8 +373,7 @@ export class DemoApp {
});
}

onRender({gl, time, width, height, aspect}) {
gl.viewport(0, 0, width, height);
onRender({gl, time, aspect, vrViewMatrix, vrProjectionMatrix}) {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Idea: Should we just call these viewMatrix and projectionMatrix and have the base AnimationLoop.onRender() supply them (as identity matrices)?

That way all demos can be written to use them and transparently work with both animation loop classes?

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 24, 2019

Author Contributor

ok

@@ -382,12 +383,15 @@ export class DemoApp {
this.translate * Math.cos(roll) * Math.cos(-pitch)
];

const uView = new Matrix4()
// TODO: find how to avoid using Array.from() to convert TypedArray to regular array
const uView = new Matrix4(vrViewMatrix ? Array.from(vrViewMatrix) : null)

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

If you are creating a Matrix4 from an existing array, you are already copying the matrix into the instance. Why would we need another copy before that copy? Did you run into any problems here?

If the matrices were already guaranteed to be provided (as suggested above) you could cut this code.

.translate([0, 0, -this.translate])
.rotateX(pitch)
.rotateY(roll);

const uProjection = new Matrix4().perspective({fov: radians(40), aspect, near: 0.1, far: 9000});
const uProjection = vrProjectionMatrix

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Similar comments on copy etc.

@ibgreen
Copy link
Contributor

left a comment

Need to land asap to make it into 7.0, please clean up and add TODOs/open task for unaddressed things.

@georgios-uber georgios-uber force-pushed the web-vr-test branch from 7f3b3c5 to d21ad73 Mar 24, 2019

@coveralls

This comment has been minimized.

Copy link

commented Mar 24, 2019

Coverage Status

Coverage decreased (-0.4%) to 45.998% when pulling c7c6fcc on web-vr-test into f1ebdf7 on master.

@georgios-uber georgios-uber force-pushed the web-vr-test branch 2 times, most recently from 73df522 to c82d44d Mar 25, 2019

@georgios-uber georgios-uber force-pushed the web-vr-test branch from c82d44d to c7c6fcc Mar 25, 2019

@georgios-uber georgios-uber merged commit 652b1d9 into master Mar 25, 2019

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
coverage/coveralls Coverage decreased (-0.4%) to 45.998%
Details
license/cla Contributor License Agreement is signed.
Details

@georgios-uber georgios-uber deleted the web-vr-test branch Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.