Disposing material nukes other materials using the same texture

[fenomas]

Is this supposed to happen?

http://www.babylonjs-playground.com/#1VL9HD#1

It seems to be a breaking change in 2.3.

[RaananW]

Hi Fenomas,

Yep, textures are being disposed together with the material (https://github.com/BabylonJS/Babylon.js/blob/master/src/Materials/babylon.standardMaterial.ts#L902) . There is an upside and a downside for doing that. The upside - no memory leaks when disposing materials (otherwise you will have to dispose the textures yourself). Downside is - see your PG link  

We'll try finding a nice solution without breaking anything.

[jerome]

nice and easy solution : change it from "bug" section to "feature" section

[RaananW]

This could be a solution for a lot of tech giants and startups alike  

[fenomas]

1 hour ago, RaananW said:

We'll try finding a nice solution without breaking anything.

Things are broken already! 

That is, this seems like insane behavior. Disposing assets is the job of whoever is keeping track of whether they're in use - if Babylon isn't doing the latter then it clearly shouldn't be doing the former.

[Sebavan]

Hello,

This is actually a feature :-)

Basically the way internal glTexture are shared is through the cache. In order to benefit from the cache, you need to either create another texture from the same url (this won t dl the texture twice but take it from the cache):

http://www.babylonjs-playground.com/#1VL9HD#2

or you could clone the texture to benefit from the same settings without copy pasting:

http://www.babylonjs-playground.com/#1VL9HD#5

The only drawback would be if you dynamically changes texture settings later on.

[fenomas]

3 hours ago, Sebavan said:

Basically the way internal glTexture are shared is through the cache. In order to benefit from the cache, you need to either create another texture from the same url

What, so if you reuse the same texture object for multiple materials then the internal glTexture doesn't get shared? Meaning multiple internal textures will get made?

[GameMonetize]

Reusing the same texture or cloning it will share the same internal textures. Even creating a new texture with the same url will reuse the same internal texture

[fenomas]

11 minutes ago, Deltakosh said:

Reusing the same texture or cloning it will share the same internal textures. Even creating a new texture with the same url will reuse the same internal texture

That's what I assumed. So is it intentional that the engine disposes assets that are still in use? I mean I can work around it, but like I said it seems like an insane default behavior.

[GameMonetize]

You can definitely improve it by also counting reference to textures used by material.

Open to PR

[GameMonetize]

Ok this is far more complex than expected  You need to track all texture member on all materials and transform them to properties

not impossible but definitely not want I want to do right now

[fenomas]

I'm not suggesting Babylon should track all usage of each texture!

I'm suggesting that it probably shouldn't dispose things if it doesn't know whether or not they're still being used.

 

[JCPalmer]

maybe:

public dispose(forceDisposeEffect?: boolean, keepTextures?: boolean): void {
    if (!keepTextures){
        if (this.diffuseTexture) {
            this.diffuseTexture.dispose();
        }

        if (this.ambientTexture) {
            this.ambientTexture.dispose();
        }

        if (this.opacityTexture) {
            this.opacityTexture.dispose();
        }

        if (this.reflectionTexture) {
            this.reflectionTexture.dispose();
        }

        if (this.emissiveTexture) {
            this.emissiveTexture.dispose();
        }

        if (this.specularTexture) {
            this.specularTexture.dispose();
        }

        if (this.bumpTexture) {
            this.bumpTexture.dispose();
        }

        if (this.lightmapTexture) {
            this.lightmapTexture.dispose();
        }

        if (this.refractionTexture) {
            this.refractionTexture.dispose();
        }
    }

    super.dispose(forceDisposeEffect);
}

 

[GameMonetize]

Yep agree with JCP. We can update the dispose function to turn OFF automatic texture disposal

 

[GameMonetize]

Done: https://github.com/BabylonJS/Babylon.js/commit/66fe80f4718e59b8d3c6137b2f318fbd97be5e6a

[fenomas]

Welp, if you're all happy then I'm happy, but speaking to posterity I still think this is crazy default behavior. You wouldn't add a feature to make mesh.dispose() aggressively dispose the mesh's material right? Aggressively disposing the material's textures is the same sort of thing; it just blows up for fewer people (maybe automated things like Blender export don't reuse texture objects?).

I mean, either way works the same in a trivial demo. But in big projects assets aren't necessarily managed in the same place by the same logic - one module might create the texture and another the material. Usually you'd want the code that created an asset to manage when it's disposed, but 2.3 makes that no longer the default - it couples together what could be separate concerns.

 

[adam]

Wouldn't this make more sense?

material.dispose(forceDisposeEffect, forceDisposeTextures)

and it would be a non-breaking change from 2.3.

[GameMonetize]

Yes I agree

I renamed it.

@fenomas: I tend to agree but this would require a big change in all materials. But I don't get why you said 2.3 changed something?

[JCPalmer]

I think I get him. I changed parse in 2.3, so that materials of the same name only load once.  Seemed like a bug, because if you assigned a material by scene.getMaterialByName(), only the first would ever be used.  Right, fenomas?

[fenomas]

Whoa, I guess i'm being really unclear here!

DK: The change in 2.3 is shown in the PG in the first post - that disposing a material defaults to disposing its textures.

I'm not suggesting any big changes - I'm suggesting that the default should be reversed. Disposing one asset shouldn't implicitly dispose unrelated assets.

So the only change I'm suggesting is that the new "forceDisposeTextures" parameter should be false by default. (With the new name this makes even more sense - boolean parameters that default to true when omitted are just bad juju!)

JC: I don't know anything about that, but I could be overlooking something. (I don't do any serialization and I don't use any getByName methods, so I normally don't pay any attention to asset names.)

[GameMonetize]

I need to think about it...I'll get back to you

[fenomas]

No worries!

[GameMonetize]

Ok! I introduced a breaking changes then  Your point was too smart to ignore it  Materials do NOT dispose textures by default now

[fenomas]

I think that's the right approach. Thanks for considering!

[jerome]

nice resolution imho