-
Notifications
You must be signed in to change notification settings - Fork 297
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
Cache images across glTFs to avoid duplication #1521
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.
@azrogers I have some specific comments below, but I haven't looked at the code exhaustively.
More generally, I think the chains of futures here have gotten really complicated, and I'm not sure it's justified. From what I can tell, the only reason that any of the CesiumGltfComponent code has been futurified at all is because of the need to generate mipmaps, and the desire to avoid blocking worker threads (via waiting on a mutex) while some other thread generates mipmaps for a shared image.
Do I have that right? If so, I think a little restructuring will go a long way.
Currently, the mipmap generation starts right at the beginning of the process in loadModelAnyThreadPart
. We get a SharedFuture for its completion. Unfortunately that runInWorkerThread
continuation in preprocessImage
is going to be run synchronously, because we're already in a worker thread (and AsyncSystem knows it), and so the mutex is going to be held for the entire duration of the mipmap generation. This will not only block all threads that need this same image, it will also block all glTF loading period (even across tilesets) due to the use of the global mutex. Ok, so that has to be fixed, or else all this complexity is pointless.
But let's say we fix that, without getting into the details of how we would do it....
Next we start the main part of the glTF loading process. Way down in the depths of creating of Unreal components from glTF node -> mesh -> primitive objects, we need to create some textures. And creating those textures requires first waiting on that previously-started mipmap generation. Which requires that process to be async (i.e., return a Future), and that requirement bubbles its way through everything.
I think we can drastically simplify this, with very little tradeoff, by simply moving the mipmap continuation earlier. loadModelAnyThreadPart
would look something like this:
static CesiumAsync::Future<TUniquePtr<UCesiumGltfComponent::HalfConstructed>>
loadModelAnyThreadPart(
const CesiumAsync::AsyncSystem& asyncSystem,
const glm::dmat4x4& transform,
const CreateModelOptions& options,
const CesiumGeospatial::Ellipsoid& ellipsoid) {
return createMipMapsForAllTextures(...).thenInWorkerThread([...]() {
doTheRestOfTheNormalProcessSynchronously(...);
});
}
In other words, we wait (via continuation, not actually waiting) for mipmaps to be available for all of our images before we do the rest of the process, and we don't need Futures in any of the rest of the process at all.
Now, you might legitimately point out that this is a little less efficient. If we have multiple tiles sharing an image, we conceptually could have one thread generating mipmaps for that image while the other threads do the rest of the model loading process in parallel, and the design I proposed would preclude this. But in this scenario, we're no less efficient than we would be if each tile had its own image. And also, if we're worried about this inefficiency, shouldn't we be more worried about the same inefficiency while actually downloading the image? We could conceptually start creating Unreal meshes while downloading the shared images, too, not just while generating mipmaps for them.
Considering how much complexity it eliminates to ignore this small inefficiency, I think it's a really good tradeoff. Especially remembering that all those extra Futures flying around have a runtime cost, too.
Now, circling back, the trick to implement createMipMapsForAllTextures
is to use a Promise
. Roughly:
- Lock the mutex
- Get/add the extension. If it already exists, return its Future and we're done. Otherwise,
- Create a Promise.
- Get the Promise's Future and call
share
to make a SharedFuture from it - Assign the SharedFuture to the field on the extension
- Unlock the mutex
- Generate the mipmaps in the current thread.
- Call
resolve
on the Promise.
This is the basic process needed to fix the problems in the current design as well.
Update cesium-unreal for shared asset changes
A few things before we can merge this, in order from rather important to mostly trivial. 😁
|
The memory leak and macOS build problems are fixed. @azrogers can you please give this a self review today, and tick off the other two issues (plus test failures if there still are any), and we should be in good shape to merge it for the release tomorrow. |
This implements the changes from CesiumGS/cesium-native#926 in Unreal. Now, when the same image is used by multiple glTFs, the texture data is only uploaded to the GPU once. Subsequent glTFs using the same image will create a new reference to the existing texture resource, reducing memory consumption.