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

Extend BaseLayerPicker to support terrain #1607

Merged
merged 12 commits into from
Apr 11, 2014
Merged

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Apr 10, 2014

  1. Add Imagery and Terrain sections to BaseLayerPicker
  2. Remove ESRI World Street Map, Stmen Watercolor, and MapQuest OSM from the default provider list, these didn't add anything extra and the number of items was getting out of hand once terrain was added (Watercolor was only half the world too).
  3. Add WGS84, STK World Terrain, and smallTerrain to the default terrains.
  4. I had to rename a ton of stuff to make properties like selectedItem imagery specific (it's now selectedImagery) and there's a new corresponding selectedTerrain.
  5. The imagery section or terrain section disappears completely if no terrain or imagery view models are supplied.
  6. CentralBody.depthTestAgainTerrain is automatically enabled if anything other than WGS84 is selected.

While this was a simple and straightforward change to make, it's a large breaking change for anyone using the BaseLayerPicker directly. Thankfully I expect almost everyone is using it through Viewer, where the breaking changes are all hidden away.

@emackey As I mentioned to you, while I'm mostly happy with the new styling; feel free to tweak (or suggest tweaks) if you have ideas for improving it.

Hopefully I didn't leave anything out.

1. Turn on depth test when any terrain other than EllipsoidTerrainProvider is being used
2. All similar Viewer options for terrain that imagery already has.
Flesh out specs and minor cleanup.
@mramato
Copy link
Contributor Author

mramato commented Apr 10, 2014

I forgot to mention; I kept this two a two line update in CHANGES with a link back to this PR. Since there are so many renames that 99% of users won't care about, I didn't want to clutter things up.

@mramato
Copy link
Contributor Author

mramato commented Apr 10, 2014

Okay, one last update and now you guys can review this, I swear 😄 I put back all of the default imagery providers and simply widened the drop down so everything still fits nicely.


selectedViewModel(value);
knockout.defineProperty(this, 'selectedTerrain', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this uses a writable computed observable, selecting a terrain provider that's already selected causes the terrain to be reloaded from scratch. Instead, if it subscribed to a normal observable, you'd get the reference equality comparison for free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we talked offline, I did the same thing by just comparing the references in the setter, which in my mind is cleaner than subscribing.

1. BaseLayerPicker now takes an options object instead of individual parameters.
2. Minor styling fix to dropdown width so that 4 items fit when vertical scroll bar is visible.
3. Add selectedImageryProviderViewModel/selectedTerrainProviderViewModel options to BaseLayerPickerViewModel
4. Fix tooltips for default terrain providers.
@mramato
Copy link
Contributor Author

mramato commented Apr 10, 2014

Ready for another look.

@@ -23,6 +23,7 @@ Beta Releases
* `TilingScheme.extentToNativeRectangle` -> `TilingScheme.rectangleToNativeRectangle`
* `TilingScheme.tileXYToNativeExtent` -> `TilingScheme.tileXYToNativeRectangle`
* `TilingScheme.tileXYToExtent` -> `TilingScheme.tileXYToRectangle`
* `BaseLayerPicker` has been extended to support terrain selection. This includes many potential breaking changes, see [#1607](https://github.com/AnalyticalGraphicsInc/cesium/pull/1607) for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to briefly list all the breaking changes, just as we've done every other time. Users doing an upgrade may not have direct internet access.

1. Fix copy/paste error.
2. Add more information to CHANGES
@mramato
Copy link
Contributor Author

mramato commented Apr 11, 2014

Okay @shunter, this is ready.

shunter added a commit that referenced this pull request Apr 11, 2014
…errain

Extend BaseLayerPicker to support terrain
@shunter shunter merged commit 2f4eafa into master Apr 11, 2014
@shunter shunter deleted the baseLayerPicker-terrain branch April 11, 2014 18:34
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.

2 participants