Remove deprecated ways of setting terrain exaggeration and a bugfix #9766
Conversation
|
Thanks for the pull request @IanLilleyT!
Reviewers, don't forget to make sure that:
|
|
Ok ignore the last paragraph in the previous comment. I got it working by calling The main downside is the |
| GlobeSurfaceTile.prototype.updateExaggeration = function ( | ||
| tile, | ||
| frameState, | ||
| quadtree | ||
| ) { | ||
| var surfaceTile = this; | ||
| var mesh = surfaceTile.renderedMesh; | ||
| if (mesh === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| // Check the tile's terrain encoding to see if it has been exaggerated yet | ||
| var exaggeration = frameState.terrainExaggeration; | ||
| var exaggerationRelativeHeight = frameState.terrainExaggerationRelativeHeight; | ||
| var hasExaggerationScale = exaggeration !== 1.0; | ||
|
|
||
| var encoding = mesh.encoding; | ||
| var encodingExaggerationScaleChanged = encoding.exaggeration !== exaggeration; | ||
| var encodingRelativeHeightChanged = | ||
| encoding.exaggerationRelativeHeight !== exaggerationRelativeHeight; | ||
|
|
||
| if (encodingExaggerationScaleChanged || encodingRelativeHeightChanged) { | ||
| // Turning exaggeration scale on/off requires adding or removing geodetic surface normals | ||
| // Relative height only translates, so it has no effect on normals | ||
| if (encodingExaggerationScaleChanged) { | ||
| if (hasExaggerationScale && !encoding.hasGeodeticSurfaceNormals) { | ||
| var ellipsoid = tile.tilingScheme.ellipsoid; | ||
| surfaceTile.addGeodeticSurfaceNormals(ellipsoid, frameState); | ||
| } else if (!hasExaggerationScale && encoding.hasGeodeticSurfaceNormals) { | ||
| surfaceTile.removeGeodeticSurfaceNormals(frameState); | ||
| } | ||
| } | ||
|
|
||
| encoding.exaggeration = exaggeration; | ||
| encoding.exaggerationRelativeHeight = exaggerationRelativeHeight; | ||
|
|
||
| // Notify the quadtree that this tile's height has changed | ||
| if (quadtree !== undefined) { | ||
| quadtree._tileToUpdateHeights.push(tile); | ||
| var customData = tile.customData; | ||
| var customDataLength = customData.length; | ||
| for (var i = 0; i < customDataLength; i++) { | ||
| // Restart the level so that a height update is triggered | ||
| var data = customData[i]; | ||
| data.level = -1; | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
This is mostly the same as before. It was moved because it's now called from GlobeSurfaceTile.processStateMachine
CHANGES.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ### 1.85 - 2021-09-01 | |||
|
|
|||
| ##### Breaking Changes :mega: | |||
|
|
|||
| - Removed `Scene.terrainExaggeration` and `options.terrainExaggeration` for `CesiumWidget`, `Viewer`, and `Scene`, which were deprecated in CesiumJS 1.82. Use `Globe.terrainExaggeration` instead. | |||
There was a problem hiding this comment.
Looks like this was deprecated in 1.83?
|
Thanks @IanLilleyT |
Terrain exaggeration was made dynamic in CesiumJS 1.82 with #9603
This PR deprecates some ways to set terrain exaggeration, including:
options.terrainExaggerationinViewer.jsoptions.terrainExaggerationinCesiumWidget.jsoptions.terrainExaggerationinScene.jsterrainExaggerationgetter / setter inScene.jsGlobe.terrainExaggerationis the only way to set terrain exaggeration as of this PRThis PR also includes a bugfix caused by using
forEachRenderedTileinstead offorEachLoadedTile. When the exaggeration changes it updates the exaggeration of all rendered tiles, but after zooming out some fully loaded tiles become renderable again and don't have the updated exaggeration and geodetic surface normals. This is especially noticeable when setting the exaggeration to1.0, then to2.0, then zooming out:One fix is to do
forEachLoadedTileevery frame until all tiles have the updated exaggeration - but as noted in #9603 (comment) this has a performance cost because a partially loaded tile may stay partially loaded forever if you don't move the camera. And so you're wasting cycles on this loop over all loaded tiles.So this PR is a bit of a compromise. It goes back to
forEachLoadedTileinstead offorEachRenderedTile, but it only does it for one frame. The main downside is if the exaggeration changes while a tile is being created in a worker it will store an old exaggeration value. In practice this happens way less often than the original problem, but I found it once:This is tricky to fix too, because the only place where you can check if exaggeration changes without doing a whole new loop is somewhere in the quadtree traversal, possibly by calling
GlobeSurfaceTile.updateExaggerationinsideGlobeSurfaceTileProvider.computeTileVisibility. But it's starting to make things messy, especially sinceupdateExaggerationneeds some objects that are not normally passed tocomputeTileVisibility, like thequadtreeitself (in order to update clamp-to-terrain callbacks). So right now this PR replaces a more severe bug with a less severe one.