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 WebXR Interstitial #3136

Merged
merged 8 commits into from Apr 17, 2020
Merged

Implement WebXR Interstitial #3136

merged 8 commits into from Apr 17, 2020

Conversation

MortimerGoro
Copy link
Contributor

@MortimerGoro MortimerGoro commented Apr 8, 2020

Fixes #1966 Fixes #3123

@MortimerGoro
Copy link
Contributor Author

The spinner animation is not the final one, @keianhzo is working on the shapeshifter exported version

@daoshengmu
Copy link
Contributor

There is a build fail on this merge:

/opt/FirefoxReality/app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/WebXRInterstitialController.java:15: error: cannot find symbol
    private WebxrInterstitialControllerBinding mBinding;

@MortimerGoro
Copy link
Contributor Author

There is a build fail on this merge:

/opt/FirefoxReality/app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/WebXRInterstitialController.java:15: error: cannot find symbol
    private WebxrInterstitialControllerBinding mBinding;

Fixed

@daoshengmu
Copy link
Contributor

I notice the controller icons only be shown when entering the immersive mode at the 1st time. Is it what we want?

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Apr 8, 2020

For now it’s displayed for 3 seconds the first time you enter immersive (on each app restart)
I don’t like forcing it every EnterVR.

After trying it I think it would be good to wait for a user click to dismiss the how-to information, and remove the forced time. IMO:

  • The first time you go to WebXR show the how-to-exit information. Dismiss after a click, not time based. We don’t know how long takes for a user to process the info and find the button.
  • Do not show the spinner at the same time, it distracts from reading the how-to
  • Show the spinner only in the next EnterVR for the whole session

Thoughts?

Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

If three window are open, the controllers are drawn behind the spinner:
image

@bluemarvin
Copy link
Contributor

For now it’s displayed for 3 seconds the first time you enter immersive (on each app restart)
I don’t like forcing it every EnterVR.

After trying it I think it would be good to wait for a user click to dismiss the how-to information, and remove the forced time. IMO:

* The first time you go to WebXR show the how-to-exit information. Dismiss after a click, not time based. We don’t know how long takes for a user to process the info and find the button.

* Do not show the spinner at the same time, it distracts from reading the how-to

* Show the spinner only in the next EnterVR for the whole session

Thoughts?

I think this is a good approach. I think the controller images could be a little bigger maybe?

@daoshengmu
Copy link
Contributor

daoshengmu commented Apr 9, 2020

For now it’s displayed for 3 seconds the first time you enter immersive (on each app restart)
I don’t like forcing it every EnterVR.

After trying it I think it would be good to wait for a user click to dismiss the how-to information, and remove the forced time. IMO:

  • The first time you go to WebXR show the how-to-exit information. Dismiss after a click, not time based. We don’t know how long takes for a user to process the info and find the button.
  • Do not show the spinner at the same time, it distracts from reading the how-to
  • Show the spinner only in the next EnterVR for the whole session

Thoughts?

I feel we can remove the spinner if we are showing the how-to-exit information, the spinner sometimes catches my attention other than the how-to-exit. However, if users end the information icon, WebXR pages are still loading. We should still show the spinner instead.

Make the controller image bigger can let me see the words more easy. I also feel if we already allow users to press a button to end the how-to-exit information, maybe longer time like 5 secs would be great for user have more time to understand it.

@MortimerGoro MortimerGoro force-pushed the v10/webxr_interstitial branch 5 times, most recently from e3943e9 to b1db18a Compare April 9, 2020 17:09
Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

I think we can remove spinners_v3.obj, spinners_v3.mtl, and spinners_v3.ktx.

Comment on lines +31 to +33
case ViveFocusPlus:
name = "Vive Focus Plus";
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a ViveFocus also?

m.drawHandler = [=](device::Eye eye) {
DrawLoadingAnimation(eye);
DrawWebXRInterstitial(eye);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Looks like there are two extra spaces added?

Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

I think we still want to have support for the regular Focus controller.

import org.mozilla.vrbrowser.BuildConfig;

public class DeviceType {
// These values need to match those in Device.h
@IntDef(value = { Unknown, OculusGo, OculusQuest, ViveFocusPlus, PicoNeo2, PicoG2 })
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like Device.h needs to be updated?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need ViveFocus as well

@daoshengmu
Copy link
Contributor

Do we have an alpha blending problem on the spinner image? I can see its rectangle background when we are going to end the animation.

@MortimerGoro
Copy link
Contributor Author

@daoshengmu @bluemarvin I implemented the flow we talked about. Let me know what you think. Feel free to change the new text

@daoshengmu
Copy link
Contributor

@daoshengmu @bluemarvin I implemented the flow we talked about. Let me know what you think. Feel free to change the new text

It seems like pressing squeeze btns on Quest can't exit the how-to-exit page.

@MortimerGoro
Copy link
Contributor Author

@daoshengmu @bluemarvin I implemented the flow we talked about. Let me know what you think. Feel free to change the new text

It seems like pressing squeeze btns on Quest can't exit the how-to-exit page.

Fixed

@MortimerGoro
Copy link
Contributor Author

Do we have an alpha blending problem on the spinner image? I can see its rectangle background when we are going to end the animation.

I noticed that too but it seems a driver bug. It only happens with the first layer displayed on a black background. If I change the order the alpha blending moves to the first layer rendered. I'll look into it next week, we can address that as a follow-up


// This is always false in Oculus Browser.
const bool thumbRest = false;
controller->SetButtonState(controllerState.index, ControllerDelegate::BUTTON_OTHERS, 5, thumbRest, thumbRest);
} else {
Copy link
Contributor

@daoshengmu daoshengmu Apr 9, 2020

Choose a reason for hiding this comment

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

It looks odd to me. This is thumbRest instead of the grip button, and thumbRest is always false. If thumbRest will cause problems to you, Neo2 will have it too because it also has thumbRest.

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 remove this commit now. I already gave a fix at #2521 by defining a new btn state BUTTON_SQUEEZE. That makes sense for us to use it in app.

@keianhzo keianhzo merged commit 93b3f9f into master Apr 17, 2020
@keianhzo keianhzo deleted the v10/webxr_interstitial branch April 17, 2020 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants