aframevr / aframe

:a: Web framework for building virtual reality experiences.
https://aframe.io/
MIT License
16.66k stars 3.97k forks source link

In fullscreen mode in desktop 2D / non-VR, camera height offset is removed. #3902

Closed EslamNemr closed 5 years ago

EslamNemr commented 5 years ago

Description:

1 2

dsinni commented 5 years ago

I can confirm this.

Currently reproducible in b9ad45a68c42c9ee0e96f62077cf1e43dd0a66fa.

beyerz commented 5 years ago

@EslamNemr @dsinni Can you provide a code snippet?

Just off the top of my head, I am assuming that you are using the look-controls component on your camera.

Just to explain what's happening here, going full screen or going VR is essentially the same thing, just depends on the device, but the in terms of camera position.

On look-controls, there is an event listener for "enter-vr", The current position is saved and then the position of the camera is set to "0 0 0" but when exiting vr, the saved position is restored.

/**
   * Save pose.
   */
  onEnterVR: function () {
    this.saveCameraPose();
    this.el.object3D.position.set(0, 0, 0);
    this.el.object3D.updateMatrix();
  },

  /**
   * Restore the pose.
   */
  onExitVR: function () {
    this.restoreCameraPose();
    this.previousHMDPosition.set(0, 0, 0);
  },

Not sure sure if this is a bug, but I do think that the camera rotation and position should be maintained between all states (normal, vr and full-screen)

dsinni commented 5 years ago

@beyerz This is also happening on desktop in fullscreen mode, though, so I"m thinking it's a bug.

Any scene will exhibit the issue if you use the latest master branch.

beyerz commented 5 years ago

@beyerz This is also happening on desktop in fullscreen mode, though, so I"m thinking it's a bug.

Any scene will exhibit the issue if you use the latest master branch.

Note: Skip to the bottom if you dont care for an explanation of why this also happens on desktop.

desktop fullscreen and VR, from what I understand share the same events called "enter-vr" and "exit-vr". Depending on the device the display will differ, it will either go fullscreen on desktop or split screens on mobile (VR mode).

This event is triggered(published) by the "vr-mode-ui" component and acted upon(subscribed to) by the "look-controls" component.

The specific method in "look-controls" (onEnterVR) that is triggered on these events is device agnostic.

So yes, it makes sense that is is also happening on desktop device.

While obviously, the decision is not mine as I am not a maintainer, I am simply trying to explain what is happening.

End of explanation, keep reading from here :) I will also point out that in the docs, they suggest but it is not advised that if the "look-controls" dont work to your specifications, you can write up your look controls component documentation

All this being said, I agree though that the default action should not reset the camera to (0,0,0) and during this week, if I get some time, I will look at doing a pull-request to reflect this. @dmarcos Do you agree with this request?

dsinni commented 5 years ago

@beyerz

I will also point out that in the docs, they suggest but it is not advised that if the "look-controls" dont work to your specifications, you can write up your look controls component

Yeah, but this isn't a custom specification; it's the way it has always worked up until recently. The camera y position should be utilized when in full-screen mode (no positional tracking), which is not currently the case. Definitely seems like a bug to me, unless it's an intended breaking change, which doesn't seem likely.

My guess is that it's related to this bug: https://github.com/aframevr/aframe/issues/3921

EslamNemr commented 5 years ago

@dsinni @beyerz i found that in the last master branch in looks-control its added to enter vr the following lines

`onEnterVR: function () { this.saveCameraPose(); this.el.object3D.position.set(0, 0, 0); this.el.object3D.updateMatrix(); },

/**

but in the pervisouse master branch it was `onEnterVR: function () { this.saveCameraPose(); },

/**

if i removed the new lines in look controls component seems work good with last master, but im not sure why this lines added in the new master branch.

beyerz commented 5 years ago

This change was introduced by @dmarcos for this commit

It seems that a lot of work was done to support webXR.

Also looking at look controls testing, I dont see any tests that validate enter-vr interaction.

Personally I would start by introducing the tests to mimic the expected result and then resolving the above mentioned code.

Ofcourse, we must checkin with @dmarcos in order to understand the effects of the additions he added.

mattrei commented 5 years ago

why don't you just set the position around the look-controls entity? like

<a-entity position="32 10 32">
   <a-camera look-controls>
   </a-camera>
</a-entity>

Problem solved.

dsinni commented 5 years ago

@mattrei While that could work as a temporary fix in some cases, it affects more than just the camera. For instance, it won't play nicely with nav-mesh since the entity won't come in contact with it. I suppose the additional entity could be added as a child of the camera rig that uses movement-controls, but that's getting a bit hacky.

I believe it may also add additional height in VR with tracking, which is probably undesirable.

This is still an issue worthy of attention, IMHO.

dmarcos commented 5 years ago

This is expected behavior. The way to position the camera is by wrapping it in a camera rig as @mattrei described

@dsinni There should not be problems with nav-mesh @donmccurdy blog post illustrates how to use a camera rig https://medium.com/@donmccurdy/creating-a-nav-mesh-for-a-webvr-scene-b3fdb6bed918

dsinni commented 5 years ago

@dmarcos Note that in @donmccurdy's blog post, the position offset is applied directly to the camera entity rather than the rig, which was common practice in 0.8.2.

Are you saying that the new way to approach this is to apply it directly to the rig? Won't that Y offset also be applied to the rig when using a tracked HMD? I will have to test this, but I do know that this is a breaking change.

donmccurdy commented 5 years ago

Note there are two offsets here:

<a-entity id="rig" movement-controls="constrainToNavMesh: true" position="0 0 5">
  <a-entity camera
            position="0 1.6 0"
            look-controls="pointerLockEnabled: true">
    <a-cursor nav-pointer
              raycaster="objects: [nav-mesh]"></a-cursor>
  </a-entity>
</a-entity>

The offset on the rig represents the user's position in the world, at ground level, and can be set wherever you need. The inner offset represents a default height (1.6m) for non-VR users, and the camera will override it when entering VR. I never use a camera offset other than 0 1.6 0.

dsinni commented 5 years ago

@donmccurdy Thanks, that's exactly how I would expect it to work, however the height offset on the camera is not currently being applied in master when entering fullscreen on desktop. The camera drops to 0.

donmccurdy commented 5 years ago

Oh I see. I guess I haven't used non-VR fullscreen much recently but that does sound like a regression. 😕

dsinni commented 5 years ago

@donmccurdy Yeah, that's what I was thinking, but wasn't sure what the plan was for 0.9.0. I actually use non-VR fullscreen a lot, so it's been staring me in the face for a while. Should this issue be reopened?

donmccurdy commented 5 years ago

Sounds like it, @dmarcos does that make sense?

EslamNemr commented 5 years ago

same as @dsinni, i think it should re-opened !

mattrei commented 5 years ago

same here, please reopen. also camera offset is not working in current master even for non-VR mode.

ngokevin commented 5 years ago

Steps:

  1. Go to https://a-frobot.github.io/aframe/examples/boilerplate/hello-world/
  2. Click full screen on desktop / laptop.
dsinni commented 5 years ago

Thanks! Looking good!