Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent Uncaught Exception from ThinEngine.runRenderLoop when window.SetTimeout in not defined #13401

Merged
merged 5 commits into from
Jan 5, 2023

Conversation

barroij
Copy link
Contributor

@barroij barroij commented Jan 4, 2023

I am having an uncaught exception when using BaylonJs in a NodeJs environment because of a call to window.setTimeout.

It is not very clear in the code what is the standard, as sometime we call the global setTimeout and sometime we call window.setTimeout.

My take on this is that it should be ok to always call the globals setTimeout/ clearTimeout as they are bound to be defined and are the very same functions as window.setTimeout/ window.clearTimeout in most cases.

Though this PR do not change them all. I just make some 2 files more consistent where we used to use a mix of both, and I updated the ThinEngine.QueueNewFrame to NOT call window.setTimeout, but to fall back on the global setTimeout.

To really be "environment independant", we should never call a function that is "environment dependant" directly, and put them all behind a configurable callback registry. This registry would be initialized with default values, and could be customizes by the user depending on its needs. But I'm not sure how it is doable, and anyway that's another story :)

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I am not sure why you opted in to use window.clearTimeout instead of removing all window references in one place, but otherwise - this is a great change.

@RaananW
Copy link
Member

RaananW commented Jan 4, 2023

And another possible change would be to use ReturnType<setTimeout> instead of casting to unknown and then to number, but that might collide with our internal type definitions in the engine. If that the case - leave it this way

@barroij
Copy link
Contributor Author

barroij commented Jan 4, 2023

And another possible change would be to use ReturnType<setTimeout> instead of casting to unknown and then to number, but that might collide with our internal type definitions in the engine. If that the case - leave it this way

Well the difficulty here is that actually setTimeout is used as a fallback in case requestAnimationFrame/requestPostAnimationFrame is not defined. So the return type of QueueNewFrame should the same as requestAnimationFrame, requestPostAnimationFrame and setTimeout...

@RaananW
Copy link
Member

RaananW commented Jan 4, 2023

let's leave it this way then.

@sebavan
Copy link
Member

sebavan commented Jan 4, 2023

@bghgary could you check it would be ok with Native ???

@sebavan
Copy link
Member

sebavan commented Jan 5, 2023

@barroij would be great if you can fix lint and format ?

@barroij barroij closed this Jan 5, 2023
@barroij barroij deleted the window_timeout branch January 5, 2023 14:56
@barroij barroij restored the window_timeout branch January 5, 2023 15:00
@barroij barroij reopened this Jan 5, 2023
@barroij
Copy link
Contributor Author

barroij commented Jan 5, 2023

@sebavan formatting done !

@RaananW RaananW enabled auto-merge (squash) January 5, 2023 15:53
@RaananW RaananW disabled auto-merge January 5, 2023 15:53
@RaananW
Copy link
Member

RaananW commented Jan 5, 2023

I'll wait for @bghgary before merging just to be sure.

@bghgary
Copy link
Contributor

bghgary commented Jan 5, 2023

Weird, I thought I replied, but maybe I forgot to actually send. :)

Anyways, in native, we put the setTimeout/clearTimeout functions on both window and global, so it should work either way.

@sebavan sebavan merged commit e6e89b5 into BabylonJS:master Jan 5, 2023
@barroij barroij deleted the window_timeout branch January 6, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants