Cleaned up VR implementation #76

Manishearth · 2018-03-10T01:13:05Z

Fixed up https://github.com/pcwalton/pathfinder/pull/75

Actually mergeable.

I still want to improve the camera code. It currently just overwrites the rotation matrix, but we can do better.

Ideally the code gets refactored into having two view transforms.

r? @pcwalton

pcwalton

Looks good, just needs a few stylistic changes. Thanks for working on this!

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/renderer.ts

@@ -146,7 +146,9 @@ export abstract class Renderer {
        this.clearDestFramebuffer();

        // Start timing rendering.
-        if (this.timerQueryPollInterval == null) {
+        if (this.timerQueryPollInterval == null &&


The entire commit that makes timer queries optional shouldn't be necessary now since I made this change right before showing my slides at the Rust meetup.

pcwalton · 2018-03-25T20:48:02Z

demo/client/html/3d-demo.html.hbs

+                 class="btn btn-outline-secondary pf-toolbar-button"
+                 aria-expanded="false" aria-controls="#pf-vr"
+                 style="display: none">
+                    <span>VR</span>


Note to self: we use Material Design Icons here and there is a VR icon, so let's use that.

huh, I looked for one and didn't find it. Will look again

google/material-design-icons#508

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/view.ts

@@ -174,6 +175,10 @@ export abstract class DemoView extends PathfinderView implements RenderContext {
    protected colorBufferHalfFloatExt: any;

    private wantsScreenshot: boolean;
+    private vrDisplay: VRDisplay | null;


nit: can you put a newline above this? Let's keep all the VR-specific stuff together.

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/view.ts

+        if ("VRFrameData" in window) {
+           this.vrFrameData = new VRFrameData;
+        } else {
+            this.vrFrameData = null;


nit: weird indentation here

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/view.ts

+    }
+
+    vrSetup(): void {
+        if (navigator.getVRDisplays) {


nit: Let's use if ('getVRDisplays' in navigator) { ... } for consistency with the above.

pcwalton · 2018-03-25T20:48:02Z

shaders/gles2/demo-3d-monument.fs.glsl

@@ -25,22 +25,28 @@ uniform float uShininess;
 /// The normal of these vertices.
 uniform vec3 uNormal;

+uniform bool uLightThings;


Cute name, but I think that uEnableLighting might be more obvious :)

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/3d-demo.ts

+        if ("VRFrameData" in window) {
+           this.vrFrameData = new VRFrameData;
+        } else {
+            this.vrFrameData = null;


nit: weird indentation

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/3d-demo.ts

+            });
+        } else {
+            // no vr support
+        }


Stylistic comments left on the earlier version of this function apply here too.

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/renderer.ts

@@ -149,6 +149,9 @@ export abstract class Renderer {
        gl.viewport(0, 0, this.destAllocatedSize[0], this.destAllocatedSize[1]);
    }

+    setClipPlanes(display: VRDisplay) {


Why this function? (Also needs a return type annotation.)

pcwalton · 2018-03-25T20:48:02Z

demo/client/src/3d-demo.ts

+                this.inVRRAF = true;
+                this.redraw();
+                this.inVRRAF = false;
+            };


Ah, I see you made the arrow function change. Disregard the previous comments then :)

Manishearth · 2018-03-26T08:37:58Z

addressed

pcwalton · 2018-03-26T17:08:24Z

@Manishearth I found a community icon here: https://materialdesignicons.com/icon/virtual-reality

pcwalton · 2018-03-26T20:26:38Z

(Note that this still has merge conflicts just as a reminder)

Manishearth · 2018-03-26T21:47:57Z

pushed. I'll fix the icon later, seems to be trickier since it's not in the font and I'll have to size it myself (and it's late)

Manishearth · 2018-03-29T12:15:50Z

Okay -- so I looked into it; that icon is only in the fonts provided on https://materialdesignicons.com/getting-started , however these work a completely different way; they don't work by ligaturifying the right text. Instead, they use a bunch of CSS with ::before attributes being used with Private Use codepoints.

I'm not sure if we should switch to that model, seems tricky to style.

pcwalton · 2018-03-29T17:37:07Z

We could always just embed the SVG.

…

On Thu, Mar 29, 2018 at 5:15 AM, Manish Goregaokar ***@***.*** > wrote: Okay -- so I looked into it; that icon is only in the fonts provided on https://materialdesignicons.com/getting-started , however these work a completely different way; they don't work by ligaturifying the right text. Instead, they use a bunch of CSS with ::before attributes being used with Private Use codepoints. I'm not sure if we should switch to that model, seems tricky to style. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/pcwalton/pathfinder/pull/76#issuecomment-377216561>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJoyeIS5mOAdXGIxnne-wvn2laqp-mMks5tjNB5gaJpZM4SlEsA> .

Manishearth · 2018-04-02T22:41:38Z

Pushed. It's hard to get it at the same size, but it's close enough.

pcwalton · 2018-04-03T02:09:17Z

Looks good, squash into a few commits and let's get it merged :)


        Remove dependence on frag_depth


        Make it possible to enter VR mode


        Use correct projection and view matrices


        Use correct viewport and clear correctly


        Turn off lighting for VR


        Move VR stuff to ThreeDView


        Match screen size as much as possible


        Various review cleanups

Manishearth · 2018-04-03T02:10:43Z

Squashed the fixups, kept the main commits so that it's easy to read.

pcwalton · 2018-04-03T02:11:30Z

Merged!

Manishearth mentioned this pull request Mar 10, 2018

Super hacky webvr support #75

Closed

Manishearth force-pushed the Manishearth:vr-better branch 2 times, most recently from 478dbbe to b57a7dd Mar 20, 2018

pcwalton reviewed Mar 25, 2018

View changes

Manishearth force-pushed the Manishearth:vr-better branch from 7600a2e to cbf334e Mar 26, 2018

Manishearth added 8 commits Mar 9, 2018

Remove dependence on frag_depth

7e11582

Make it possible to enter VR mode

975685c

Use correct projection and view matrices

148e77d

Use correct viewport and clear correctly

3af3635

Turn off lighting for VR

2b0b699

Move VR stuff to ThreeDView

0751da8

Match screen size as much as possible

3d223c1

Various review cleanups

8ed4e96

Manishearth force-pushed the Manishearth:vr-better branch from e6c7a76 to 8ed4e96 Apr 3, 2018

pcwalton merged commit 6f839cd into servo:master Apr 3, 2018

Manishearth deleted the Manishearth:vr-better branch Apr 3, 2018

servo / pathfinder

Cleaned up VR implementation #76

Cleaned up VR implementation #76

Manishearth commented Mar 10, 2018 •

edited

pcwalton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Mar 26, 2018

pcwalton commented Mar 26, 2018

pcwalton commented Mar 26, 2018

Manishearth commented Mar 26, 2018 •

edited

Manishearth commented Mar 29, 2018

pcwalton commented Mar 29, 2018

Manishearth commented Apr 2, 2018

pcwalton commented Apr 3, 2018

Manishearth commented Apr 3, 2018

pcwalton commented Apr 3, 2018

servo / pathfinder

Join GitHub today

Cleaned up VR implementation #76

Cleaned up VR implementation #76

Conversation

Manishearth commented Mar 10, 2018 • edited

pcwalton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Mar 26, 2018

pcwalton commented Mar 26, 2018

pcwalton commented Mar 26, 2018

Manishearth commented Mar 26, 2018 • edited

Manishearth commented Mar 29, 2018

pcwalton commented Mar 29, 2018

Manishearth commented Apr 2, 2018

pcwalton commented Apr 3, 2018

Manishearth commented Apr 3, 2018

pcwalton commented Apr 3, 2018

Manishearth commented Mar 10, 2018 •

edited

Manishearth commented Mar 26, 2018 •

edited