-
-
Notifications
You must be signed in to change notification settings - Fork 982
feat: Use a Free List Strategy on BatchItem indexes within SpriteBatch and return index from .add() #3650
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
base: main
Are you sure you want to change the base?
feat: Use a Free List Strategy on BatchItem indexes within SpriteBatch and return index from .add() #3650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I added one comment and I think we need tests for the added functionality!
Can you explain your use-case a bit further? I'm not sure I understand why this is needed to do animations based on sprite batches, and whether that is even anything that the sprite batch should be aware of. It seems to me like one would want all the sprites related to an animation within the atlas, not replace the entries. |
In my use case, I often spawn hundreds of orbs that contain health. Sprite batching significantly improves performance here. Each orb has four states: materialize, idle, dissipate, and collect. All animations are texture-packed into a single image. I’m generating BatchItems dynamically for the materialize animation, then replacing and eventually removing them. Currently, SpriteBatch management is index-based. While I could store and track indices in my orb model, it’s unreliable—especially when animations change rapidly. More importantly, there’s no built-in way to remove a BatchItem by index, which is what inspired this PR. You wouldn’t manage database indexes outside the database—likewise, managing BatchItems by a unique ID makes CRUD operations more reliable and intuitive. This approach ensures I always target the correct item, even as orb states change. Regarding your comment about animation sprites needing to be within the atlas: to use drawAtlas() efficiently for hundreds of animated orbs, I need to create and replace BatchItems on every frame. Unless there’s a better methodology I’ve missed, that’s currently the only way to animate with sprite batching. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It would be good to have a simple example of how to use it with animations.
removeAt
,removeById
andids
are lacking tests.
Other than that and the comment on ids
I think it looks good!
@spydon A problem I'm running into currently with this implementation is race conditions if you're mutating the index from separate components. I've implemented a Free List Strategy to account for this and it's working perfectly in my game. Here's an overview of my changes: Error safety improvements
BONUS! Performance improvements
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this PR should be split up in two, one that is introducing the id
, which should be quite simple to land. And then one with the performance changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, Thanks for your contribution!
@gnarhard can you have a look at why the tests are failing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is going wrong in here, it seems to remove some images that it shouldn't remove, check this diff with the updated goldens: a474672
Head branch was pushed to by a user without write access
@spydon I need your help on fixing these tests. These tests were passing before the move to Flutter 3.35 and other changes to main. I'm wondering if the precision issues are related to Matrix4 returning a Float32List instead of a Float64List. I combed through the file and compared it to main, but I can't figure out why the goldens are different and I don't see a reason for that to happen from my updates. |
5f09d9d
to
16ae942
Compare
…h and return index from .add()
16ae942
to
69ae8a4
Compare
I squashed all the commits in this PR so that it can easily cherry-picked into an older version of Flame and try the tests there. I don't think this is related to any precision issues, because as you can see in the image diffs that I linked, some tiles are just completely gone. |
@gnarhard I cherry-picked this PR onto Flame v1.29.0 and used Flutter 3.27.1 to run the tests, and the same goldens still fail, so I'm pretty certain the bug is within this PR and not external. |
Okay, thanks for doing that. I'll look into it again. |
Description
This PR adds functionality to make it easier to manage BatchItems within a SpriteBatch. .add() and .addTransform() now return the generated index for tracking the BatchItem. This now uses a Free List Strategy to avoid concurrent modifications and allow manipulations on the BatchItems from different components in the same frame.
Originally, this PR added an ID for BatchItem management, but that was replaced with index management using the Free List Strategy.
Checklist
docs
and added dartdoc comments with///
.examples
ordocs
.Breaking Change?
Related Issues