Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Refactors WebVRCamera.cs into smaller scripts #215

Merged
merged 15 commits into from
Apr 7, 2018
Merged

Refactors WebVRCamera.cs into smaller scripts #215

merged 15 commits into from
Apr 7, 2018

Conversation

caseyyee
Copy link
Contributor

@caseyyee caseyyee commented Mar 27, 2018 •
edited
Loading

Basic overview:

WebVRManager

  • manages eventing, state and handling of WebVR data from browser.

WebVRControllerManager

  • Manages controller data and bindings for gameobjects.

WebVRCamera

  • manages camera rig, stereo rendering and applies view data.

WebVRController

  • Applies controller data to game objects, provides eventing for control interactions.

@cvan cvan self-requested a review March 30, 2018 02:51
cvan
cvan suggested changes Mar 30, 2018
View reviewed changes
Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

looking good, let me know when you have some updates for me to test in the headset.

also, when you get a chance, could you comment either in this PR or in the issues assigned to you in the v1.2.0 milestone (remaining Unity milestone issues: https://github.com/mozilla/unity-webvr-export/milestone/3) which issues you've fixed already in this PR (or plan to in separate PRs)?

thanks! great work 👍

Assets/WebGLTemplates/WebVR/webvr.js Outdated
@@ -87,7 +87,7 @@
return vrDisplay.requestPresent([{source: canvas}]).then(function () {
// Start stereo rendering in Unity.
console.log('Entered VR mode');
gameInstance.SendMessage('WebVRCameraSet', 'Begin');
gameInstance.SendMessage('WebVRCameraSet', 'EnterVR');
Copy link
Contributor

Choose a reason for hiding this comment

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

just FYI: @salva has changed this to OnStartVR in PR #213

Assets/WebGLTemplates/WebVR/webvr.js Outdated
@@ -100,7 +100,7 @@
}
function done () {
// End stereo rendering in Unity.
gameInstance.SendMessage('WebVRCameraSet', 'End');
gameInstance.SendMessage('WebVRCameraSet', 'ExitVR');
Copy link
Contributor

Choose a reason for hiding this comment

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

just FYI: @salva has changed this to OnEndVR in PR #213

Assets/WebVR/Scripts/WebVRController.cs
@@ -0,0 +1,34 @@
using UnityEngine;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you sort these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvan what's the conversion here? alphabetical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went alphabetical

Assets/WebVR/Scripts/WebVRController.cs
gameObject.transform.position = sitStand.MultiplyPoint(controller.position);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a newline at the EOF?

Assets/WebVR/Scripts/WebVRManager.cs
}

[System.Serializable]
private class WVRData
Copy link
Contributor

Choose a reason for hiding this comment

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

have you seen this in @delapuente's PR #213?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, have integrated that work. Class names are a bit different, but we can fix the consistency.

Assets/WebVR/Scripts/WebVRManager.cs
if (wvrData.sitStand.Length > 0)
sitStand = numbersToMatrix (wvrData.sitStand);

if (OnHeadsetUpdate != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

curly braces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it was still essentially one line with props separated out, so I left them out.

Assets/WebVR/Scripts/WebVRCamera.cs Outdated
#endif
}

private void handleVrChange()
Copy link
Contributor

Choose a reason for hiding this comment

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

you can call this onVRChange

Assets/WebVR/Scripts/WebVRCamera.cs Outdated
WebVRManager.OnVrChange += handleVrChange;
WebVRManager.OnHeadsetUpdate += handleHeadsetUpdate;
cameraMain = GameObject.Find("CameraMain").GetComponent<Camera>();
cameraL = GameObject.Find("CameraL").GetComponent<Camera>();
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be cameraMain.Find("CameraL").GetComponent<Camera>()?

Assets/WebVR/Scripts/WebVRCamera.cs Outdated

void Start()
{
WebVRManager.OnVrChange += handleVrChange;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's += do here? method concatenation? I'm not super familiar with Objective-C

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds the function to the delegate. Basically subscribing to an event. When triggered, handeVrChange() gets run.

Assets/WebVR/Scripts/WebVRCamera.cs Outdated

private IEnumerator endOfFrame()
{
// wait until end of frame to report back to WebVR browser to submit frame.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: capitalise Wait

@cvan
Copy link
Contributor

cvan commented Apr 6, 2018 •
edited
Loading

@caseyyee I tested this, and it works well. nice work! 👍 can you just rebase this one more time? thanks!

image

delapuente
delapuente reviewed Apr 6, 2018
View reviewed changes
Assets/WebGLTemplates/WebVR/webvr.js Outdated
@@ -100,7 +100,7 @@
return vrDisplay.requestPresent([{source: canvas}]).then(function () {
// Start stereo rendering in Unity.
console.log('Entered VR mode');
gameInstance.SendMessage('WebVRCameraSet', 'OnStartVR');
gameInstance.SendMessage('WebVRCameraSet', 'EnterVR');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking but, per consistency, all the events in Unity are of the form On.... Do you mind if we stick to that convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Start and End make more sense anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he means add the On prefix

@caseyyee caseyyee changed the title Refactors WebVRCamera.cs into smaller scripts [wip] Refactors WebVRCamera.cs into smaller scripts Apr 6, 2018
@cvan
Copy link
Contributor

cvan commented Apr 7, 2018

tested in Vive and Windows MR (Dell Visor), looks good. ship it! 👍

@caseyyee caseyyee merged commit 81e1870 into MozillaReality:master Apr 7, 2018
This was referenced Apr 9, 2018
Closed
Closed
@cvan cvan added this to the v1.2.0 milestone Apr 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delapuente delapuente delapuente left review comments

@cvan cvan cvan approved these changes

Assignees

@caseyyee caseyyee

Labels
None yet
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@caseyyee @cvan @delapuente