-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Request Scheduler from 3d-tiles #3476
Changes from 7 commits
be9e9ce
3f38546
a1cc29b
172da97
8d698e9
28c03c1
38b9d55
da3420d
b9e8042
0b3b1cc
bfb356c
cfe237f
228bb06
40b9293
bf9119e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ define([ | |
'./defaultValue', | ||
'./defined', | ||
'./defineProperties', | ||
'./deprecationWarning', | ||
'./DeveloperError', | ||
'./Event', | ||
'./GeographicTilingScheme', | ||
|
@@ -19,8 +20,9 @@ define([ | |
'./Math', | ||
'./OrientedBoundingBox', | ||
'./QuantizedMeshTerrainData', | ||
'./Request', | ||
'./RequestType', | ||
'./TerrainProvider', | ||
'./throttleRequestByServer', | ||
'./TileAvailability', | ||
'./TileProviderError' | ||
], function( | ||
|
@@ -32,6 +34,7 @@ define([ | |
defaultValue, | ||
defined, | ||
defineProperties, | ||
deprecationWarning, | ||
DeveloperError, | ||
Event, | ||
GeographicTilingScheme, | ||
|
@@ -43,14 +46,15 @@ define([ | |
CesiumMath, | ||
OrientedBoundingBox, | ||
QuantizedMeshTerrainData, | ||
Request, | ||
RequestType, | ||
TerrainProvider, | ||
throttleRequestByServer, | ||
TileAvailability, | ||
TileProviderError) { | ||
'use strict'; | ||
|
||
/** | ||
* A {@link TerrainProvider} that access terrain data in a Cesium terrain format. | ||
* A {@link TerrainProvider} that accesses terrain data in a Cesium terrain format. | ||
* The format is described on the | ||
* {@link https://github.com/AnalyticalGraphicsInc/cesium/wiki/Cesium-Terrain-Server|Cesium wiki}. | ||
* | ||
|
@@ -489,17 +493,16 @@ define([ | |
* @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. | ||
* @param {Boolean} [throttleRequests=true] True if the number of simultaneous requests should be limited, | ||
* or false if the request should be initiated regardless of the number of requests | ||
* already in progress. | ||
* @param {Request} [request] The request object. | ||
* | ||
* @returns {Promise.<TerrainData>|undefined} A promise for the requested geometry. If this method | ||
* returns undefined instead of a promise, it is an indication that too many requests are already | ||
* pending and the request will be retried later. | ||
* | ||
* @exception {DeveloperError} This function must not be called before {@link CesiumTerrainProvider#ready} | ||
* returns true. | ||
*/ | ||
CesiumTerrainProvider.prototype.requestTileGeometry = function(x, y, level, throttleRequests) { | ||
CesiumTerrainProvider.prototype.requestTileGeometry = function(x, y, level, request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now every terrain/imagery provider sets This same idea applies to 3D tilesets too. A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think something less coarse-grained or at least named differently needs to be exposed. Have you looked at other engines using HTTP2 or doc on best practices with HTTP2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've been pretty clear that I'm squarely in the "don't throttle" crowd (I'm all for prioritization and cancellation, just not throttling) so take this with that bias up front. Just because a server supports HTTP/2, doesn't mean the client does. So it's impossible for the developer to make this decision during development. As far as I know, it's also impossible to detect at runtime whether you're using HTTP/2, so we can't do some magic on our end either. That means the best option is to not throttle at all and let the browser handle it. I tried to hunt around to see what other webmaps are doing, and as far as I can tell none of them throttle (I checked OL/Leaflet/Mapbox). OpenLayers has an open issue to cancel unneeded requests, but that's it. But since I'm not going to be able to convince you guys to not do it, I think the second best option is having a single top-level setting on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If HTTP/2 becomes widely adopted, I agree that throttling by server is a waste. However as of now throttling does make a big difference to performance: I just got these numbers for an Everest static view:
When would the client not support HTTP/2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Up until recently, not all browsers supported it, and even then there could be lots of atypical browsers (embedded, webview, etc..) that don't support it. In the long run, everyone will.
I know I'm missing something, but why are there less requests with the 4G version? Shouldn't the total number of requests be the same? (if we're just talking about throttling and not cancellation). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect that none of us individually knows enough about everything - tile selection and HTTP2 - to make a good call here. Let's get together for a few minutes today in person. @lilleyse please organize. |
||
//>>includeStart('debug', pragmas.debug) | ||
if (!this._ready) { | ||
throw new DeveloperError('requestTileGeometry must not be called before the terrain provider is ready.'); | ||
|
@@ -522,8 +525,6 @@ define([ | |
url = proxy.getURL(url); | ||
} | ||
|
||
var promise; | ||
|
||
var extensionList = []; | ||
if (this._requestVertexNormals && this._hasVertexNormals) { | ||
extensionList.push(this._littleEndianExtensionSize ? 'octvertexnormals' : 'vertexnormals'); | ||
|
@@ -532,17 +533,19 @@ define([ | |
extensionList.push('watermask'); | ||
} | ||
|
||
function tileLoader(tileUrl) { | ||
return loadArrayBuffer(tileUrl, getRequestHeader(extensionList)); | ||
if (typeof request === 'boolean') { | ||
deprecationWarning('throttleRequests', 'The throttleRequest parameter for requestTileGeometry was deprecated in Cesium 1.35. It will be removed in 1.36.'); | ||
request = new Request({ | ||
throttle : request, | ||
throttleByServer : request, | ||
type : RequestType.TERRAIN | ||
}); | ||
} | ||
throttleRequests = defaultValue(throttleRequests, true); | ||
if (throttleRequests) { | ||
promise = throttleRequestByServer(url, tileLoader); | ||
if (!defined(promise)) { | ||
return undefined; | ||
} | ||
} else { | ||
promise = tileLoader(url); | ||
|
||
var promise = loadArrayBuffer(url, getRequestHeader(extensionList), request); | ||
|
||
if (!defined(promise)) { | ||
return undefined; | ||
} | ||
|
||
var that = this; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked that passing a promise instead of a string does not occur during running the unit tests. There could be old code or demos that rely on this though.
Also the number
1.36
is just placeholder, what would be good times to remove?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a request object is now the last parameter for every
load
related function. Is this a change worth documenting here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably two releases so 1.37.
Here or below in CHANGES.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if this has already been asked and answered, but why remove this functionality? Just because we think no one is using it? Ultimately I'm okay with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I didn't really explain this. In order to throttle by server the
RequestScheduler
needs to derive the server from the string url.This used to be fine, just do a
when(url)
call first. But to signal that a request was throttled the load functions (viaRequestScheduler
) need to be able to returnundefined
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, makes sense.