Skip to content

Temporary disabling the terrain#3165

Merged
offtherailz merged 3 commits intogeosolutions-it:masterfrom
baloola:cesium_terrain
Aug 29, 2018
Merged

Temporary disabling the terrain#3165
offtherailz merged 3 commits intogeosolutions-it:masterfrom
baloola:cesium_terrain

Conversation

@baloola
Copy link
Copy Markdown
Contributor

@baloola baloola commented Aug 20, 2018

Description

the terrain provider was disabled, the app renders the ellipsoid instead.

Issues

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 20, 2018

Coverage Status

Coverage increased (+0.03%) to 80.942% when pulling 3cd0ce5 on baloola:cesium_terrain into ce0f7c2 on geosolutions-it:master.

let {type, ...tpOptions} = rawOptions.terrainProvider;
switch (type) {
case "cesium": {
overrides.terrainProvider = new Cesium.CesiumTerrainProvider(tpOptions);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should stay as is is if it can work after the API changes, with new options
The case entry, otherwise, removed.
If a terrainProvider must be added, you should instead change the default terrainProvider option to "ellipsoid" (in viewer and localConfig, and add an entry in this switch case for "ellipsoid", that is also the switch case "default".
doing this, also if we do not support cesium anymore, the switch will fall back to ellipsoid.

Copy link
Copy Markdown
Member

@offtherailz offtherailz Aug 27, 2018

Choose a reason for hiding this comment

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

Sorry, I didn't see the comment in the related issue. So please change the defaults (viewer and localConfig) and add a default/ellipsoid entry to this switch. It is a cleaner solution.

terrainProvider: {
type: "cesium",
type: "ellipsoid",
url: "https://assets.agi.com/stk-terrain/world",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know I'm boring, but url and requestVertexNormals are not needed anymore, please leave only "type"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

of course the same in the other file, please always remove lines of code and configurations that are not needed

@offtherailz offtherailz merged commit 26143fd into geosolutions-it:master Aug 29, 2018
offtherailz pushed a commit to offtherailz/MapStore2 that referenced this pull request Aug 29, 2018
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.

Update Cesium Terrain

3 participants