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

Implement and rename hmdEnabled to magicWindowTrackingEnabled #4528

Merged

Conversation

andreyrd
Copy link
Contributor

Description:
This is an implementation of the fix for #4520

Changes proposed:

  • Rename hmdEnabled to magicWindowTrackingEnabled
  • Pass through magicWindowTrackingEnabled setting to magicWindowControls.enabled
  • Reset magic window eulers when tracking is disabled. Otherwise you can get stuck looking at the ground. This was also what the original code was aiming to do, since previous versions used pitchObject and yawObject for gyroscope tracking. But the previous check oldData && !data.hmdEnabled && !oldData.hmdEnabled should actually have been more like oldData && !data.hmdEnabled && oldData.hmdEnabled in order to catch the transition from enabled to disabled.

this.pitchObject.rotation.set(0, 0, 0);
this.yawObject.rotation.set(0, 0, 0);
// Reset magic window eulers if tracking is disabled.
if (oldData && !data.magicWindowTrackingEnabled && oldData.magicWindowTrackingEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be

if (oldData && !data.magicWindowTrackingEnabled && !oldData.magicWindowTrackingEnabled) 
{
...
}

equivalent to old code

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 think the old code might be wrong.

My intention is that it only resets when the property changes from disabled -> enabled.
With the old check it will only change the second time that update is called with the value disabled.

So it will still work, but on the second update and will reset every update afterwards.

If I go back one more commit in git history of that line, it appears the original intent was closer to what I'm thinking with my change:
https://github.com/aframevr/aframe/blame/e9ba54ebcf724d828fba90fce9adc530e40526d6/src/components/look-controls.js#L45

!hmdEnabled && oldData && hmdEnabled !== oldData.hmdEnabled can be logically reduced to !hmdEnabled && oldData && false !== oldData.hmdEnabled and then to !hmdEnabled && oldData && oldData.hmdEnabled

Commit fe199d changed this: fe199da#diff-6285039eee70ed830f70f7d2f7b7dfa5R47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any thoughts on this? I can change it to be equivalent with the old code, even though I think the logic is wrong. It doesn't affect my use case of it anyway, so doesn't have to be my problem to deal with.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we always want to reset if it value has changed?

if (oldData && data.hmdEnabled !== oldData.hmdEnabled)

Copy link
Member

Choose a reason for hiding this comment

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

This is pretty close. What do you think about the comment above?

Copy link
Contributor Author

@andreyrd andreyrd Jun 2, 2020

Choose a reason for hiding this comment

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

Hey, sorry, have been quite busy lately. That makes sense. I think either way when enabling, the objects will almost immediately get overwritten from motion data, but I think it makes sense to always have a clean state when toggling. Especially since now it's resetting the proper magicWindowAbsoluteEuler, which means you can still set a "starting place" for motion using touch.

@dmarcos
Copy link
Member

dmarcos commented Mar 27, 2020

Thanks! I thinks there's a bug. I left a comment

@dmarcos
Copy link
Member

dmarcos commented May 13, 2020

Thanks for the patience. I think we're pretty close to merge this.

@dmarcos
Copy link
Member

dmarcos commented Jun 15, 2020

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants