CesiumGS / cesium-unity

Bringing the 3D geospatial ecosystem to Unity
https://cesium.com/platform/cesium-for-unity/
Apache License 2.0
358 stars 83 forks source link

Fix UnityPrepareRendererResources trying to destroy texture assets #402

Closed azrogers closed 9 months ago

azrogers commented 9 months ago

Fixes #344.

Currently, when UnityPrepareRendererResources frees a tile, it calls DestroyImmediate on the material and all of its textures. This is fine and in fact desired behavior for the materials and textures Cesium creates at runtime - we need to make sure these texture resources get cleaned up so we don't leak memory. This is also fine for user-provided materials, as we create a new instance of the material - as long as the material doesn't have any texture assets set for its texture properties. Textures aren't copied when you copy a material, so when freePrimitiveGameObject goes through all of the material's texture properties and calls Destroy on everything it finds, it ends up attempting to destroy the assets the end user specified on the material. This produces a "Destroying assets is not permitted to avoid data loss" error.

There are two ways we can solve this. The first way would be to make a new instance of all the textures the same way we make a new instance of the materials. However, it would be simpler to just keep track of all the textures we're creating at runtime, and just not try to destroy textures we didn't create. There's a few ways to do this, but I went with the simplest - setting the UnityEngine::HideFlags::HideAndDontSave hideFlag on each texture created, and checking it before destroying. This prevents assets from being destroyed.

One caveat is that if the user sets a texture they created on the material that also has UnityEngine::HideFlags::HideAndDontSave set, our code will attempt to destroy it too. This won't result in errors, just undesirable behavior. However, I think this possibility is pretty slim, and in the case where it's an issue we can suggest that they simply use UnityEngine::HideFlags::DontSave instead.

azrogers commented 9 months ago

@kring Merged with main and addressed your comment!

kring commented 9 months ago

Thanks @azrogers! Lots of people will be happy to see this one fixed!