Introducing FreeFlight fallback #213

delapuente · 2018-03-23T09:45:17Z

Fix #125
I need some feedback. Look at how it behaves on:

Desktop supporting WebVR: no fallback is available.
Desktop not supporting WebVR: movement and orientation fallbacks are available.
Mobile: only movement fallback would be available but the absence of keyboard renders it unusable. I'm planning to address this problem in another bug and provide two-finger navigation of the scene.

Use W, A, S, D or arrow keys to move forward, left, backward or right.
Use R, F to increase or decrease elevation.
Use Q, E to roll to the left or to the right.
Use click and drag to yaw and pitch.

cvan

Desktop not supporting WebVR: movement and orientation fallbacks are available.

first off: fantastic, fantastic work! this works spiffy! and your code handles all the position and orientation cases well 👍👍👍💯💯💯

Desktop supporting WebVR: no fallback is available.

this is confusing to me. I think we should still support the mouse-look controls ("fallback"). see my screenshots earlier in the issue.

when I'm in Chrome on Windows (with no WebVR/OpenVR flags enabled), the free-flight controls work properly and the keybindings too 👍 🎉

when I'm in Firefox on Windows (which has WebVR enabled), before I enter VR, I'd expect to have the same controls. or at a minimum, have the orientation and position in mono mode before I enter VR.

currently, in Firefox on Windows the demo scene seems broken in its current state (what with the default camera position and the scenery).

what are your thoughts on this?

see my screenshots:

Firefox (with WebVR API support)

Chrome (without WebVR API support)

Mobile: only movement fallback would be available but the absence of keyboard renders it unusable. I'm planning to address this problem in another bug and provide two-finger navigation of the scene.

awesome! sounds totally reasonable. can you file issues for those?

cvan · 2018-03-26T22:56:40Z

Assets/WebGLTemplates/WebVR/webvr.js

