Can we skip the ready check in screenshots?

Bugs
1

We have been trying to solve this but our application can still sometimes run into a situation in which a texture has been inadvertently disposed or an unused or hidden mesh has a texture value that is no longer valid. In this instance, when the screenshot code is invoked, it will never resolve.

You can see the behavior in this PG:

In an older version of Babylon, this isReady check was not implemented, so while the screenshot would result in the sphere appearing black, it would still render the scene. In the newer version of Babylon, this will break the functionality and the only way out is to add a Promise.race which is brittle and unreliable.

So, is it possible to add a flag to skip the isReady check?

2

Maybe I’m missing something here but why would you expect any other result if you dispose the texture before awaiting an asynchronous method?

Is there a compelling reason or scenario where you would need to take a screenshot after assets have been disposed? Sorry if it’s obvious!

Ed: reread your post and I see what you’re saying. I would think your best bet would be to use these screenshot issues as a flag to help you trace down and fix your inadvertently disposed resources.

Ed Ed: Reason it worked before is because of single thread nature of JS; if you were to put a no-op call to setTimeout or setImmediate in your code after the call to dispose, you will likely see things break other than just the screenshot!

3

This isn’t entirely correct. What happens is that when the screenshot code is invoked, it now loops through every active mesh, checks those materials for textures, and then checks those textures for being ready. This is a problem for us as we have to dispose of textures on a continual basis to avoid GPU memory leaks, but that means there might be an edge case in which a mesh has a material that used to have a texture, but that texture assignment now points to a disposed texture which breaks everything. This isn’t a problem caused by the event loop because Babylon is continually checking the isReady status in Mesh.ts which you can see here:

This will just continually return false as the effectiveMaterial is never ready.

I agree that at the core, this is an issue with our code and that we should not have this happening, but it would also be good to be able to override this if we need to.

4

Ah, thanks for the elaboration - makes more sense now. In lieu of directly answering your question (I can’t speak for the BJS team, ofc!) perhaps I can still help by obviating the need to have the flag in the first place?

To sum up my understanding:

The long-term (global) problem to solve is your GPU memory leaks, but you already know that :grin:

Your immediate (local) problem is that older versions of BJS ignored the is ready check and now that it isn’t, it’s breaking due to the solution you’ve put into place to help mitigate what was introduced by the underlying problem by GPU memory leaks.

The solution, to dispose of textures, causes the proximate problem of screenshots freezing the application due to an unresolved Promise waiting for the isReady that will never resolve.

—
One suggestion I can think of to try is what mentioned earlier, adding a no-op 0 duration call to setTimeout immediately after disposing of a texture. In this PG, though admittedly I’m on my tablet atm, I see the amiga texture briefly appear before turning black. The scene remains responsive.

Is this something you’ve tried?

Another option that occurred to me would be to keep a base texture like the amiga one in memory at all times and whenever you dispose a texture you replace it with the default texture. The various Observables available on Scene that I’m sure you’re familiar with already could be a potential place to do so.

Finally, have you tried putting the screenshot logic in the onAfterRenderObservable? You could the input trigger to capture a screenshot to register the observer to invoke the capture logic at what could be a more appropriate time.

HTH!

5

Actually, the problem is that the material texture properties are an object that has something called an “internal texture”. This problem arises is the material texture prop is NOT null but the internal texture is null. The only way to fix this is something like this:

 scene.getActiveMeshes().forEach(mesh => {
            if (mesh.material) {
                if(!mesh.material.albedoTexture.getInternalTexture()) { mesh.material.albedoTexture = null }            
            }
        })

Here, I am checking for the absence of that internal texture and nulling out the prop, and now you can see the screenshot completes as expected.

6

That’s super interesting!

Hmmm.

This to me has the scent for being a timing issue. I’ll have to research how and when those internal textures are synched with outward-facing properties to wind that backwards and see what falls out.

Or perhaps this is a question for @Evgeni_Popov - any thoughts Evgeni?

7

I think it’s really a bug on your side, as you said here:

=> a material should not use a texture which is disposed.

It would not be right to update the core code to account for a bug in user code, except if the updated code can also be useful in regular cases (but it wouldn’t be the case here).

Having said that, I think your workaround is ok:

What’s the problem with it?

8

If I can respectfully push back a bit on this, I do think it’s a bug in Babylon that when you dispose of a texture on a material, that material still retains a reference to the texture prop that then puts it in a broken state.

pbrMat.albedoTexture.dispose()

Should be a safe operation in terms of immediately generating a screenshot afterwards. But also, this check for isReadyForSubMesh is problematic because it drills down into the material and then its textures but will get stuck if the _texture prop is null.

