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

Camera #282

Merged
merged 171 commits into from
Dec 10, 2012
Merged

Camera #282

merged 171 commits into from
Dec 10, 2012

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Oct 10, 2012

Removed Camera2DController, CameraCentralBodyController, CameraColumbusViewController, CameraFlightController, CameraFreeLookController, CameraSpindleController, and CameraControllerCollection. Common ways to modify the camera are through the CameraController object of the Camera and will work in all scene modes. The default camera mouse handler is the CameraMouseController object on the Scene. The CameraMouseController translates mouse input to camera movement through the camera's controller.

@kring
Copy link
Member

kring commented Dec 10, 2012

In the Simple CZML Demo Sandcastle example, clicking the Vehicle button throws this error:
Uncaught TypeError: Object [object Object] has no method 'getCameraMouseController' (on line 118 of http://localhost:8080/Source/DynamicScene/DynamicObjectView.js)

@kring
Copy link
Member

kring commented Dec 10, 2012

Also the "Two Canvases" example throws this error on load:
Uncaught TypeError: Object [object Object] has no method 'setSunPosition' (on line 46)

@kring
Copy link
Member

kring commented Dec 10, 2012

Last one: picking seems to be messed up in the Volumes example. Mousing over the blue-white volume usually highlights the red one instead.

@bagnell
Copy link
Contributor Author

bagnell commented Dec 10, 2012

@kring Thanks. The first two are fixed. The third is a problem in master as well.

@emackey
Copy link
Contributor

emackey commented Dec 10, 2012

Volume picking bug is #349.

@mramato
Copy link
Contributor

mramato commented Dec 10, 2012

EventHandler was renamed, but EventModifier was not. These types seem like they should be consistent with each other.


if (modeChanged) {
that._mode = that.scene.mode;
that.scene.getScreenSpaceCameraController().enableTranslate = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be a big deal (and could be reserved for future cleanup) but we can probably cache the result of getScreenSpaceCameraController() instead of calling the function every time, right? Same for other calls to this function throughout this file.

Copy link
Member

Choose a reason for hiding this comment

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

This might not be a big deal (and could be reserved for future cleanup) but we can probably cache the result of getScreenSpaceCameraController() instead of calling the function every time, right? Same for other calls to this function throughout this file.

DynamicObjectView apparently doesn't require that the scene be provided to the constructor, as long as it is set before update is called. So Dan's change as a result of this comment broke two tests in DynamicObjectViewSpec that were not providing a scene. I was just going to fix it myself, but it's not obvious to me what should change. If scene is optional at construction time and then the user can set it later, then we can't cache the result of screenSpaceCameraController in the constructor or maybe even at all.

@bagnell
Copy link
Contributor Author

bagnell commented Dec 10, 2012

@mramato I made the changes you suggested. Anything else that can be improved?

mramato added a commit that referenced this pull request Dec 10, 2012
@mramato mramato merged commit d546027 into master Dec 10, 2012
@mramato
Copy link
Contributor

mramato commented Dec 10, 2012

Looks good, merging.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 11, 2012

@emackey what touch events does this merge add? Can you please add them to CHANGES.md in master? (yes, I said master; crazy, I know).

@emackey
Copy link
Contributor

emackey commented Dec 13, 2012

Can you please add them to CHANGES.md in master?

Done.

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.

7 participants