LinesMesh disposes externally provided material

Bugs
1

This post is related to LinesMesh no longer disposes shaderMaterial by default — causes memory leak in BoundingBoxGizmo as well as the resulting PR.

While the linked post/PR actually revert the behavior to fix a memory leak (which is good), I stumbled upon an issue in the original implementation.

If you actually supply an external material to the LinesMesh using the constructor, it will by default dispose of the material when disposing the LinesMesh. I would argue that supplying a material from outside means you may want to reuse the material elsewhere and handle ownership / disposal yourself.

This is complicated by the fact that the disposal can be disabled when actually calling dispose on the LinesMesh using the parameter doNotDisposeMaterial. However you won’t be able to specify this parameter when the mesh is retrieved through other mechanisms that give you the object as a Mesh or AbstractMesh. It is also confusing that dispose already has a disposeMaterialAndTextures parameter (which is false by default!) but the linesmesh disposal provides an additional parameter called doNotDisposeMaterial which works the opposite way. So what if you don’t specify either? The defaults would say:
disposeMaterialAndTextures = false → Do not dispose the material
doNotDisposeMaterial = false → Do dispose the material

I was thinking about opening a PR as suggested by @sebavan but I know far too little about the implications of making a change in this area.

I would however suggest two changes to fix / improve this behavior:

  1. Remember if material has been externally supplied and act accordingly.
    It is possible to detect whether dispose has been called with an explicit value for doNotDisposeMaterial or if it is simply undefined. If undefined, only dispose self-created materials. If an explicit value has been given, use this to decide.
  2. Optional: Remove the third parameter and just consider disposeMaterialAndTextures. In this case, the LinesMesh should still always dispose of self-created materials to prevent memory leaks but you can opt-in to dispose of any externally supplied materials as well by setting this parameter to true.

Here is a playground to get a better understanding of our issue:

We currently create a few line systems which share the same external material and give them a tag. Once its time to remove all the line systems we retrieve them by tag and dispose them. It is not possible to keep the material alive unless you cast back to the LinesMesh and specify the appropriate parameter.

2

I like point 1. and about 2. it would be a breaking change unfortunately. I would advise to still keep the 3rd param as optional and it would take precedence over the second if defined for back compat purpose ?

Do you want to make a PR with that ?

If not I ll do it.

3

Since I don’t have the Babylon repo set up at all on my machine it would be nice if you can make the PR. I’m ok with any solution as long as an externally provided material no longer gets disposed without explicitly asking for it.

1 Like
4

I ll do today

6

This should help Fix line mesh material dispose by sebavan · Pull Request #17075 · BabylonJS/Babylon.js · GitHub

8

Thanks a lot.
I noticed that my original playground already seems to work now (and doesn’t if I switch back to v7). So I am confident it will work in the real world. But is the playground always using the latest commit from master? It shows it’s running on 8.25.0 even though that doesn’t include the commit as far as I can tell.

Anyways, I’ll notify you if this also helped in our software (may take some time).