-
Notifications
You must be signed in to change notification settings - Fork 1.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
Extract only required references at Sketchfab Zip worker #5574
Extract only required references at Sketchfab Zip worker #5574
Conversation
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.
Looking good to me, thanks.
} | ||
|
||
const rewriteUri = uri => { | ||
fileMap[uri] = fileMap[uri] || zip.extractAsBlobUrl(uri); |
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.
Note to other reviewers: I haven't checked the ZipLoader
API yet but I assume the return value from zip.extractAsBlobUrl(uri)
is not null
or emptry strings.
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.
Checking ziploader It always returns a blob url. Also it already keeps track of extracted uris so we may simplify here just calling the ziploader method
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 for checking.
Also it already keeps track of extracted uris so we may simplify here just calling the ziploader method
The current fileMap[uri] = fileMap[uri] || zip.extractAsBlobUrl(uri);
is ok to me. it already keeps track of extracted uris
is ZipLoader internal design. There might be a chance they ZipLoader will change it in the future version.
gltfJson.buffers && gltfJson.buffers.forEach(b => (b.uri = fileMap[b.uri])); | ||
gltfJson.images && gltfJson.images.forEach(i => (i.uri = fileMap[i.uri])); | ||
gltfJson.buffers && gltfJson.buffers.forEach(b => (b.uri = rewriteUri(b.uri))); | ||
gltfJson.images && gltfJson.images.forEach(i => (i.uri = rewriteUri(i.uri))); | ||
fileMap["scene.gtlf"] = URL.createObjectURL(new Blob([JSON.stringify(gltfJson, null, 2)], { type: "text/plain" })); |
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.
Note to other reviewers: I assume the file name scene.gtlf
is not in gltfJson.buffers
or gltfJson.images
.
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.
Note to other reviewers: Probably scene.gtlf
should be renamed to scene.gltf
here and at https://github.com/mozilla/hubs/blob/8433044e520f25a62f5f3ac37f086b8d887f1952/src/components/gltf-model-plus.js#L651 but I want to do it in another PR because a single PR should include a single change logically.
Thanks for the contribution! |
Fixes #5469