Skip to content

Fix fast refresh for PG #454

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

Merged
merged 21 commits into from
Oct 7, 2022

Conversation

CedricGuillemet
Copy link
Contributor

@CedricGuillemet CedricGuillemet commented Sep 6, 2022

Before this PR: initPromise was created in resetView. But the promise was checked here

await BabylonNative.initializationPromise;
before it was created again. As it was resolved, bgfx was assumed to be available. JS continues its work until bgfx is really shutdown and we get a crash (partner experience)

fix: When asked to reset the view from the JS side, reset the Promise.

Note for fast refresh:
When a fast refresh is asked, the view is unchanged meaning UpdateView is not called. As the promise is resolved in UpdateView, rendering will stop.
To fix that, I've added a m_resetDone boolean that gets true when resetView is called and rendering is disabled. As rendering continues, this boolean is check to effectively UpdateView with the last know window configuration.

@ryantrem
Copy link
Member

ryantrem commented Sep 6, 2022

Do we really want to tear down and re-create bgfx/graphics on fast refresh? Fast refresh is meant to enable a very fast dev inner loop, and be able to quickly show incremental changes. If we re-create bgfx/graphics, is that noticeably slower and/or disruptive to the dev inner loop?

Also, regarding the UpdateView not getting called after a fast refresh... the way the code is currently written, creating a ReactNativeEngine instance must be deferred until graphics is initialized, which in turn must be deferred until there is a window. But if in the case of fast refresh we already have a window (and it is reusable), then it seems like we just need to adjust the code to have the graphics initialization be triggered by something else and see there is already a window to use. I could see doing something like:

  • Tracking an arcana task inside BabylonNative.cpp that is completed when a window is eventually provided (via UpdateView)
  • Tracking an arcana task inside BabylonNative.cpp for initializing graphics, which could be triggered by the call to retrieve initializationPromise, and would await the window task above

Then if we don't have a window yet, async graphics init will need to wait. If we already have a window, then it will just continue on and init if needed.

@CedricGuillemet
Copy link
Contributor Author

@ryantrem yes, fast refresh could be faster without bgfx re-init. It needs a separate issue/PR.

@ryantrem ryantrem linked an issue Oct 6, 2022 that may be closed by this pull request
@CedricGuillemet CedricGuillemet merged commit c0a2841 into BabylonJS:master Oct 7, 2022
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.

Get reset (e.g. bgfx::shutdown) working with fast refresh
2 participants