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

Cache images across glTFs to avoid duplication #926

Merged
merged 100 commits into from
Oct 30, 2024
Merged

Cache images across glTFs to avoid duplication #926

merged 100 commits into from
Oct 30, 2024

Conversation

azrogers
Copy link
Contributor

As described in #497, sometimes tilesets contain multiple tiles that point to the same image resources. At the moment, these images are loaded once for every tile that uses them, meaning a tileset with 1,000 tiles that use the same image will load that image 1,000 times (and the runtimes will allocate video memory for each of them). This change adds a SharedAssetDepot class that stores images across tiles, and a SharedAsset smart pointer type for tracking and cleaning up the images.

SharedAssetDepot is set up so that in the future, if we need, we can extend this feature to handle glTF buffers as well.

There's a few TODO items currently (besides the runtime implementations):

  • Every copy of the image is still counted towards memory used, making the metric inaccurate.
  • Currently, the SharedAsset contains either a pointer to a stored asset and counter, or it contains the asset itself if no SharedAssetDepot was provided. This means that the "smart pointer" is at least 96 bytes (the size of ImageCesium), even if it does contain a pointer, which is unacceptable. I'm not sure what to do about this, short of just requiring the use of a SharedAssetDepot - but this would increase the number of necessary code changes to other projects that use CesiumGltf besides the native runtimes.
  • The way I've laid out namespaces and usings in SharedAssetDepot.h is almost certainly not compatible with the style guide (I started it before the meeting) so I need to fix that.

@lilleyse
Copy link
Contributor

@azrogers do you think it will be straightforward to extend this approach to also support external schemas (#727) in a follow-up PR?

@azrogers
Copy link
Contributor Author

@lilleyse I've designed the SharedAssetDepot to support multiple kinds of assets, though only images are implemented in this PR. It should support extending it to also handle loading external schemas.

Copy link
Member

@kring kring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some first comments for you @azrogers. I mostly focused on SharedAssetDepot.h because I think that's the meat of the design, so this isn't a complete review yet. It's looking good! Just a few comments to tighten up the design a bit. In general, I think it would be valuable to really focus on how different things are passed and stored. I think there are some cases where things are copied unnecessarily, and for an ImageCesium that's a very expensive problem. I tried to flag some of these issues, but didn't look exhaustively. If you haven't read it before, Effective Modern C++ has lots of good info about how to decide between r-value references, pass-by-value, etc. (even though it only covers up to C++14).

CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
CesiumGltf/include/CesiumGltf/SharedAssetDepot.h Outdated Show resolved Hide resolved
@azrogers azrogers marked this pull request as ready for review September 11, 2024 20:14
@azrogers
Copy link
Contributor Author

@kring Reviewed your changes to cesium-native and cesium-unreal. Both look good, only found a minor typo to fix (which I did myself). Looks like getting all the CI checks to pass is the next step here - anything else remaining before we can get this in?

@kring
Copy link
Member

kring commented Oct 29, 2024

Thanks @azrogers, I plan to merge this today!

@kring
Copy link
Member

kring commented Oct 30, 2024

This looks great, merging!

@kring kring merged commit 586ad64 into main Oct 30, 2024
24 checks passed
@kring kring deleted the shared-assets branch October 30, 2024 03:34
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.

5 participants