Skip to content
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

Refactor texture creation to avoid the need for a global lock #1527

Merged
merged 29 commits into from
Oct 10, 2024

Conversation

kring
Copy link
Member

@kring kring commented Sep 30, 2024

This is a PR into #1521. Still to do:

  • Metadata textures are most likely thoroughly broken at the moment.
  • Lots of cleanup and doc updates are still necessary.

Previously, creating textures in Cesium for Unreal was a two stage process:

  1. Do some work in a worker thread to turn the glTF Texture/Sampler/Image/ImageCesium into a LoadedTextureResult and Unreal FTextureResource. This was synchronous.
  2. Do some work in the main thread to turn the above into an Unreal UTexture2D. This was also synchronous.

Now it's a three step process:

  1. Create an FTextureResource from an ImageCesium, which may or may not be shared among multiple models/tiles. This happens in a worker thread but completes asynchronously (a Future resolves when it is done) because only the first worker thread that needs a shared image will process it, while the others wait (via Future) for the first.
  2. Do some work in a worker thread to turn the above FTextureResource plus the glTF Texture/Sampler/Image/ImageCesium into a new FTextureResource (pointing to the same RHI texture) and a LoadedTextureResult. This part is synchronous.
  3. Do some work in the main thread to turn the above into an Unreal UTexture2D. This part is also synchronous.

@azrogers
Copy link
Contributor

azrogers commented Oct 8, 2024

Implemented the change to properly report texture stats. However, this led me to the discovery that, while textures are correctly being deduplicated, and are only loaded once per URL per tileset, textures aren't getting cleaned up after the tileset unloads. I believe this is because the SharedAsset holds an IntrusivePointer to the SharedAssetDepot, while the SharedAssetDepot holds an IntrusivePointer to the SharedAsset. This means that, even if every mesh that uses a texture unloads, the reference remaining in the SharedAssetDepot's asset map will prevent the deletion logic from running. Therefore, the only way for textures to actually unload is for the SharedAssetDepot to destruct, which can't happen because the SharedAssets still reference the depot.

I believe what needs to happen is:

  • The SharedAssetDepot needs to hold a weak pointer of some sort in the asset map instead of an IntrusivePointer, so that it doesn't count towards the reference count of the asset.
  • Similarly, the IntrusivePointer to the SharedAssetDepot in SharedAsset needs to be changed. I don't believe there would be a circumstance where you'd want that pointer to still be valid after the SharedAssetDepot has been cleaned up, since if a tileset is unloaded, we won't be using any of its previously loaded assets anyways.

Does this sound right to you, @kring?

@kring
Copy link
Member Author

kring commented Oct 9, 2024

Good eye @azrogers! I just opened a PR (CesiumGS/cesium-native#964) to address that.

Update cesium-unreal to work with shared-assets-tweaks branch of cesium-native
@azrogers azrogers merged commit 62bdd81 into shared-assets Oct 10, 2024
12 of 14 checks passed
@azrogers azrogers deleted the shared-assets-kring branch October 10, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants