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

Fix UnityPrepareRendererResources trying to destroy texture assets #402

Merged
merged 7 commits into from
Feb 19, 2024

Conversation

azrogers
Copy link
Contributor

@azrogers azrogers commented Feb 2, 2024

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.

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.

This looks good to me. Just one comment below. Could you also please merge in main? Unfortunately you'll probably have to reapply the changes after #385 was merged. Sorry about that!

native~/Runtime/src/UnityPrepareRendererResources.cpp Outdated Show resolved Hide resolved
@azrogers
Copy link
Contributor Author

@kring Merged with main and addressed your comment!

@kring
Copy link
Member

kring commented Feb 19, 2024

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

@kring kring merged commit 6c843c7 into main Feb 19, 2024
2 of 5 checks passed
@kring kring deleted the dont-destroy-texture-assets branch February 19, 2024 05:56
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.

Using copies of the default materials results in error log spam
2 participants