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
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 The ?
means the parameter is optional and the JSDoc says “(default: true)”. So: Optional with default behaviour to true
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 thebabylon.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.
@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
- Observable: Do not include deleted observers in hasObservers result by Popov72 · Pull Request #13283 · BabylonJS/Babylon.js · GitHub
- Observable: Do not include deleted observers in hasObservers result by Popov72 · Pull Request #13283 · BabylonJS/Babylon.js · GitHub
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
So… looks like there is a bug with your PR @Evgeni_Popov could you check please?
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 ?
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:
- register an observable that has unregisterOnNextCall set to true.
- call Observable._deferUnregister so that _numObserversMarkedAsDeleted is set to 1
- 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
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.
- https://playground.babylonjs.com/#9K3MRA#1132 - note that the 2nd log message never gets logged out
Here is a working version - I realised that by using add, rather than addOnce would avoid the issue:
Oh, i had assumed it was intentional to not fire scene.onReadyObservable after the scene is ready anymore.
Amazing !!! sorry to be late to the party but so glad this one has been fixed !!!