[HUBS] FileLoader: HTTP Range requests support #68
Conversation
| @@ -19,7 +30,10 @@ class FileLoader extends Loader { | |||
|
|
|||
| url = this.manager.resolveURL( url ); | |||
|
|
|||
| const cached = Cache.get( url ); | |||
| const isRangeRequest = this.requestHeader.Range !== undefined; | |||
There was a problem hiding this comment.
Any reason for range header to be special? Might want to just include all headers in cache key.
There was a problem hiding this comment.
Because we Three.js have never heard problems for other headers so far. Perhaps we may rethink if we get problem report.
Cache key must be distinguished with range headers. Otherwise FileLoader misunderstands that different range request against the same URL are the same request. The responses must be different for different range requests.
| @@ -77,7 +91,7 @@ class FileLoader extends Loader { | |||
| fetch( req ) | |||
| .then( response => { | |||
|
|
|||
| if ( response.status === 200 || response.status === 0 ) { | |||
| if ( response.status === 200 || response.status === 206 || response.status === 0 ) { | |||
There was a problem hiding this comment.
I know its not new but I wonder what the 0 status is about.. Also again not new and maybe not worth changing but technically 200-299 are all "successful" responses.
There was a problem hiding this comment.
I know its not new but I wonder what the 0 status is about..
There was a problem hiding this comment.
wow, not sure how I missed that on the next line lol
| @@ -88,6 +102,12 @@ class FileLoader extends Loader { | |||
|
|
|||
| } | |||
|
|
|||
| if ( isRangeRequest && response.status === 200 ) { | |||
There was a problem hiding this comment.
This was metnioned in the upstream PR as well. I think its a bit odd to throw in this case. The thing using the FileLoader should decide what they want to do in this case. Ex. The GLBRangeRequest should take that full response and pass it along to GLTFLoader so the whole glb does not need to be downloaded again.
With the above suggestion about cachekey it would also make it so this PR has nothing specific about range headers and is instead just fixing cache keys to respect headers.
There was a problem hiding this comment.
See
mrdoob#24580 (comment)
mrdoob#24580 (comment)
And Mugen approved.
the whole glb does not need to be downloaded again.
As I wrote somewhere else, I think disk cache can work and duplicated full range downloads can be avoided.
There was a problem hiding this comment.
I forgot FileLoader does additional parsing and doesn't just pass you the response directly. Unfortunately that would make it hard to do what I was suggesting. As for browser cache.. Ideally we should be able to rely on that, but I think the browser isn't so great about honoring cache on large files.
There was a problem hiding this comment.
I'm not sure if I understand correctly what you suggested but
The thing using the FileLoader should decide what they want to do in this case.
This is possible. See
|
Merging this Three.js upcoming feature. If you have any design feedback, better to send it to Three.js. |
[HUBS] This change is needed for glTF LOD progressive loading. This change is not merged to the official Three.js PR yet but likely it can get in soon. mrdoob#24580 We can remove this commit if the PR is merged to the official Three.js and we upgrade our Three.js fork to the one including the change.
This PR is a cherry-pick of upcoming Three.js PR mrdoob#24580 needed for Hubs HTTP range requests support (mainly for LOD) Hubs-Foundation/hubs#5713
We can remove this commit when mrdoob#24580 will be merged to Three.js and we will upgrade our Three.js fork.