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

Make SharedAsset a base class, eliminate AssetContainer #960

Merged
merged 7 commits into from
Oct 4, 2024

Conversation

kring
Copy link
Member

@kring kring commented Oct 4, 2024

This is a PR into #959.

There's a corresponding cesium-unreal branch that goes with this:
https://github.com/CesiumGS/cesium-unreal/tree/shared-assets-image-cesium-only

@azrogers this is the refactoring we talked about (mentioned here). Once again it isn't completely done but is working well enough that you can build on it, if it seems like a reasonable direction to you. I renamed SingleAssetDepot to SharedAssetDepot, and AssetDepot to SharedAssetSystem. Feel free to merge into your branch when you're ready, or wait a bit on that if you prefer.

Some things I'm still concerned about (but don't know the best way to solve yet)...

Asset Factory

Currently, a Factory class is passed to getOrFetch. This is conceptually kind of broken. I mean, if I call that function twice with the same URI but a different factory, do I get the same resource both times?

Maybe it's a little bit academic, but if so, why not construct the SharedAssetDepot with the Factory instead, and save the trouble of passing it through each time? Well, we immediately run into the problem that we can't construct a factory for images without knowing the KTX options, which can technically be different on different tilesets (though it's not likely).

Intrusive reference counting feels error-prone

It's disastrous to create an ImageCesium instance on the stack or as a member of another class and then refer to it with an IntrusivePointer. If we do, after the last IntrusivePointer goes away, the instanced will be deleted, which will go very poorly.

This isn't really a new problem, but I think it became obvious to me while working on this. ImageCesium used to be a class that was usually instantiated on the stack or as a member of some larger object. But now it is reference counted and therefore must be heap allocated (i.e., allocated with new). There's a fair bit of possibility for error in the conversion.

I'm not sure yet how much to worry about this, or what we can do about it. One possibility is to prevent allocation anywhere other than the heap by making the constructor private and providing a factory function.

Random aside while I'm thinking about it: ImageCesium is a strange name. Perhaps ImageAsset would be more appropriate now.

Base automatically changed from shared-assets-kring to shared-assets October 4, 2024 14:39
@azrogers azrogers merged commit aa81cbd into shared-assets Oct 4, 2024
24 checks passed
@azrogers azrogers deleted the shared-assets-image-cesium-only branch October 4, 2024 17:28
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