ExtendRealityLtd / VRTK

An example of how to use the Tilia packages to create great content with VRTK v4.
https://www.vrtk.io/
MIT License
3.69k stars 1k forks source link

Unity 2018.1.0b12: Can't create empty Texture (not allowed, needs to be subclass) #1758

Closed BlackDragonBE closed 6 years ago

BlackDragonBE commented 6 years ago

This line causes projects to fail compilation:

https://github.com/thestonefox/VRTK/blob/e43089a4dbefba55af2716d22de31458e0b615ca/Assets/VRTK/Scripts/Interactions/Highlighters/VRTK_MaterialColorSwapHighlighter.cs#L152

This can be fixed by replacing the line above with:

renderer.material.SetTexture("_MainTex", new Texture2D(0,0));

Cheers!

kristianpd commented 6 years ago

This is addressed in https://github.com/thestonefox/VRTK/pull/1730

thestonefox commented 6 years ago

@kristianpd I wonder if the proposed fix of new Texture2D(0,0) is actually better than Texture2D.whiteTexture

Then at least the new texture should be garbage collected in scope.

Thoughts?

kristianpd commented 6 years ago

I assumed Texture2D.whiteTexture would be a shared texture already in memory as opposed t created at call time. Wouldn't that texture be around for the entire run anyway? Are we assuming it created it the first time it's called then keeps it around forever? I didn't expect a leaked reference on every call.

On Fri, Apr 6, 2018, 5:20 AM The Stonefox notifications@github.com wrote:

@kristianpd https://github.com/kristianpd I wonder if the proposed fix of new Texture2D(0,0) is actually better than Texture2D.whiteTexture

Then at least the new texture should be garbage collected in scope.

Thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thestonefox/VRTK/issues/1758#issuecomment-379197919, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXEtl6MGP6iHobSgIMhPZGz0Ed0ORrcks5tlzNqgaJpZM4S-zDS .

thestonefox commented 6 years ago

I'm trying to look through the new c# reference code and struggling to see where they define Texture2D.whiteTexture

here maybe?

https://github.com/Unity-Technologies/UnityCsReference/blob/11bcfd801fccd2a52b09bb6fd636c1ddcc9f1705/Runtime/Export/Texture.bindings.cs#L56

kristianpd commented 6 years ago

I assumed that Texture2D.whiteTexture would be a 1x1 (ish) white pixel. Since it's a class attribute, I was under the impression it would be garbage collected as part of Texture2D's internals and assumed it was a reference to a single texture in memory.

If it was creating a new texture every time, wouldn't the old ones be GC'd when the new material texture is set and no longer referencing the object? If it is a shared texture then I would assume nothing is going to be cleaning it up for as long as unity is running. If it is a new instance of a texture, no longer referencing it via the material texture should allow it to be GC'd, no?

Am I missing something obvious here?

thestonefox commented 6 years ago

You may be totally right, it would just be nice to know what Unity is doing!

Have you ever seen what they do with Camera.main? it will blow your mind :p

bddckr commented 6 years ago

GC is of no interest: A texture(2D) is a unity object and as such you need to call Destroy on it.

The actual asset of the texture is unloaded with a call to that method Unity has to unload unused assets.

The recompiled source shows all the methods of interest (texture getters and constructors) are just calling into their native C++ layer, so can't see what it's doing. Docs don't tell anything either.

Quick test would be to just access .whiteTexture in an update method and keep looking at the GPU/Rendering profiler in the Editor - one of them lists the number of textures in use.

I'd expect that one to not increase, but the constructor call to create multiple textures except when you call Destroy on them.

thestonefox commented 6 years ago

Can confirm Texture2D.whiteTexture does not increase texture count whereas new Texture2D does.

Texture2D text = new Texture2D(0, 0); newtexture

vs

Texture2D text = Texture2D.whiteTexture; whitetexture

so whiteTexture is the solution imho

kristianpd commented 6 years ago

Cool, TIL. I'll update the PR