Added selective ContentManager asset unloading #6663
Conversation
|
-What would happen if I unload a Model ? -What would happen if I continuously unload and reload the same Model? (the same applies to any custom assets that isn't disposable but contains native resources, not just Models) |
|
-I'm not the most knowledgeable about models, but I just went through the code and it doesn't look to unload any of those. The currently existing -It would keep creating new Effects, etc. and likely cause memory leaks, should the user fail to manually free up the additional resources the model uses. -If custom assets contain native resources, they should be implementing |
No, the content manager used to load the model will contain all disposable resources the model requires like effects and textures, so they're disposed when the content manager is unloaded.
They should after this change, but just like Model, assets that can only be created through the content pipeline could depend on the unloading behavior instead of implementing IDisposable. |
|
I think that unloading models should only unload the vertex buffer to give the control to the developer to handle the effect/texture on their own if need be (thinking about shared effect/texture). Which is what this PR does. |
|
What modifications need to be made to get this merged? |
|
As far as I understand the C# language, I don't think this code is safe to use. While it will remove the item from memory, it does not remove any pointers to it, which could cause null point exceptions. This is because the way the content manager works, any time a part of the code wants to load the asset, (say for example, grass in a game, an object containing location and texture) it checks to see if it already has the asset. If yes, it just returns the pointer to the same asset. (pointer, pass by reference, asset is a generic, generics are classes, classes are only ever handled by reference). So say, I have lots of grass objects, and I go into a house, so I unload a scene, we have to make sure at no point does the grass object act on or off of the texture after it gets loaded, every object owning a pointer to the asset must remove that connection or the parent object must be deleted (disconnected and forced garbage collected, maybe even defragged by a second forced collection), or by some other trick make sure the texture is not used ever again, so that you can then unload the asset safely knowing that no other objects have fantom memories of the asset. Generally, the idea is not idiot-proof, that doesn't make the code bad, but it does make it an issue. Basically, you have to be smart and safe with the unloading. A typical workaround that is kinda safe, it seems many users make a unique content manager for each unique scene, which they tell to unload or rather, dispose, at the end of the scene. But that still requires users to design scene so that all the objects are destroyed as well. The only issue with this, however, is now you have to reload all assets, none carry over. A bit more load time at the risk of memory conservation. though I would also garbage-collect and defragment at this time as well. |
|
Not sure if this has been on anyone’s mind lately, but I’m also curious about what this needs or what decisions need to be made to be able to merge.
I don’t think this should be a concern, because unloading the entire ContentManager has the same problem. It’s up to the developer to manage either way. |
|
It's up to the developer to manage the lifecycle of objects and the content manager is no substitute to this. I believe that this PR is as good as it can get, though it would be nice to update the documentation to highlight the fact that unloading a |
What are the options for manually unloading these? I haven't looked through how associated content is loaded, but would it be possible to unload these with an additional call to |
| /// Unloads a set of assets. | ||
| /// </summary> | ||
| /// <param name="assetNames">The names of the assets to unload.</param> | ||
| public virtual void UnloadAssets(IList<string> assetNames) |
There was a problem hiding this comment.
For usability, can this be params string[] assetNames instead?
| /// Unloads a single asset. | ||
| /// </summary> | ||
| /// <param name="assetName">The name of the asset to unload. This cannot be null.</param> | ||
| public virtual void UnloadAsset(string assetName) |
There was a problem hiding this comment.
Does overloading Unload make more sense than UnloadAsset, since the load method is Load, not LoadAsset?
|
It depends on the setup, but basically yes. Either resources are internal to a model (and are therefore freed with the model) or they are external (and are not freed but can be unloaded through the content pipeline by naming them manually). |
| if (disposable != null) | ||
| { | ||
| disposable.Dispose(); | ||
| disposableAssets.Remove(disposable); |
There was a problem hiding this comment.
I'm not sure why disposableAssets is a list, since it seems like it's only used as a set. Should it be changed to a set for perf here?
|
@Jjagg @harry-cpp May I request a review to move forward? It's good to go to me, and could be improved later on demand. |
|
I'm fine with merging this as is. |
This PR adds two new methods to ContentManager:
UnloadAsset- Unloads a single asset and disposes it if it's disposableUnloadAssets- Takes in anIList<string>of asset names to unload and disposes the associated assets if they're disposableRelevant issue: #6164