-
Notifications
You must be signed in to change notification settings - Fork 250
Read a glTF file's external data before postprocessing. #1280
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
Conversation
An new method `CesiumGltfReader::readGltfAndExternalData` has been added which reads external data and then does postprocessing. Addresses #1034
kring
left a comment
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 @timoore. A few comments below, but I'm going to fix them myself so that we can include this in today's release.
| assetFetcher.baseUrl, | ||
| options) | ||
| .thenInWorkerThread( | ||
| [&assetFetcher](CesiumGltfReader::GltfReaderResult&& gltfResult) { |
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.
It's not safe to lambda capture a parameter by reference if the lambda will outlive the function invocation.
| options) | ||
| .thenInWorkerThread( | ||
| [&assetFetcher](CesiumGltfReader::GltfReaderResult&& gltfResult) { | ||
| if (gltfResult.model) { |
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.
Looks like the convertImmediate function above has been inlined here, so it can be removed.
|
|
||
| result.contentKind = std::move(*gltfResult.model); | ||
| return asyncSystem | ||
| .runInWorkerThread([result = std::move(result), |
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.
postProcessContentInWorkerThread is already running in a worker thread, so this runInWorkerThread is unnecessary.
CesiumGltfReader/src/GltfReader.cpp
Outdated
| CesiumAsync::HttpHeaders httpHeaders; | ||
| httpHeaders.insert(headers.begin(), headers.end()); |
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.
| CesiumAsync::HttpHeaders httpHeaders; | |
| httpHeaders.insert(headers.begin(), headers.end()); | |
| CesiumAsync::HttpHeaders httpHeaders(headers.begin(), headers.end()); |
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 @kring for fixing my broken code!
|
I've addressed all of my comments above, but there's a more subtle problem remaining. Previously, |
|
Thanks @timoore! |
An new method
CesiumGltfReader::readGltfAndExternalDatahas been added which reads external data and then does postprocessing.Addresses #1034