Is there ever a scenario in which a texture is loaded but that prop is not immediately populated? If not, then a null check might be the right fix in that check. Failing that, texture.dispose() should still be a safe operation that doesn’t break screenshots.

On this point I agree. It’s not a huge lift to write some validation code on our end now that we understand the issue. But I’d still argue (respectfully!) that the root of the problem is in the approach Babylon takes to checking textures after disposing of them.

9

Just following a little more into the issue, there is this function in thinTexture.ts

The issue here is that this texture is:

  1. Not in delay load state (it’s not waiting for a loading event) so it doesn’t ever get to this code in texture.ts
  2. The _texture prop on this texture is forever null, so will never be ready
  3. The prop is not removed from the material on dispose

So it seems that we could change this line to:

 if (!this._texture && this.delayLoadState !== Constants.DELAYLOADSTATE_NOTLOADED) {
        return true;
 }

Or, as you add a delayLoadState property, we can also add a disposeState flag which gives us something like:

 if (!this._texture && this.disposeState === Constants.DISPOSESTATE_DISPOSED) {
        return true;
 }
10

what about :

let texture = pbrMat.albedoTexture
texture.dispose();
pbrMat.albedoTexture = null;
1 Like
11

Sorry, have to use this other account :slight_smile:

This works, but as I note above, it requires us to know which material prop this applied to. In the case in which two materials share the same texture (like the reflectanceTexture) or something else that is embedded in a GLTF, disposing a texture from one material can break a completely different material, causing the whole scene to be unable to be rendered as a screenshot. Note that this does not affect normal Babylon rendering of a scene, so you only see this when a screenshot is invoked.

The proposal above is to mark a texture object as disposed or else otherwise skip this check if the internal texture is null. As our app loads different GLTF files, this edge case keeps occurring and it’s very difficult to capture every scenario and stop it. By marking a disposed texture to skip this check, the issue could be solved.

12

One more suggestion here:

When a texture is disposed, the _texture property is marked as null, but so is the _engine property.

However, we can reasonably expect this property to be populated from the constructor:

With that in mind, this could be fixed if we just add this code to the isReady function.

if(!this._texture && !this._engine) {
  return true;
}

Somewhere in this code:

@Evgeni_Popov what do you think of that idea?

13

So all this is because of an optimization for memory where you discard the textures of a mesh. But what use is that mesh in the scene now anyway? You mention it is hidden, In what way does that mesh become usable again and get its textures back to be visible again? Why do you not remove the mesh as well?

To be honest I do also have to side with you on some aspects of all this. I mean if the renderer can handle having hidden mesh objects in the scene with disposed textures on some of its materials , then the screenshot tool should be able to do the same , right? Even if you made those mesh objects visiable , they would not freeze the renderer , they would just display incorrectly and that is that. User error. I dont think the screenshot tool should be checking the state of textures in materials at all.

14

Imagine you have a series of cars, and while we don’t want to render each car at once, the geometry buffers can hang around but the texture buffers will overwhelm the GPU memory and so must be disposed. But one of the hubcaps doesn’t get a new material assigned (was chrome, should now be plastic, but that is somehow missed in the data). In Babylon 5x, this results in the hubcap showing incorrectly (all black or whatever). In Babylon 6 this results in a hanging screenshot that never resolves and so the whole application is broken.

15

We are trying to see how to fix it by relying on the dispose observables of textures within the material textures setters… it sounds promising but no huge hope yet :slight_smile:

2 Likes
17

This is an interesting approach, and we can see if this helps in our current situation! One possible edge case with this idea is if we clone materials, then the observable needs to be copied as well.
(pseudo code)

const tex = new Tex()
const mat1 = new Mat()
mat1.tex = tex
mat1.tex.onDispose(() => mat1.tex = null)
const mat2 = mat1.clone()
tex.dispose()
console.log(mat2.tex) // will still have a prop?
1 Like
18

A better approach might be to write the registration code once as part of an observer for the relevant Scene-level observable fired whenever a new mesh, material, or texture is added. Have you looked at that yet?

19

Personally, this approach might work but adds a lot of complexity where there’s some sort of manager class that keeps everything straight. The main issue here is that setting a texture prop on a material causes the screenshots to fail if one of the textures is disposed. It seems to me that the best solve is to mark the texture object as disposed in a way that the screenshot validation check understands, and therefore happily skips waiting for the internal texture to be ready. That’s why I think either checking the ._engine or a flag set as isDisposed = true is really how I’d solve for this.

20

We hope that having observers on texture dispose would fix the problem cleanly, so we will have a try asap (but we have to prepare the v7 release for the end of the month, though).

2 Likes