@@ -288,24 +288,33 @@

    return navigator.getVRDisplays().then(function(displays) {
      if (!displays.length) {
-        return null;
+        showInstruction(document.querySelector('#novr'));


nit: it can happen in #104, but the showInstruction function could be changed to accept no arguments:

el = document.querySelector('#novr');

cvan · 2018-03-26T22:57:54Z

Assets/WebGLTemplates/WebVR/webvr.js

+      if (vrDisplay.capabilities) {
+        if (vrDisplay.capabilities.canPresent) {
+          // Enable button to toggle entering/exiting VR.
+          entervrButton.dataset.enabled = 'true';


nit: again, this could be a #104 issue: I know you didn't change this, but shouldn't this be set to 'false' (or the data-enabled attribute removed) if !vrDisplay.capabilities || !vrDisplay.capabilities.canPresent?

I made a more in-depth refactor here.

cvan · 2018-03-26T23:01:47Z

Assets/WebGLTemplates/WebVR/webvr.js

-      if (vrDisplay.capabilities && vrDisplay.capabilities.canPresent) {
-        // Enable button to toggle entering/exiting VR.
-        entervrButton.dataset.enabled = 'true';
+      if (vrDisplay.capabilities) {


I know it's been a while since we wrote this code, so I can't recall the scenario but do you know the cases in which there wouldn't be a capabilities object on the VRDisplay instance? VRDisplayCapabilities is polyfilled in the WebVR Polyfill.

at least in v1.1, I think if there's a VRDisplay, there's guaranteed to be VRDisplayCapabilities.

long story short: I think we can remove this check.

cvan · 2018-03-26T23:02:21Z

Assets/WebGLTemplates/WebVR/webvr.js

+          showInstruction(document.querySelector('#novr'));
+        }
+        if (!hasOrientation) {
+          gameInstance.SendMessage('WebVRCameraSet', 'RequestFallback', 'orientation');


if vrDisplay.capabilities does not exist, this code path won't be reached (see my comment above about removing the if (vrDisplay.capabilities) statement)

cvan · 2018-03-26T23:02:54Z

Assets/WebVR/Scenes/WebVR.unity

+  m_Enabled: 1
+  m_EditorHideFlags: 0
+  m_Script: {fileID: 11500000, guid: 683f145ef879be447ba25c1080ac1984, type: 3}
+  m_Name: 


should there be a name?

It seems to be OK. Unity writes these files. If there was an error, I suppose Unity would complain about it. I don't see any error but if this is a bug, let's report it in a different bug.

cvan · 2018-03-27T00:10:08Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+
+	float ForwardMovement() {
+		return TranslationSpeed(GetDirectionFromAxis(
+			new List<KeyCode> { KeyCode.W, KeyCode.UpArrow },


can these keys be defined as configurable class properties?

I don't know, but do you think there is any significant perf overhead incurred from invoking these function calls and creating these List objects (as opposed to just doing some conditional if statements checking Input.getKey(…) for each)?

I've removed constructing the constant objects again and again. I don't think the if alternative would improve the performance in a noticeable way.

cvan · 2018-03-27T00:24:15Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+
+	float SideMovement() {
+		return TranslationSpeed(GetDirectionFromAxis(
+			new List<KeyCode> { KeyCode.D, KeyCode.RightArrow },


can these keys be defined as configurable class properties?

nice work handling all these various transforms!

I have to admit: D makes me a bit sick heh - is that a standard convention? and for that key? (I don't doubt it; I just am not used to it.)

Usually D means stride-right in FPS games. I'm convinced that Unity users and game players would find it comfortable and natural.

All the keys are now customizable except for the drag button.

cvan · 2018-03-27T00:26:12Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+		return RotationMovement(QuantifiedDirection(mouseDeltas.y));
+	}
+
+	float RotationMovement(float direction) {


this method is helpful (and DRY), but for me it was a little tricky to follow the nested calls to know the calculated values of the pitch, yaw, roll, etc.

cvan · 2018-03-27T00:28:19Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+
+	bool isRotationEnabled;
+
+	bool isTranslationEnabled;


same as comments above for isRotationEnabled

cvan · 2018-03-27T01:02:02Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+	float RollMovement() {
+		return RotationMovement(GetDirectionFromAxis(
+			new List<KeyCode> { KeyCode.Q },
+			new List<KeyCode> { KeyCode.E }


E probably makes me the sickest 🤢heh. I can see how this might be useful in particular scenes. do you think we could have some of these key options disabled by default? (I suppose like the Toggle VR Mode Key key option, we could just assume an empty value means to not respect it?)

Right, this kind of rotation is not very usual and I would prefer to remove it completely following the recommendation on customization.

cvan · 2018-03-27T02:48:45Z

Desktop not supporting WebVR: movement and orientation fallbacks are available.

oh, btw, in the default scene, the controller hands appear smooshed together at the origin.

it looks funny 😆but I assume it's a bug/oversight?

delapuente · 2018-03-27T09:44:24Z

What happened to the hands seems like a bug. They should not be displayed unless there are controls.

You can use WASD or the arrow keys to move forward/backwards, to the left/right. You can also configure all these keys. Click and drag the mouse to rotate the views. The fallback mode is enabled according to the capabilities of the VR device. In desktop environments with VR, movement and rotation are available while not presenting. In mobile environments, movement and rotation are enabled or disabled according to the capabilities of the device.

delapuente · 2018-03-27T15:12:30Z

@cvan take another look. The RequestFallback API is gone. Now we send the capabilities to Unity so the developer can decide what to do. Actually, the free-flight camera is not special in any way and it can illustrate how to create a non-VR camera for when VR is not available.

I've removed the capability of rolling and added customization options for all the keys moving the camera. Rotation is mouse/touch-screen only.

currently, in Firefox on Windows the demo scene seems broken in its current state (what with the default camera position and the scenery).

This is tricky and it is related to #184. I would provide a "default-height-for-when-no-vr" component in a different component. I can draft one tomorrow. What do you think?

cvan

looking real good! awesome improvements, and thanks for addressing everything!

cvan · 2018-03-27T22:37:17Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+		return direction * rotationSpeed * Time.deltaTime;
+	}
+
+	float DirectionFromMovement(float number, float threshold=0.001f) {


you don't think this should be configurable? probably fine, eh

cvan · 2018-03-27T22:37:27Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+		if (number < -threshold) {
+			return -1;
+		}
+		return 0;


yeah, I did - is this convention?

cvan · 2018-03-27T22:38:07Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+	}
+
+	void RegisterMouseMovement() {
+		bool stoppedTracking = Input.GetMouseButtonUp(0);


IMO "Tracking" is a weird word to use because of VR "positional tracking"

can't think of the best word: stoppedMouseDrag, mouseDragReleased, mouseClickReleased?

cvan · 2018-03-27T22:38:59Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+	}
+
+	void Update() {
+		if (translationEnabled) {


perfect, thanks!

cvan · 2018-03-27T22:41:08Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+
+public class FreeFlightController : MonoBehaviour {
+
+	[Tooltip("Enable/disable rotation control.")]


thanks for moving all the public properties and adding the tooltip text labels. they're very helpful (for me definitely anyway)

cvan · 2018-03-27T22:50:02Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+		return 0;
+	}
+
+	bool SomeIsPressed(List<KeyCode> keys) {


can you rename this to AnyKeyIsPressed or AnyKeyPressed (basically just include the word Key somewhere in this name)?

cvan · 2018-03-27T22:51:40Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+		return keys.FindAll(k => Input.GetKey(k)).Count > 0;
+	}
+
+	void RegisterMouseMovement() {


perhaps I missed it in a comment, but did you look into using the Unity MonoBehaviour's onMouseDrag?

According to the documentation:

OnMouseDrag is called when the user has clicked on a GUIElement or Collider and is still holding down the mouse.

Similar to what happened with the event systems, this is intended for interacting with things that are draggable and not as a generic way of detecting dragging on the screen.

cvan · 2018-03-27T22:53:10Z

Assets/WebVR/Scripts/UI/WebUI.cs

+using System.Runtime.InteropServices;
+public class WebUI {
+
+	[DllImport("__Internal")]


no biggie, really, just FYI: the .meta files use two soft spaces 😄

cvan · 2018-03-27T22:53:44Z

ProjectSettings/ProjectVersion.txt

@@ -1 +1 @@
-m_EditorVersion: 2017.3.0f3
+m_EditorVersion: 2017.3.1f1


want to do in separate commit? or are you fine having this?

Sure, I will commit this directly to master.

cvan · 2018-03-27T22:54:19Z

Assets/WebGLTemplates/WebVR/styles/webvr.css

@@ -130,6 +130,7 @@ a:link, a:visited {
  position: absolute;
  top: 0;
  width: 100%;
+  pointer-events: none;


I noticed this - thanks for fixing this 👍 😄

delapuente · 2018-03-28T16:36:52Z

I've managed to add a fixing offset for when there is no position capability or we are in a desktop-like environment. I've also changed the components to the main camera since to fix a couple of edge cases and cleaning some things from bug #159 that interfered with this implementation. @cvan feel free to take another look. Remember I won't be available this week.

cvan

thanks again. this looks almost perfect. great work!

just see my comments in RegisterMouseMovement about the mouse-look controls not feeling right (stuttery)

cvan · 2018-03-28T17:25:58Z

Assets/WebGLTemplates/WebVR/webvr.js

-      testTimeStart = null;
-      return;
-    }
+    // This way of passing messages is deprecated. Use rich objects instead.


this is a TODO?

cvan · 2018-03-28T17:27:20Z

Assets/WebGLTemplates/WebVR/webvr.js

+
+      if (displays.length) {
+        vrDisplay = displays[displays.length - 1];
+        canPresent = vrDisplay.capabilities.canPresent;


perhaps might be easier to just manage 1 variable called capabilities than the 4 intermediate variables?

cvan · 2018-03-28T17:28:10Z

Assets/WebGLTemplates/WebVR/webvr.js

-      }
+      gameInstance.SendMessage(
+        'WebVRCameraSet', 'OnVRCapabilities',
+        JSON.stringify({


can we not just JSON.stringify(vrDisplay.capabilities)?

can handle in #104

cvan · 2018-03-28T17:28:30Z

Assets/WebVR/Plugins/WebGL/webvr.jslib

+  },
+
+  ShowPanel: function (panelId) {
+    document.dispatchEvent(new CustomEvent('Unity', {detail: {type:'ShowPanel', panelId: Pointer_stringify(panelId)}}));


nit: space after type:

cvan · 2018-03-28T17:28:53Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+	public float translationSpeed = 7;
+
+	[Tooltip("Minimum distance the mouse must move to start registering movement.")]
+	public float rotationDeadDistance = 0.001f;


thanks 😄

cvan · 2018-03-28T17:29:59Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+	/// Enables rotation and translation control for desktop environments.
+	/// For mobile environments, it enables rotation or translation according to
+	/// the device capabilities.
+	void EnableAccordingToPlatform() {


ahh, okay makes more sense. thanks for the explanation (and filing the follow-up issue) 👍

cvan · 2018-03-28T17:30:40Z

ProjectSettings/ProjectSettings.asset

@@ -260,7 +260,7 @@ PlayerSettings:
  m_BuildTargetGraphicsAPIs: []
  m_BuildTargetVRSettings:
  - m_BuildTarget: Standalone
-    m_Enabled: 1
+    m_Enabled: 0


good call; we should have this disabled anyway

cvan · 2018-03-28T18:10:47Z

Assets/WebVR/Scripts/Input/FreeFlightController.cs

+			mouseMovement.Set(0, 0, 0);
+		}
+		else if (mouseIsDragging) {
+			mouseMovement = Input.mousePosition - lastMousePosition;


not sure if it's coming from the YawMovement or PitchMovement, but the mousedrag (especially at lower regions of the screen) doesn't seem to rotate the camera correctly; it's like a lag or stutter

filed issue #219 for enhancement. no worries! no real other concerns I have with the controls; they're totally usable and a huge improvement to the project. thanks again! 👍

cvan · 2018-04-03T06:30:18Z

thanks! this is sooo good! 🏅

caseyyee · 2018-04-03T19:51:27Z

Nice job @delapuente !
Good one 💯

delapuente requested review from cvan and caseyyee March 23, 2018 09:47

delapuente added this to the v1.2.0 milestone Mar 23, 2018

cvan suggested changes Mar 27, 2018

View reviewed changes

delapuente requested a review from cvan March 27, 2018 15:03

cvan suggested changes Mar 27, 2018

View reviewed changes

delapuente added 2 commits March 28, 2018 08:21

Addressed naming issues

72e79dc

Clarifying rotation and translation switches

cb7d0b3

delapuente requested a review from cvan March 28, 2018 07:26

Fixing initial position

735f97e

cvan suggested changes Mar 28, 2018

View reviewed changes

This was referenced Mar 30, 2018

Reset camera position after enter/exit VR #184

Closed

Refactors WebVRCamera.cs into smaller scripts #215

Merged

improve mousedrag movements in FreeFlight Controls #219

Closed

fix nit: add whitespace

f1c2e90

cvan approved these changes Apr 3, 2018

View reviewed changes

cvan merged commit ec95a92 into MozillaReality:master Apr 3, 2018

cvan mentioned this pull request Apr 4, 2018

regression: controllers in VR do not work on master #220

Closed


		public class FreeFlightController : MonoBehaviour {

		[Tooltip("Enable/disable rotation control.")]

		@@ -1 +1 @@
		m_EditorVersion: 2017.3.0f3
		m_EditorVersion: 2017.3.1f1

Introducing FreeFlight fallback #213

Introducing FreeFlight fallback #213

Conversation

delapuente commented Mar 23, 2018 • edited Loading

cvan left a comment

Choose a reason for hiding this comment

Firefox (with WebVR API support)

Chrome (without WebVR API support)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delapuente Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

delapuente Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan commented Mar 27, 2018

delapuente commented Mar 27, 2018

delapuente commented Mar 27, 2018

cvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delapuente commented Mar 28, 2018

cvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

cvan commented Apr 3, 2018

caseyyee commented Apr 3, 2018

delapuente commented Mar 23, 2018 •

edited

Loading

delapuente Mar 27, 2018 •

edited

Loading

delapuente Mar 27, 2018 •

edited

Loading

cvan Apr 3, 2018 •

edited

Loading