Scene ready observable not firing with v5.35.0

We definitely need a repro for this one as nothing obvious between both releases could lead to your observations:-(

I may had seen the same problem with v5.34.0 but I cannot reproduce it.

Our project is way to complicated, I’m not sure to be able to create a reproduction repo :disappointed:

Just to note, I think isReady() takes arguments. You are defaulting. Just taking a wild stab at this.

Yeah right! Nice though.

But the checkRenderTargets argument already is true by default

/**
* This function will check if the scene can be rendered (textures are loaded, shaders are compiled)
* Delay loaded resources are not taking in account
* @param checkRenderTargets true to also check that the meshes rendered as part of a render target are ready (default: true)
* @returns true if all required resources are ready
*/
isReady(checkRenderTargets?: boolean): boolean;

I am sensing that you do not write Typescript. The ? means it is optional, which is undefined by default, I think. For a boolean, that is not a true.

To have it default to true, it would need to be:

isReady(checkRenderTargets = true) { ... }

Actually that’s how the method is declared in Scene:

/**
 * This function will check if the scene can be rendered (textures are loaded, shaders are compiled)
 * Delay loaded resources are not taking in account
 * @param checkRenderTargets true to also check that the meshes rendered as part of a render target are ready (default: true)
 * @returns true if all required resources are ready
 */
public isReady(checkRenderTargets = true): boolean {

I’m writing TypeScript a lot :wink: The ? means the parameter is optional and the JSDoc says “(default: true)”. So: Optional with default behaviour to true :wink:

Yes, that first sentence you quoted was an error on my part & not necessary, sorry. I am not sure how far to take this, since nothing that follows will resolve the original issue. You did get me checking though, & this is what I end up with:

  • You must have got the syntax that you posted of isReady from the babylon.d.ts, where even if you explicitly assigned a default in the source, it just shows a ?: type. The source file (where I almost always look) is forcing the default to be true.

  • The ? is used by the Typescript transpiler to check that the minimum of arguments to were used when a function is called, but does not end up in the final js file.

  • Turns out that all arguments are optional in actual JavaScript. This doc, says

In JavaScript, function parameters default to undefined.


Back to the original issue. If boiling the problem down to a nice neat pg is not viable, starting with a working pg, and adding stuff to it, in an attempt to get it to show the problem, can sometimes work. You can “shop” for playgrounds to use to start using searching.

Just checking what happens in the simplest case is useful in itself to know.

1 Like

@sebavan I’ve checked the diff between the 2 versions: Comparing 5.34.0...5.35.0 · BabylonJS/Babylon.js · GitHub

And it looks like the root observable class has been modified with: Observable: Do not include deleted observers in hasObservers result

I’ve just updated to v5.35.1 to reproduce the bug… but when I go to node_modules and manually comment this:

    /**
     * Gets a boolean indicating if the observable has at least one observer
     * @returns true is the Observable has at least one Observer registered
     */
    hasObservers() {
        return this._observers.length/* - this._numObserversMarkedAsDeleted*/ > 0;
    }

Everything is working fine :wink:

So… looks like there is a bug with your PR @Evgeni_Popov could you check please? :pray:

Edit: Related to Observable.hasObservers() is still true after removing all the observers - #4 by sharp

@yvele this is actually a bug fix as we were before accounting for deleted observers.

Would be really good if you could provide a repro ? as it is almost impossible to fix without it ?

1 Like

not sure what doesn’t work in this case? Have you previously shared code?

There was a long standing issue that was fixed (see previous messages), so it might require a slight change in your code to work correctly.

we hate it when there is something that doesn’t work or if there is a bug in the framework. But we can only fix it if we can reproduce this.

If you can provide steps to reproduce or a project where it doesn’t work, we will be happy to dive in and find the issue. But if it is working in the playground and doesn’t work in your project there might be something in the project setup that prevents it from working correctly. Not saying it is the project’s fault, just saying that in the case of this project something wrong is happening.

So - we will be very happy to see a reproduction, otherwise it is very hard for us to help. We will anyhow keep that in mind in case it pops up in the future.

Hi @RaananW - I have also seen this issue in my code and have done some digging.

It looks like the issue is caused by:

  1. register an observable that has unregisterOnNextCall set to true.
  2. call Observable._deferUnregister so that _numObserversMarkedAsDeleted is set to 1
  3. call Observable.clear() before Observable._remove() is called. This step will leave _numObserversMarkedAsDeleted as 1, breaking future calls to Observable.hasObservers().

I was able to resolve the issue by updating Observable.clear() to set _numObserversMarkedAsDeleted to 0.

In addition:

  • calling either Observable.makeObserverTopPriority() or Observable.makeObserverBottomPriority() will call Observable._remove without first incrementing _numObserversMarkedAsDeleted, so _numObserversMarkedAsDeleted will become negative.

I haven’t yet been able to create a playground for the above, but I thought it was worth highlighting.

UPDATE:
it looks like scene._checkIsReady() will cause the above issue by calling:

this.onReadyObservable.notifyObservers(this);
this.onReadyObservable.clear();

I can only reproduce this issue in PG when adding a delay to scene.onReadyObservable.add /addOnce
Without a full repro, it’s diffucult to say exactly for your case, but i’ll play along and make guesses.

I would almost bet that the issue occurs due to async/await, and has nothing to do with renderTargets other than the time they take to load/compile.

It’s likely that

scene.isReady()

turns true after your code check, but around the same time your async function (promise) is being created, and some of the times, before your onReadyObservable is registered, this is why it works sometimes.
Or maybe this is all just side-effects :slight_smile:

scene.onReadyObservable

is not no longer fired (unless forced to recheck) once the scene is already ready,
you can use

scene.executeWhenReady( function(){} )

which forces a recheck and works with any delay.

There’s also

scene.whenReadyAsync() // returns a promise

Which i suppose is the function ment to be used in async/await

@Evgeni_Popov - here is a playground, you will need to open the debugger to see the issue.

Here is a working version - I realised that by using add, rather than addOnce would avoid the issue:

1 Like

Oh, i had assumed it was intentional to not fire scene.onReadyObservable after the scene is ready anymore. :sweat_smile:

Thanks @Charge for actually finding the problem and the repro!

Here’s the PR:

5 Likes

Amazing !!! sorry to be late to the party but so glad this one has been fixed !!!