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.

Sign up for GitHub

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

Jump to bottom

Maintain camera heading, pitch, and roll on zoom #5603

Merged
merged 7 commits into from
Sep 8, 2017
Merged

Maintain camera heading, pitch, and roll on zoom #5603

merged 7 commits into from
Sep 8, 2017

Conversation

wallw-teal
Copy link
Contributor

This fixes #4639

It occurred to me that there might be a better way to do this under the hood, but this seemed to be the best way through the API, with which I'm more familiar.

@hpinkos
Copy link
Contributor

hpinkos commented Jul 10, 2017

Thanks for the pull request @wallw-bits! Can you please send us a CLA so we can review this?

@bagnell do you want to take a look at this?

@wallw-teal
Copy link
Contributor Author

You should have a corporate CLA from us as of this morning. I had a brief look at the build failure in Travis but that build target works fine locally so I'm unsure what is going on.

@hpinkos
Copy link
Contributor

hpinkos commented Jul 10, 2017

Yes, thanks! We did receive that. Make sure you add yourself to the corporate section of CONTRIBUTORS.md

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 11, 2017

@ggetz could you please review this?

@pjcozzi pjcozzi requested a review from ggetz July 11, 2017 12:23
@ggetz
Copy link
Contributor

ggetz commented Jul 11, 2017

@wallw-bits Merging in master should fix the documentation errors you're getting in Travis. Also please update CHANGES.md with your fix.

@wallw-teal wallw-teal force-pushed the maintain-camera-heading-on-zoom branch from c3d8233 to bcb0ad7 Compare July 11, 2017 16:49
wallw-teal added a commit to wallw-teal/cesium that referenced this pull request Jul 11, 2017
@wallw-teal
Updated CHANGES.md for CesiumGS#4639 and pull CesiumGS#5603
bcb0ad7
ggetz
ggetz approved these changes Jul 11, 2017
View reviewed changes
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @wallw-bits !

@wallw-teal
Copy link
Contributor Author

You're quite welcome!

pjcozzi
pjcozzi requested changes Jul 11, 2017
View reviewed changes
Source/Scene/ScreenSpaceCameraController.js Outdated
@@ -446,6 +446,13 @@ define([
var scratchCartesian = new Cartesian3();
var scratchCartesianTwo = new Cartesian3();
var scratchCartesianThree = new Cartesian3();
var scratchZoomViewOptions = {
orientation: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use HeadingPitchRoll here?

Copy link
Contributor Author

@wallw-teal wallw-teal Jul 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. There's a branch block in Camera.setView() that calls out to setView3D, setViewCV, and setView2D with a HeadingPitchRoll instance, but those functions are private to Camera. I did not see any API functions in the deployed documentation for Camera that take a HeadingPitchRoll as an argument.

Were you looking for a larger change to make that possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be as simple as orientation : new HeadingPitchRoll() and keep everything else the same and this still works? We generally don't use object literals when we have explicit types. Heading/pitch/roll is a bit more awkward since that type was introduced after the separate properties were used in several places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, got it. Sure.

@wallw-teal
Copy link
Contributor Author

Made the requested changes. Should be good to go!

@ggetz
Copy link
Contributor

ggetz commented Jul 18, 2017

@pjcozzi These changes look good to me!

@mramato
Copy link
Contributor

mramato commented Jul 18, 2017

Changes needs to be moved to 1.36.

Are we sure of all of the implications of this change? Such as when following an object or 2D/CV?

@mramato
Copy link
Contributor

mramato commented Jul 18, 2017

I'm hopefully this fixes some of the touch issues from #4363, but I haven't checked. We should definitely evaluate touch controls.

@wallw-teal
Copy link
Contributor Author

@mramato is there something I need to do to move the changes to 1.36?

I didn't see any different behavior with 2D or CV, but I didn't try following an object.

@hpinkos
Copy link
Contributor

hpinkos commented Jul 26, 2017

is there something I need to do to move the changes to 1.36?

He meant that you added a comment to CHANGES.md under the 1.35.2 section. Sync your fork with our master, then add a comment under the 1.36 section.

@wallw-teal wallw-teal force-pushed the maintain-camera-heading-on-zoom branch from 040c84f to 533ce64 Compare July 26, 2017 17:08
wallw-teal added a commit to wallw-teal/cesium that referenced this pull request Jul 26, 2017
@wallw-teal
Updated CHANGES.md for CesiumGS#4639 and pull CesiumGS#5603
171d06c
@wallw-teal
Copy link
Contributor Author

Got it. Done.

@wallw-teal wallw-teal force-pushed the maintain-camera-heading-on-zoom branch from 6210bf2 to 4d17079 Compare August 2, 2017 16:04
@wallw-teal
Copy link
Contributor Author

wallw-teal commented Sep 7, 2017 •
edited
Loading

Is there a better way to do these pull requests that does not require constantly rebasing or merging due to changes in CHANGES.md or CONTRIBUTORS.md?

@ggetz
Copy link
Contributor

ggetz commented Sep 8, 2017

Ah sorry @wallw-bits, usually we'd like to merge pull requests in faster so that isn't a problem.

Just update the changes once again in CHANGES.md to a new ### 1.38 - 2017-10-02 section and this should be good to merge, @pjcozzi.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 8, 2017

Very nice, thanks again @wallw-bits!

@pjcozzi pjcozzi merged commit 10f78b5 into CesiumGS:master Sep 8, 2017
@pjcozzi pjcozzi mentioned this pull request Sep 8, 2017
Closed
@lilleyse
Copy link
Contributor

It seems like there might be a bug related to this PR: https://groups.google.com/forum/#!topic/cesium-dev/50CK4haCIiw.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 15, 2017

@wallw-bits any ideas?

@lilleyse please submit a new issue that links to this PR and the above forum issue.

@ggetz ggetz mentioned this pull request Sep 18, 2017
Closed
@ggetz
Copy link
Contributor

ggetz commented Sep 18, 2017

Submited #5837

@wallw-teal
Copy link
Contributor Author

The additional call to camera.setView() should not affect its position, at least according to how I understand the documentation. However, it most certainly is affecting the position as commenting out that one line fixes the problem (but leaves #4639 unresolved in some cases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjcozzi pjcozzi pjcozzi requested changes

@ggetz ggetz ggetz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Maintain camera heading on zoom
6 participants
@wallw-teal @hpinkos @pjcozzi @ggetz @mramato @lilleyse