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

Replace CesiumViewerWidget #838

Merged
merged 93 commits into from
Jun 14, 2013
Merged

Replace CesiumViewerWidget #838

merged 93 commits into from
Jun 14, 2013

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jun 4, 2013

This isn't quite ready yet, but I'm opening it up for feedback so you guys can help me put the finishing touches on everything.

This change removes the Dojo-based CesiumViewerWidget and replaces it with a leaner, more streamlined Viewer widget which does not depend on dojo and is part of the combined Cesium.js. Unlike CesiumViewerWidget, which was basically an entire application in widget form, the new Viewer widget is meant to serve as a foundation for new applications. The new Viewer is also fully documented and unit tested.

The new base widget is very simple. It includes all of the standard widgets, but allows you to specify at construction time which ones you actually want to use. It also handles resize and layout so that things look good no matter which widgets you turn on or off. Finally, it provides a DataSourceCollection instances which makes it easy to add/remove/manage multiple data sources and will also be forward compatible with the to-be-developed TableOfContents widget.

The widget has very little built in functionality in order to make it as flexible as possible. For example, there's no "click-to-zoom" or "drag&drop" functionality. Instead, you can extend the base functionality by adding mix-in types, ViewerDropHandler and ViewerDynamicSceneControls are the current two we have. This allows users to trade flexibilty for built-in functionality with just one or two lines of code.

There's still a few more things that need to be done before this is ready for master.

  1. Change the utility classes to extend Viewer as actual mix-in styles objects. Also need to finish up ViewerDynamicSceneControls.
  2. Fix resize behavior so that logo and credits move to the bottom or left when Timeline or Animation are not shown.
  3. While I've updated all of the Sandcastle examples already, we need to verify that the ones that no longer need Dojo have been updated to use the non-dojo buckets.
  4. Do a final pass on documentation.
  5. Update CHANGES
  6. Replace chrome frame check (@shunter)
  7. Add back loading indicator for data sources. (@mramato)

Other changes in this branch

  1. The Client CZML SandCastle demo has been removed. It's redunant with the Simple CZML demo and will become less and less relevant as we improve client-side data generation in the DynamicScene layer.
  2. The Two Viewer Widgets SandCastle demo has been removed. This is because we don't actually have "real" support for multiple widgets yet and doing so properly is not something we are ready to undertake yet. We will add back a multi-scene example when we have a good architecture for it in place.
  3. highlightObject has been removed. I was originally planning on sticking this function in Scene/highlightObject.js, however, now that I've looked at it, I'd rather just remove it completely. It's only used in 5 of the current Sandcastle examples and I don't really feel it adds that much since we have the code highlighting in place anyway. Also, the nature of its current implementation doesn't really make it useful in general (in my opinion). I'd love to see us add a highlightPrimitive function in the future, preferably one that uses post-process or some other graphics means that doesn't involve modifying the object being highlighted; but I don't think the current one is worth being kept around. If there are strong objections to removing it, we could potentially add it as a mix-in to the standard Viewer control and add doc and specs for it so that we can verify it's functionality.
  4. Fixed an issue in BaseLayerPicker where destroy wasn't properly cleaning everything up.
  5. Extracted all of the default imagery providers in the BaseLayerPicker to a helper function, BaseLayerPicker\createDefaultBaseLayers.js
  6. Added the ability to unsubscribe to the Timeline events. The Timeline itself still needs a lot of cleanup, but that's outside the scope of this request.
  7. Added a screenSpaceEventHandler property to CesiumWidget. Also added a sceneMode option to the constructor to set the starting scene mode.

CC @shunter @emackey

Add ability to control which widgets are created at construction time.  Also properly implement destroy and clean up some Timeline code to allow it to be destroyed.
Conflicts:
	Source/Widgets/BaseLayerPicker/BaseLayerPicker.js
	Source/Widgets/FullscreenButton/FullscreenButton.js
	Source/Widgets/lighter.css
	Source/Widgets/widgets.css
