Maintain camera heading, pitch, and roll on zoom #5603
Conversation
|
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. |
|
Yes, thanks! We did receive that. Make sure you add yourself to the corporate section of CONTRIBUTORS.md |
|
@ggetz could you please review this? |
|
@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
force-pushed
the
maintain-camera-heading-on-zoom
branch
from
c3d8233
to
bcb0ad7
Compare
|
You're quite welcome! |
| @@ -446,6 +446,13 @@ define([ | |||
| var scratchCartesian = new Cartesian3(); | |||
| var scratchCartesianTwo = new Cartesian3(); | |||
| var scratchCartesianThree = new Cartesian3(); | |||
| var scratchZoomViewOptions = { | |||
| orientation: { | |||
There was a problem hiding this comment.
Can we use HeadingPitchRoll here?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahh, got it. Sure.
|
Made the requested changes. Should be good to go! |
|
@pjcozzi These changes look good to me! |
|
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? |
|
I'm hopefully this fixes some of the touch issues from #4363, but I haven't checked. We should definitely evaluate touch controls. |
|
@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. |
He meant that you added a comment to |
wallw-teal
force-pushed
the
maintain-camera-heading-on-zoom
branch
from
040c84f
to
533ce64
Compare
|
Got it. Done. |
wallw-teal
force-pushed
the
maintain-camera-heading-on-zoom
branch
from
6210bf2
to
4d17079
Compare
|
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? |
|
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 |
|
Very nice, thanks again @wallw-bits! |
|
It seems like there might be a bug related to this PR: https://groups.google.com/forum/#!topic/cesium-dev/50CK4haCIiw. |
|
@wallw-bits any ideas? @lilleyse please submit a new issue that links to this PR and the above forum issue. |
|
Submited #5837 |
|
The additional call to |
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.