fix(NA): black frame when activating overlays #1093
Conversation
renancaraujo
requested review from
spydon,
wolfenrain,
erickzanardo,
feroult,
japalekhin,
luanpotter and
a team
renancaraujo
force-pushed
the
renan/fix-gamewidget
branch
from
957c4d0
to
6a0ac89
Compare
| @@ -39,6 +39,7 @@ mixin Loadable { | |||
| /// implementing class, it is cached so that it can be reused when the parent | |||
| /// component/game/widget changes. | |||
| @internal | |||
| @nonVirtual | |||
There was a problem hiding this comment.
Made this to make it non-overridable even inside flame.
| @@ -138,11 +138,11 @@ class _GameWidgetState<T extends Game> extends State<GameWidget<T>> { | |||
|
|
|||
| MouseCursor? _mouseCursor; | |||
|
|
|||
| Future<void> get _loaderFuture { | |||
| late final Future<void> _loaderFuture = (() { | |||
There was a problem hiding this comment.
I don't think this will work, now it will only run onMount once, at the same time as onLoad?
There was a problem hiding this comment.
Actually it is off because it will not run onmount at all (if onload returns a non null future). I am not sure why we should run onmount every time the widget tree rebuilds?
There was a problem hiding this comment.
Whatever is the logic between onload and unmount, we should not generate a new future every time the build method is called, since a widget rebuild may happen even because of things not related to Flame. In the previous implementation, we cached the future.
There was a problem hiding this comment.
Bottom line:: since Game is now a component, this seems more like a component lifecycle and should be treated as such: inside Flame. Hooks to the widget lifecycle may, be more specialized (on widget mount, on widget rebuild etc).
There was a problem hiding this comment.
I agree, it shouldn't trigger on each widget rebuild. But, we need onMount to trigger if the GameWidget is moved from one place in the tree to another (but is already loaded), or if it is removed and then later added back to the tree, is this even possible to differentiate from a normal widget rebuild?
There was a problem hiding this comment.
There are some possibilities here:
- The package user removes
GameWidgetfrom the tree temporarily: this unmounts the widget and kills the state (unless there is a global key keeping it), de-caching the future. - The package user moves
GameWidgetin the widget tree: Same as above. - The package user changes the game in which it passes to the
GameWidget: The future is de-cached here.
If the user keeps the state in a global key, we should assume it wants to keep things as it is.
There was a problem hiding this comment.
Alright, seems like this is the way to go then. Are the lifecycle docs in line with this?
Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
renancaraujo
force-pushed
the
renan/fix-gamewidget
branch
from
4b84109
to
d2edaf0
Compare
Description
There is a nasty black frame every time GameWidget rebuilds. Fixing that by memoizing the onLoad future that loads the game only once. Since changing the game instance between builds is already an anti-pattern, there is no problem caching the load state.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///) and updated/added examples indoc/examples.flutter formatand theflutter analyzedoes not report any problems.[next]inCHANGELOG.md.Draftstatus, by clicking on theReady for reviewbutton in this PR.Breaking Change
Does your PR require Flame users to manually update their apps to accommodate your change?
CHANGELOG.md).Related Issues
Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR.