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

Add configurable eye separation and focal length for VR #5917

Merged
merged 7 commits into from
Oct 20, 2017

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Oct 19, 2017

Adds Scene.eyeSeparation and Scene.focalLength properties to configure VR settings.

@cesium-concierge
Copy link

@bagnell, thanks for the pull request! Maintainers, we have a signed CLA from @bagnell, so you can review this at any time.

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

@bagnell
Copy link
Contributor Author

bagnell commented Oct 19, 2017

Fixes #3909.

@lilleyse
Copy link
Contributor

Looks good.

@lilleyse lilleyse merged commit 1317bad into master Oct 20, 2017
@lilleyse lilleyse deleted the eye-separation branch October 20, 2017 21:22
@@ -653,6 +653,18 @@ define([
this._actualInvertClassificationColor = Color.clone(this._invertClassificationColor);
this._invertClassification = new InvertClassification();

/**
* The focal length for use when with cardboard or WebVR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the units and the default value.

this.focalLength = undefined;

/**
* The eye separation distance in meters for use with cardboard or WebVR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the default value.

@@ -2282,8 +2294,8 @@ define([
var savedCamera = Camera.clone(camera, scene._cameraVR);

var near = camera.frustum.near;
var fo = near * 5.0;
var eyeSeparation = fo / 30.0;
var fo = near * defaultValue(scene.focalLength, 5.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this should be done here and not at initialization like Cesium does everywhere else? Is it valid to set this back to undefined?

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2017

Is it possible to add reasonable unit tests?

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

Successfully merging this pull request may close these issues.

4 participants