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.

Sign up for GitHub

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

Jump to bottom

Availability to add/remove multiple overlays at once #1546

Closed
Hwan-seok opened this issue Apr 13, 2022 · 3 comments · Fixed by #1657
Closed

Availability to add/remove multiple overlays at once #1546

Hwan-seok opened this issue Apr 13, 2022 · 3 comments · Fixed by #1657
Labels
enhancement

Comments

@Hwan-seok
Copy link
Contributor

What could be improved

Currently, the ActiveOverlaysNotifier can add/remove only one by one.
It can add/remove multiple overlays at once.

Why should this be improved

Imagine adding/removing multiple sets of overlays.
It can lead to multiple function calls to flame users in addition to executing multiple notifyListeners() inside the Flame.

Any risks?

No

More information

flame/packages/flame/lib/src/game/mixins/game.dart

Line 319 in ac3d4bf

notifyListeners();

Are you interested in working on a PR for this? Yes
@spydon
Copy link
Member

spydon commented Apr 13, 2022

Sounds good to me!
Maybe we should also consider if we should prohibit modifications to the underlying set externally too, because now you could do overlays.value.addAll(..) for example and circumvent sending out notifications.

@st-pasha
Copy link
Contributor

Maybe we should also consider if we should prohibit modifications to the underlying set externally too,

This is already done in #1498.

@spydon
Copy link
Member

spydon commented Apr 13, 2022

I guess we should wait with this issue until that PR is merged, since it changes that class a lot.

@Hwan-seok Hwan-seok mentioned this issue May 26, 2022
Merged
7 tasks
spydon pushed a commit that referenced this issue May 27, 2022
@Hwan-seok
feat: Add ability to add/remove multiple overlays at once (#1657)
0ac84c0 
As described #1546, It would be more convenient and can reduce the game widget updates if adding or removing overlays at once.
st-pasha pushed a commit to st-pasha/flame that referenced this issue May 29, 2022
@Hwan-seok @st-pasha
feat: Add ability to add/remove multiple overlays at once (flame-engi…
005ca7b 
…ne#1657)

As described flame-engine#1546, It would be more convenient and can reduce the game widget updates if adding or removing overlays at once.
st-pasha pushed a commit to st-pasha/flame that referenced this issue Jun 3, 2022
@Hwan-seok @st-pasha
feat: Add ability to add/remove multiple overlays at once (flame-engi…
31281dd 
…ne#1657)

As described flame-engine#1546, It would be more convenient and can reduce the game widget updates if adding or removing overlays at once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Add ability to add/remove multiple overlays at once Hwan-seok/flame
3 participants
@spydon @st-pasha @Hwan-seok