This change primarily adds back functionality to the Cesium Viewer that is now considered app specific, in this case, the existing query parameters.  Also made a few other tweaks.
1. Replace all usage of CesiumViewerWidget with the new Viewer widget.
2. Delete `Client CZML` example becasue it's no longer needed and redundant with `Simple CZML Demo`
3. Delete `Two Viewer Widgets` because we aren't ready to support multiple scenes the correct way.  The technique used by this demo resulted in poor performance and lots of data duplication.
Conflicts:
	Specs/DynamicScene/DataSourceDisplaySpec.js
Conflicts:
	Apps/CesiumViewer/CesiumViewer.js
	Apps/Sandcastle/gallery/Simple CZML Demo.html
	Apps/Sandcastle/gallery/Terrain.html
	Source/Widgets/Dojo/CesiumViewerWidget.js
	Source/Widgets/FullscreenButton/FullscreenButton.js
1. Make the new Viewer follow the new API style of other widgets.
2. Fix Sandcastle examples so that they all run.
3. Fix specs.
@mramato
Copy link
Contributor Author

mramato commented Jun 12, 2013

I got rid of media queries and resize everything based on the widget size in Javascript. I'd like to move to all CSS in the future, but for now I think this is the best we can do (it actually works very nicely). Can you guys take a look and let me know what you think.

@mramato
Copy link
Contributor Author

mramato commented Jun 12, 2013

Okay guys, I've built release and doc and ran through all of the Sandcastle examples and everything looks good to me. As far as I'm concerned there's nothing left for me to do in this branch. I know @shunter has the chrome frame check he's finishing up. After that, @emackey it would be good if you could put this through the final paces and merge if you're happy.

@emackey
Copy link
Contributor

emackey commented Jun 13, 2013

That was fast!

@mramato
Copy link
Contributor Author

mramato commented Jun 13, 2013

Couch potato with a laptop is a productive open source combination 😄

@emackey
Copy link
Contributor

emackey commented Jun 13, 2013

Load CesiumViewer/?source=Gallery/simple.czml. Use the middle mouse button on the Earth's surface to rotate the camera: It works. Next, click any orbit line to have the camera follow a vehicle. Then click the "Home" button to fly home. Now try the middle mouse on the Earth again: No movement.

@mramato
Copy link
Contributor Author

mramato commented Jun 13, 2013

@emackey I think what you're seeing is #544.

@shunter
Copy link
Contributor

shunter commented Jun 13, 2013

New Chrome Frame check is in. I'm going to take another review pass over this whole PR.

@mramato
Copy link
Contributor Author

mramato commented Jun 13, 2013

I was wrong, the tilt issues was not #544. I just fixed it.

@mramato
Copy link
Contributor Author

mramato commented Jun 13, 2013

Okay, I'm done. I'll let @shunter finish his review before making any changes. (if they are even needed)

@shunter
Copy link
Contributor

shunter commented Jun 13, 2013

This all looks good to me. @emackey merge when satisfied.

@emackey
Copy link
Contributor

emackey commented Jun 14, 2013

Can we / Should we switch Hello World to use this new Viewer? I ask because new Viewer is awesome.

@mramato
Copy link
Contributor Author

mramato commented Jun 14, 2013

I think the concern with doing that is giving people the impression that Viewer "is" Cesium (i.e. an app with a set widgets). I don't feel super strongly about it either way, but I think @pjcozzi might. Why not merge this first and then we can open a small one-line change pull to switch over Hello World and then let people comment about it there.

@emackey
Copy link
Contributor

emackey commented Jun 14, 2013

All further comments will be new issues. Happy to merge this one 👍

emackey added a commit that referenced this pull request Jun 14, 2013
@emackey emackey merged commit bba1734 into master Jun 14, 2013
@emackey emackey deleted the viewer branch June 14, 2013 14:35
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.

3 participants