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

change widget home button to use camera flights #803

Merged
merged 14 commits into from
Jun 11, 2013
Merged

change widget home button to use camera flights #803

merged 14 commits into from
Jun 11, 2013

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented May 28, 2013

No description provided.

@@ -120,12 +123,14 @@ define(['../createCommand',
*/
this.transitioner = transitioner;

this.flightDuration = flightDuration;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an observable knockout.observable(flightDuration). It also needs doc, something like "Gets an Observable whose value is the duration in milliseconds of the flight to home position."

@hpinkos
Copy link
Contributor Author

hpinkos commented May 29, 2013

@mramato better?

@emackey
Copy link
Contributor

emackey commented May 29, 2013

The camera flight has some weirdness. From the home position, spin the globe to the opposite side, looking at India, and zoom in about 50%. Then click the home button. The globe actually leaves the screen and comes back. I'm pretty sure it's @bagnell's code doing this (sorry Dan!) so if needed this could be moved to a separate issue.

@@ -121,11 +124,16 @@ define(['../createCommand',
this.transitioner = transitioner;

/**
* Gets an Observable whose value is the duration in milliseconds of the flight to home position.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs @type Observable

@mramato
Copy link
Contributor

mramato commented May 29, 2013

So is the issue @emackey pointed out definitely outside this pull request? @hpinkos did I notice you opening a separate issue for it?

@hpinkos
Copy link
Contributor Author

hpinkos commented May 29, 2013

@mramato, I agree with @emackey that it's the CameraFlightPath that's generating that behavior. I think it should be a separate issue. I've opened issues for two other camera flight path things, not one for this yet though. Should I open an issue?

@emackey
Copy link
Contributor

emackey commented May 29, 2013

I'm OK with this being merged before the above three are fixed. Is there some mechanism to disable flights for users that don't want them?

@mramato
Copy link
Contributor

mramato commented May 29, 2013

Settings flightDuration to 0 should do that, but we should make sure it works as expected.

@mramato
Copy link
Contributor

mramato commented May 29, 2013

This is awesome, but I did find one more problem. Load simple.czml and click on the facility. When you hit home, it stays locked onto the facility even though it returns to the home position. I don't believe this happens in master. I think the transforms are no longer being set properly. We need to make sure this works for each scene mode.

* The command for switching to home view.
* @type Command
*/
this.command = createCommand(function() {
viewHome(that.scene, that.ellipsoid, that.transitioner);
viewHome(that.scene, that.ellipsoid, that.transitioner, that.flightDuration);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of that.flightDuration, pass in that.flightDuration() so that the viewHome just deals with the raw value and not the observable.

@hpinkos
Copy link
Contributor Author

hpinkos commented May 29, 2013

@mramato when duration is 0, nothing happens. and good catch with the transforms thing. Do you think these changes should be made in CameraFlightPath or just for the home button? I was thinking CameraFlightPath because I feel like this should be default behavior for all camera flights.

@mramato
Copy link
Contributor

mramato commented May 29, 2013

Probably a question for @bagnell but I think the answer is the home button, unless the CameraFlightPath stuff is earth specific. Dan?

@hpinkos
Copy link
Contributor Author

hpinkos commented May 29, 2013

@bagnell can you take a look at the changes I made to CameraFlightPath? I decided to do the camera position change in the onComplete function so it will execute when it's added to an AnimationCollection. Do you think that's reasonable? Thanks =)

@mramato
Copy link
Contributor

mramato commented May 30, 2013

@bagnell don't forget to look at this.

var newOnComplete = function() {
var position = destination;
if (frameState.mode === SceneMode.SCENE3D) {
dirScratch = defaultValue(description.direction, position.negate(dirScratch).normalize(dirScratch));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should only do this if both direction and up are undefined. Otherwise, just compute right as direction.cross(up).

Copy link
Contributor

Choose a reason for hiding this comment

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

Compute the direction in the if below when its undefined and just use description.direction in the else block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for up. The reason to do this is that the arguments to defaultValue will be evaluated when its called.

@bagnell
Copy link
Contributor

bagnell commented May 31, 2013

@hpinkos Looks good to me. I just had some comments about computing the camera orientation.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 10, 2013

Thanks @bagnell, I finally got the 2D check for same camera positino to work. Is there anything else I need to change?

}
upScratch = defaultValue(description.up, rightScratch.cross(dirScratch, upScratch));
} else {
dirScratch = defaultValue(description.direction, Cartesian3.UNIT_Z.negate(dirScratch));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment.

@bagnell
Copy link
Contributor

bagnell commented Jun 11, 2013

@hpinkos Just a few more comments. Make sure to add tests so that the new conditionals are executed.

@mramato Want to do the final merge when this is ready?

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 11, 2013

@bagnell anything else?

@mramato
Copy link
Contributor

mramato commented Jun 11, 2013

@bagnell I'm in a meeting all day, you can go ahead and merge this if you're happy with it.

@bagnell
Copy link
Contributor

bagnell commented Jun 11, 2013

Merging.

bagnell added a commit that referenced this pull request Jun 11, 2013
change widget home button to use camera flights
@bagnell bagnell merged commit 45cd217 into CesiumGS:master Jun 11, 2013
@hpinkos hpinkos deleted the homeButtonCameraFlight branch June 11, 2013 20:49
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