-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
FileLoader: Allow HTTP Range requests #24580
base: dev
Are you sure you want to change the base?
Conversation
src/loaders/FileLoader.js
Outdated
const offset = this.offset; | ||
const length = this.length; | ||
const isRangeRequest = offset !== null; | ||
const key = url + ( isRangeRequest ? `:${offset}-${length}` : '' ); |
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.
Please fix linting (spaces in ${ something }
).
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.
@mrdoob Are the spaces needed even in ${ }
? I couldn't find it in the coding style guide and the lint test passes. Also I can find some lines not having spaces in ${ }
in the core
lines2.push( `${line === errorLine ? '>' : ' '} ${line}: ${lines[ i ]}` ); |
Hi and thank you @takahirox!
There's a lot I'm not sure about within this workflow...
gltf-transform partition in.glb out.gltf --meshes We've supported lazy-loading in GLTFLoader for a while, and glTF-Transform has supported partioning data into multiple .bin files for a while, but honestly I don't get the feeling that very many users are making much use of either of these features. So I am a bit worried about adding complexity to GLTFLoader for what feels like a different way of doing the same thing. I am happy with the idea of THREE.FileLoader supporting range requests. I am still trying to decide about GLTFLoader. There are some other use cases we could think about too, like streaming mip levels (low to high res) of KTX2 textures with HTTP Range Requests, the KTX2 format is designed to allow that. It is just all a tradeoff in terms of complexity and goals of GLTFLoader. 😅 |
Hi Don @donmccurdy , thanks for the comments.
I think it would be better to separate Most of your comments look |
I think @elalish has a few opinions about this. |
It is possible too creating an streaming linear LOD for progressive loader. I made this in sea3d for texture and geometry, it is more efficient once it not needed multiples request. Example: |
This looks great, but my interests are in GLTF, so I'll comment in #24506. |
The fallback feels dicey to me. We're creating a new FileLoader for each request, progressive loading will make multiple requests, and caching is disabled by default. This may lead to downloading a full asset multiple times if the server doesn't support range requests. I'd suggest that HTTP Range requests should be opt-in for downstream classes like GLTFLoader and KTX2Loader, and should fail in FileLoader if the server does not support them, or at least log warnings. I'm also wondering if HTTP Range requests might be better routed through a new Promise-based method, and just not be supported through the const loader = new FileLoader();
loader.onProgress(() => { ... });
const result = await loader.loadAsync('path/to/file.glb', options); The |
Curious to know if we can expect the browser's cache. Or are the response data distinguished because they are responses to different range headers requests? If browser cache works, might be acceptable. I will check Chromium so far.
I'm ok that FileLoader fails for 200 response against range request as I raised as another option because it can avoid additional complexity in
I may like this idea. Honestly I don't prefer |
5613054
to
c8c86c1
Compare
c8c86c1
to
c378d59
Compare
I have updated the PR to simplify. New proposed API (no change from the current API).
I have omitted Don's overriding request header idea is interesting. But I want to focus on minimal change in this PR. I hope we can think of better API in another PR if needed. And I changed to always call Changes
|
I'm not sure this is the best. |
Adding workarounds adds complexity. I don't think HTTP range requests is main stream use case so I don't think |
What blocks this PR? Do we need more discussion about fallback? |
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.
The new simplified version of the PR looks good to me.
I want to add one comment for fallback.
And I speculate even duplicated full range requests won't be a big deal thanks to browser's disk cache. If you folks think the name "HttpError" is confusing because 200 is not an error, we may add "RangeRequestError". |
@takahirox lengthcomputable maybe false positive because fileloader doesn't handle content-encoding header, if server responds with compressed content, or #24962 (comment), |
Thanks for the comment. If I understand correctly, the problem is not range requests specific so it won't block this PR, and we can work on resolving it in another PR if possible and needed, isn't it? |
@takahirox yes. |
Fixed #24485
Update: The proposed API has been updated. See #24580 (comment)
Description
This PR introduces HTTP range requests to
FileLoader
.It is helpful to save network usage by downloading only needed part of a file.
Proposed API
Changes
FileLoader.setRange(offsetInBytes, lengthInBytes)
method.FileLoader.setRequestHeader()
butFileLoader
want to know the range because of the following items so having a new clear method would be simpler (by avoiding to use regex forthis.requestHeader.Range
)Cache
(for fetched data cache) andloading
(for duplicated requests management)..slice()
forarraybuffer
andblob
response types, and throw an error for other types because of the complexity to rescue them. Another option may be throw an error regardless of response types and ask users for fallback in theironError
callback.Example use case
glB bundle + glTF LOD extension + progressive loading. First load the lowest levels and then progressively load higher levels on demand.
Currently
FileLoader
andGLTFLoader
don't support HTTP range requests so they load the entire content. With HTTP range request they will be able to partially load files so can save network usage.Codes
Additional context
When previously HTTP range requests support is discussed, (if I understand correctly) there seems to be a chance that content length in
onProgress
callback can't be computable if servers support gzip Content-Encoding.But the recent
FileLoader
already checks if the content length is computable and it's notified to the callback. So, non-computable content length may not be a big deal (?).three.js/src/loaders/FileLoader.js
Lines 112 to 114 in f5e2d49