Mesh isReady can get stuck during screenshots

Bugs
1

This one will be almost impossible to provide in a PG because it seems to be random. On this line, the mesh will return false every time if a material had a texture that has now been disposed and the mesh is still enabled.

We can put a break point here and observe that the mesh never resolves as it appears to point to the old texture name and it may not exist anymore. If we add a Promise.race around the Screenshot code it can recover, but this seems like a bug somewhere. Sorry if that’s not much to go on, but maybe it rings a bell?

2

It unfortunately does not ring a bell :frowning: maybe @Evgeni_Popov ?

I am not too sure to understand this part ? could you detail it a bit more ?

3

Sure, sorry if that wasn’t clear. When you request a screenshot, the code loops through each active mesh and checks to see if it’s “ready” before it starts pulling pixels. There is a way to get this in a broken state in which a mesh will never be ready, so the screenshot code will never return. If you step through the code, it’s because the mesh has a material and the material has a texture that is never ready, so it just hangs. I think this is caused by adding/removing textures on a material, so we’ve solved it by doing mesh.setEnabled(false) where we can, but there seem to be some edge cases sneaking in. Does that clarify?

4

Could you check what element is not ready to let us know ?

5

It’s the texture on a material, which is listed in the code above as the effectiveMaterial and the method isReadyForSubMesh always returns false. This is usually when a mesh has been enabled or disabled and the material disposed. I’m wondering if it somehow uses the string name of the material as a way to cache somehow, so it skips a step somewhere?

Sorry, these are all just guesses.

6

Do you mean you disposed of a material but a mesh is still using this material?

7

I don’t know the exact sequence here because we’re still tracking it down, but it’s something like the following:

  1. A mesh gets assigned a material and a texture
  2. That mesh is no longer needed, so it is disabled and the texture is disposed (material should stay)
  3. The mesh is needed again, but perhaps is only enabled and not assigned a new texture
  4. The material is still pointing to the old texture even though it no longer exists
  5. The texture appears to be a valid JavaScript object but the texture never shows as “ready”

Hard to isolate the exact steps here.

8

It is not valid to have a material points to a texture that has been disposed. You should reset the material property corresponding to this texture to null.

9

Interesting! I wonder if our edge case is caused because 99.9% of the time the material is assigned a new texture but in some rare circumstances it’s omitted, so it breaks. I’ve updated the places we dispose textures and now I can’t repro locally.

So, does this mean that when you dispose a material with material.dispose(true, true) that the textures will also persist, so it’s also necessary to do…?

material.dispose(true, true);
material = null;
10

Yes, using a disposed material for a mesh is a bug. If you’ve disposed a material, you must set mesh.material = null (or set another valid material) for all meshes that used this material.

If you dispose only a texture (say, the texture used for diffuse), you can keep the material but set material.diffuseTexture = null (or another valid texture).

11

Ok, that is definitely something I did not know. Thank you!

So, just to help me understand the approach here, is there not a good reason to make the dispose function handle this and set itself to null? Is there a use case in which you dispose a material but want the mesh to still have an attached material object? I can create a wrapper function in my code to standardize this but I wonder if that’s something better done in Babylon?

Just thinking out loud…

EDIT:
It appears that this is happening in the material.dispose function. This part here attempts to null out the material.

However, the baseTexture does not appear to do the same cleanup:

12

Indeed.

For the texture, you will have to null out the material property yourself, as it’s not possible to do it automatically in a generic way.

13

I wonder if there it a simple way to automate the process on our end ? without detrimental perf impact ?

14

We’re doing something similar, albeit for a different reason (we didn’t realize it would affect the screenshot functions). If you loop through the scene.materials array, you can do material.hasTexture(tex) and null it there.

15

Another approach might be storing just the material IDs, and texture id in the texture class, then doing scene.getMaterialById(id) and handling it that way.

16

What you want to do is material.the_property_pointing_to_the_texture = null. The problem is to know what the_property_pointing_to_the_texture is. The only way we could do it in an automatic way would be to scan all the properties of all scene.materials and set them to null when they point to the texture… I don’t know what it means for performance, though. cc @sebavan

17

This is the operation we do in a separate context and it’s reasonably fast, but I wouldn’t say for sure it’s the right solve. Another approach would be storing simple strings and a helper method to convert the string to the prop. For example, if the texture stores {matId: someId: prop: TextureProps.AlbedoTexture} you could deduce the context to do this more cheaply and get to the same result. And if those are a hash map of materials, you could handle multiple material pointers of differing types using the same texture. it would be pretty efficient to find and destroy the objects.

One advantage of this approach is that there is no chance the material becomes a retainer for the texture or vice versa.