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

HeadsetAdjustment scene for demoing #547

Merged
merged 8 commits into from Apr 11, 2017
Merged

Conversation

@vbandi
Copy link

@vbandi vbandi commented Mar 3, 2017

Ensure proper headset adjustment for demo scenarios.

When demoing an app, it is important to ensure that the user has the headset on properly. This PR adds a scene that shows a border at the edge of the user's field of view and instructs him/her to adjust the headset until all four edges are visible. The user (or the demonstrator) can then air-tap or say "I'm ready" to proceed to the actual experience. The next scene is then loaded and the experience may begin. This next scene is determined by either the scene build order in the Unity Build Settings dialog or by explicitly specifying the scene name that follows.

}

// Use this for initialization
void Start () {

This comment has been minimized.

@StephenHodgson

StephenHodgson Mar 3, 2017
Contributor

Nit: please add private access modifier.

This comment has been minimized.

@vbandi

vbandi Mar 4, 2017
Author

Done

// Update is called once per frame
void Update () {

}

This comment has been minimized.

@StephenHodgson

StephenHodgson Mar 3, 2017
Contributor

We can probably remove this update if we're not going to use it.

This comment has been minimized.

@vbandi

vbandi Mar 4, 2017
Author

Done

@StephenHodgson
Copy link
Contributor

@StephenHodgson StephenHodgson commented Mar 3, 2017

Thanks @vbandi, this is a simple and great addition!

else
{
var sceneIndex = SceneManager.GetActiveScene().buildIndex;
SceneManager.LoadScene(sceneIndex + 1);

This comment has been minimized.

@thebanjomatic

thebanjomatic Mar 3, 2017

The documentation for SceneManager.LoadScene isn't clear about what happens if the provided sceneBuildIndex is out of range. Do we need to be defensive against the edge-case where the HeadsetAdjustment scene is the only (or last) scene in the build?

This comment has been minimized.

@vbandi

vbandi Mar 3, 2017
Author

It throws an exception. I decided not to be defensive about this, since the exception thrown by Unity itself is clear enough, and the property's description clearly points to what went wrong. Repeating / rephrasing the exception wouldn't have added much.

This comment has been minimized.

@thebanjomatic

thebanjomatic Mar 3, 2017

Sounds good, thanks for verifying.

@thebanjomatic
Copy link

@thebanjomatic thebanjomatic commented Mar 3, 2017

I am building this now to test it out, but there is a possible typo.
The textmesh game object is named "InstructionsTest", should that be "InstructionsText" instead?

@vbandi
Copy link
Author

@vbandi vbandi commented Mar 3, 2017

Yes, you are correct. Nice catch!

@StephenHodgson
Copy link
Contributor

@StephenHodgson StephenHodgson commented Mar 3, 2017

Should we unregister from the input manager as a global listener after we've changed scenes?

@thebanjomatic
Copy link

@thebanjomatic thebanjomatic commented Mar 3, 2017

For some reason I am having a really hard time focusing on the text its making me go cross-eyed. It looks like the text is being placed 177.3 meters in front of you. I'm also experiencing issues with the outline fading out in the corners because of how thin the lines are and the way the display does that rainbow effect.

@vbandi
Copy link
Author

@vbandi vbandi commented Mar 3, 2017

@thebanjomatic hmm, I'll check this in a few days, but I don't have a HL with me right now. Distances were surely strange in this scene, but I tried to place things a few meters in front of me.

@thebanjomatic
Copy link

@thebanjomatic thebanjomatic commented Mar 3, 2017

I was able to tweak things to place the canvas 2m in front of the camera. Here were my settings (can submit a pull request off your fork if that is easier):

Image:

image

InstructionText:

  1. Switch things to be based on UITextPrefab instead of 3DTextPrefab
  2. Parented to the same canvas as the image

image

Canvas:

  • World Space, 2m in front on the Z-axis
  • Had to scale things down until everything fit reasonably inside the frame
    image

InputManager:

  • Set the Target Override to be the canvas object + track velocity. This reduces color separation when you turn your head.

image

Sorry about the giant screenshots, I'm on a high-dpi machine right now

@vbandi
Copy link
Author

@vbandi vbandi commented Mar 3, 2017

@thebanjomatic I really appreciate your help. Yes, please submit a PR as my next week is going to be super busy with much flying, so it could help getting this feature out the door much sooner.

@thebanjomatic
Copy link

@thebanjomatic thebanjomatic commented Mar 6, 2017

@vbandi Just loading that project in 5.6 introduced a bunch of changes that I wasn't expecting so it looks like I will need to install 5.5 before making that pull request. I'll try to get that done tomorrow.

@StephenHodgson
Copy link
Contributor

@StephenHodgson StephenHodgson commented Mar 6, 2017

@thebanjomatic is 5.6 even out yet?

What sort of changes have you seen?

I'd stick with 5.5.2f1 for now.

@thebanjomatic
Copy link

@thebanjomatic thebanjomatic commented Mar 6, 2017

Yeah, 5.6 has been in beta for a little while and is scheduled to be released on the 31st. Unfortunately I don't have enough storage to spare on my machine to install both 5.5 and 5.6 so I've just been running with the beta. What can I say, I like life on the bleeding edge!

@thebanjomatic
Copy link

@thebanjomatic thebanjomatic commented Mar 6, 2017

@StephenHodgson You can see the commit here: thebanjomatic@6a81e75

The serialization version changed, and there were also a handful of other settings relating to the new lighting mode options in 5.6. Enough differences to the file to make me wary of pushing.

@StephenHodgson
Copy link
Contributor

@StephenHodgson StephenHodgson commented Mar 6, 2017

Yeah, I've been trying to keep the toolkit at the latest release (5.5.2f1 is still pending though, so anyone want to review that I'll update accordingly.)

@vbandi
Copy link
Author

@vbandi vbandi commented Mar 13, 2017

I've implemented all changes and suggestions above. IMHO, the PR is ready to be merged. Please do so if you agree.

###[Scenes](Scenes)
---

####HeadsetAdjustment.unity

This comment has been minimized.

@NeerajW

NeerajW Mar 21, 2017

Should this be a Scenes folder under Tests following the convention in other folders?

This comment has been minimized.

@NeerajW

NeerajW Mar 21, 2017

Utilities\Tests\Scenes basically. We don't want to force app developers to use this and hence I recommend putting this under Tests.

This comment has been minimized.

@vbandi

vbandi Mar 21, 2017
Author

@NeerajW If you don't agree with my argument below, I'll move this under Utilities\Tests\Scenes as requested.

Having an entire scene as a building block is new to the HoloToolkit, but I feel that putting it under Utilities/Scenes is the right thing to do.

I put it under Scenes because it is not a test scene for a prefab or a script, but is supposed to be used as a whole scene. The Utilities\Tests\Scenes would suggest that this is an example or test playground on how to use another part of the toolkit. But HeadsetAdjustment is a stand alone building block.

Developers are not forced to use it, they can select what scenes are in their project in the Build Settings dialog. It is not even included by default.

This comment has been minimized.

@vbandi

vbandi Mar 21, 2017
Author

Also, just to make sure: this scene is intended to be used similar to a "Loading" scene, not as a starting point for an entire app. The real app will be a separate scene after this one.

This comment has been minimized.

@jessemcculloch

jessemcculloch Apr 3, 2017
Member

I happen to agree with @vbandi here, @NeerajW. I think the scene as a whole should be included as a tool, not as a test.

This comment has been minimized.

@StephenHodgson

StephenHodgson Apr 3, 2017
Contributor

I also agree this should be included in tools, and not tests

This comment has been minimized.

@jessemcculloch

jessemcculloch Apr 7, 2017
Member

@NeerajW - Any way you can respond here? I would love to see this move forward into the Toolkit...

This comment has been minimized.

@jessemcculloch

jessemcculloch Apr 11, 2017
Member

@StephenHodgson - Is there any way to ping @NeerajW outside of here to approve this? Or a way to push this forward?

This comment has been minimized.

@StephenHodgson

StephenHodgson Apr 11, 2017
Contributor

I'm sure he will be around. I think everyone at MSFT is gearing up for Build.

This comment has been minimized.

@StephenHodgson

StephenHodgson Apr 11, 2017
Contributor

@jessemcculloch, Pull Requests only require that two people review and approve before it can be merged. I encourage you to test it out and review to move this forward.

public class HeadsetAdjustment : MonoBehaviour, IInputClickHandler, ISpeechHandler {

[Tooltip("The name of the scene to load when the user is ready. If left empty, the next scene is loaded as specified in the 'Scenes in Build')")]
public string NextSceneName;

This comment has been minimized.

@StephenHodgson

StephenHodgson Apr 11, 2017
Contributor

There's a way to reference the scene object in the inspector instead of a string.

Can be done later.

This comment has been minimized.

@jessemcculloch

jessemcculloch Apr 11, 2017
Member

What is that method for doing so?

@StephenHodgson StephenHodgson merged commit 9c54ba3 into microsoft:master Apr 11, 2017
@vbandi
Copy link
Author

@vbandi vbandi commented Apr 11, 2017

Thank you for the merge!

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

Successfully merging this pull request may close these issues.

None yet

6 participants