getTileDataAvailable() handling for terrain providers #2102
Conversation
|
Profiling was done in the scenes under Specs/Profile. Profiling method was using Chrome developer tools 'Network' tab with 'Disable cache' enabled and 'stk-terrain' as a filter. Values are as per the summary line at the bottom of the network tab. Tests were repeated to take into account variations in network traffic etc. An interval is used in the profile scenes to set the camera otherwise it is automatically pushed above the low res terrain. HalfDome.html current master... 355/3746 requests | 2.0MB / 26.9MB transferred | 1.0min tile-loading branch... 339/2912 requests | 2.0MB / 23.2MB transferred | 41.98s SanFrancisco.html current master... 294/2772 requests | 719KB / 17.2MB transferred | 43.69s tile-loading branch... 327/2669 requests | 748KB / 16.6MB transferred | 40.06s |
|
CC #526 |
| * @param {Number} level The level of the tile for which to request geometry. | ||
| * @returns {Number} Undefined if not supported by the terrain provider, otherwise true or false. | ||
| */ | ||
| TerrainProvider.prototype.getTileDataAvailable = DeveloperError.throwInstantiationError; |
There was a problem hiding this comment.
This is a breaking change for folks who implemented TerrainProvider, right?
We should provide a default implementation to not break them and perhaps warn if we want to eventually require this. See the Deprecation Guide.
There was a problem hiding this comment.
Ah, I see isDataAvailable below in GlobeSurfaceTile.js. Is that the right place? I would expect to treat this as an abstract base class instead so all users do not need to check for the function.
There was a problem hiding this comment.
Currently there are no "base classes" or inheritance in Cesium. TerrainProvider only exists as documentation of an implicit interface, so there are no breaking changes here.
There was a problem hiding this comment.
Ah, right. This is still a little sketchy. It's as if we want to document this function as optional so users know they need to check to see if the function exists or we need to start using inheritance of whatever form to avoid this.
There was a problem hiding this comment.
It's as if we want to document this function as optional so users know they need to check to see if the function exists or we need to start using inheritance of whatever form to avoid this.
I believe inheritance would have a performance cost we'd rather not pay. I agree it should be documented as optional, though.
|
Can you add tests and update CHANGES.md? Add a new 1.02 section; I don't think this will make 1.01 today. |
|
Thanks for this @chris-cooper. There is still a lot more we can do (dare, we not require loading parents before children?!?!?), but this is improvement is very welcomed. |
|
Thanks @chris-cooper. @kring this is all you to merge. I suggest a default implementation for |
| * @param {Number} x The X coordinate of the tile for which to request geometry. | ||
| * @param {Number} y The Y coordinate of the tile for which to request geometry. | ||
| * @param {Number} level The level of the tile for which to request geometry. | ||
| * @returns {Number} Undefined if not supported, otherwise true or false. |
There was a problem hiding this comment.
This should be {Boolean}, right?
|
Well spotted @shunter. And now I've spelt amend wrong in the commit comment :-( |
Rather than documenting the new function as optional, I added a |
|
OK with me. Can you add it to a new Deprecated section in CHANGES.md? |
getTileDataAvailable() handling for terrain providers
This is a tile loading optimisation for cameras that start near the globe based on advice from Kevin. It is currently only implemented for the Cesium terrain provider which knows in advance which tiles are present.