fix: performance improvements on `SpriteBatch` APIs #1637

nathanaelneveux · 2022-05-18T03:31:04Z

Description

Fix for the performance decrease mentioned in #1614

Some items that need work:

I feel like I can be more efficient with object creation here and do the entire operation on the first Uint8List
After finding decodeImageFromPixels() I believe I can do the whole operation without adding the Bitmap package
I need to do more testing of the caching of the image. What I've written so far seems straight forward but I'm not totally convinced subsequent calls to retrieve the image from the cache are working. As I was stepping through the debugger testing something else I kept seeing the same image being generated, but I haven't done a lot of testing here yet.

Checklist

The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
I have read the Contributor Guide and followed the process outlined for submitting PRs.
I have updated/added tests for ALL new/updated/fixed functionality.
I have updated/added relevant documentation in docs and added dartdoc comments with ///.
I have updated/added relevant examples in examples.

Breaking Change

Yes, this is a breaking change.
No, this is not a breaking change.

Related Issues

Fixes #1614

packages/flame/lib/src/sprite_batch.dart

st-pasha

I don't think there is anything wrong with the current approach that uses PictureRecorder. (If there is, we need to have a clear confirmation of that, then we need to file a bug to Flutter team, and use the suggested workaround until that bug is resolved, but also put it into a separate function so that it can be reused through multiple other places where we use PictureRecorder in Flame).

The more probable cause of #1614 is that SpriteBatch.load() method attempts to generate atlas on every call - even if the sprite batch won't be using the flipped images later on. And that image generation is not cached. So, if the user has code that does SpriteBatch.load() whenever a component is created, and if many such components are created per second, then there will be lots of unnecessary image generation happening. This explanation fits well with the problem described in #1614 as "IOS cannot even see the app launches because it shutting down immediately after launched due to memory issue."

So, I suggest to do the following:

Restore the original image generation through PictureRecorder;
Make invocation of _generateAtlas happen only if the user explicitly requests support for flipped sprites in their SpriteBatch;
Cache that generated image;
(Optionally) Measure the speed difference between generating an image via PictureRecorder vs via the Bitmap library.

packages/flame/pubspec.yaml

st-pasha

@nathanaelneveux can we please start with the following:

Add a boolean flag into SpriteBatch.load() which will tell whether to generate the mirrored image or not. We don't want to create extra images for those users who will not be using them.
For now, have two methods _generateAtlas1 and _generateAtlas2 with the PictureRecorder and the Bitmaps approaches -- this way it would be easier for us to compare and debug the two methods.

nathanaelneveux · 2022-05-20T04:01:44Z

I think we can do better than a flag for the generation of the mirrored images. For one thing SpriteBatch.load is buried pretty deep within RenderableTiledMap with the main user facing function being .fromFile().

How do you feel about doing it as a "lazy load" where once we get a call to addTransform with flip = true we call _generateAtlas?

spydon · 2022-05-20T18:00:11Z

How do you feel about doing it as a "lazy load" where once we get a call to addTransform with flip = true we call _generateAtlas?

Will that not potentially impact performance mid-game instead of in onLoad?

packages/flame/lib/src/sprite_batch.dart

nathanaelneveux · 2022-05-20T19:40:00Z

How do you feel about doing it as a "lazy load" where once we get a call to addTransform with flip = true we call _generateAtlas?

Will that not potentially impact performance mid-game instead of in onLoad?

addTransform is part of the loading process as well and should happen pretty early on in the lifecycle of this component. You have to add all your transforms before you would do your first render. See how its done in Flame Tiled for example.

spydon · 2022-05-20T19:52:50Z

addTransform is part of the loading process as well and should happen pretty early on in the lifecycle of this component. You have to add all your transforms before you would do your first render. See how its done in Flame Tiled for example.

Alright, sounds like it makes sense to have it lazily initialized then!

Co-authored-by: Pasha Stetsenko <stpasha@gmail.com>

packages/flame/lib/src/sprite_batch.dart

spydon · 2022-05-24T20:20:23Z

@nathanaelneveux any progress on this one? Anything that you need help with? :)

nathanaelneveux · 2022-05-26T01:07:48Z

@nathanaelneveux any progress on this one? Anything that you need help with? :)

I'm honestly struggling with what pattern to use to do this with null safety - I'll post what I have so far in a bit with some notes - I could use some advice.

nathanaelneveux · 2022-05-26T15:53:16Z

So obviously you’ll see in the most recent commit that it’s not quite there yet. It’s still generating the flippableAtlas right away.

I was hoping late would cause it to wait until it was used to begin generating which right now would be at render but it isn’t waiting. Once I can get late working then the next step would be to move the late generation up into the addTranform so that it’s still being generated early on in the lifecycle when needed.

I need a reference to Images and a reproducible key - which right now I’m using path with a suffix. I’m trying to avoid adding those to the constructor because I think if your using the constructor it doesn’t make sense to do lazy initialization? Or maybe I’m over thinking that?

nathanaelneveux · 2022-06-02T03:17:55Z

Even though this is passing tests this isn't working on my test map - the flips are wrong. I'll have a test that looks at a larger atlas to get the bottom of this and make sure it doesn't happen again.

spydon · 2022-06-02T19:46:57Z

Even though this is passing tests this isn't working on my test map - the flips are wrong. I'll have a test that looks at a larger atlas to get the bottom of this and make sure it doesn't happen again.

It seems like it is failing both of the tests that you added, even the one not using the atlas?
What error are you experiencing with larger maps?

nathanaelneveux · 2022-06-03T00:48:26Z

Even though this is passing tests this isn't working on my test map - the flips are wrong. I'll have a test that looks at a larger atlas to get the bottom of this and make sure it doesn't happen again.

It seems like it is failing both of the tests that you added, even the one not using the atlas? What error are you experiencing with larger maps?

There was an error introduced when I stacked the flipped image vertically instead of horizontally in 35e08d8. source.top + source.height, should have been atlas.height/2 + source.top. This was still passing tests because the tile map in this test was only 1 tile high - source.top = atlas.height/2 in this instance. I reverted to the generateAtlas I had before 35e08d8. I believe that isn't passing tests now because atlas.width is no longer reliable - it may still be the width of the atlas before _makeFlippedAtlas() completes.

nathanaelneveux · 2022-06-03T01:23:19Z

I was wrong about the screenshot I posted yesterday - the above is what is produced by Canvas.drawImageRect. So the batchItem.source is also going to need some adjustments.

nathanaelneveux · 2022-06-03T04:01:15Z

Tests are passing now - I know there must be a better way to do the conditional in the BatchItem construction but its too late here for me to think straight

spydon

Thanks for your very hard and long work on this PR @nathanaelneveux, very appreciated! 🎉
(Are you on our discord?)

spydon · 2022-06-03T12:09:43Z

packages/flame/lib/src/sprite_batch.dart

+          ..translate(source.width / 2, source.height / 2)
+          ..rotateY(flip ? pi : 0)
+          ..translate(-source.width / 2, -source.height / 2),


Could these be defined directly in the Matrix4? 🤔
@st-pasha Do you know how to do that, or if it is possible?

We could have some extension method to make a Matrix4 out of RSTransform, looks like there isn't one right now.

That _defaultScale looks mighty suspicious to me as well, I think we can just remove it without anyone noticing.

Removed it, should this really be 0 and not 1? 🤔

This is the difference between the matrix's with the rotateY code and without:

WITH FLIPS [0] 1.0,-0.0,-1.2246467991473532e-16,32.0 [1] -0.0,-1.0,0.0,16.0 [2] -0.0,0.0,0.0,0.0 [3] -0.0,0.0,0.0,1.0 [0] -0.0,1.0,0.0,48.0 [1] 1.0,0.0,-1.2246467991473532e-16,0.0 [2] -0.0,0.0,0.0,0.0 [3] -0.0,0.0,0.0,1.0 [0] -0.0,-1.0,0.0,48.0 [1] -1.0,0.0,1.2246467991473532e-16,32.0 [2] -0.0,0.0,0.0,0.0 [3] -0.0,0.0,0.0,1.0 [0] -1.0,-0.0,1.2246467991473532e-16,64.0 [1] -0.0,1.0,0.0,16.0 [2] -0.0,0.0,0.0,0.0 [3] -0.0,0.0,0.0,1.0 NO FLIPS [0] -1.0,-0.0,0.0,48.0 [1] 0.0,-1.0,0.0,16.0 [2] 0.0,0.0,0.0,0.0 [3] 0.0,0.0,0.0,1.0 [0] 0.0,1.0,0.0,48.0 [1] -1.0,0.0,0.0,16.0 [2] 0.0,0.0,0.0,0.0 [3] 0.0,0.0,0.0,1.0 [0] 0.0,-1.0,0.0,48.0 [1] 1.0,0.0,0.0,16.0 [2] 0.0,0.0,0.0,0.0 [3] 0.0,0.0,0.0,1.0 [0] 1.0,-0.0,0.0,48.0 [1] 0.0,1.0,0.0,16.0 [2] 0.0,0.0,0.0,0.0 [3] 0.0,0.0,0.0,1.0

(.toString doesn't look to be in the same order as the Matrix4 initializer)

packages/flame/lib/src/sprite_batch.dart

Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>

nathanaelneveux · 2022-06-04T00:37:59Z

Thanks for your very hard and long work on this PR @nathanaelneveux, very appreciated! 🎉
(Are you on our discord?)

Phew :) that definitely ending up being more than I expected when first tackling this as a hobby project lol. Thank you all for your patience and help - great community you’ve built here

packages/flame/lib/src/sprite_batch.dart

packages/flame/lib/src/cache/images.dart

alestiago · 2022-06-04T10:47:12Z

packages/flame/lib/src/cache/images.dart

+  /// [name] in the cache.
+  Future<Image> fetchOrGenerate(
+    String name,
+    Future<Image> Function() imageGenerator,


Why is it named "Generator" instead of "Builder"?

I didn't name it, but it's not a builder (if the builder name is used according to the builder pattern).

alestiago · 2022-06-04T10:47:46Z

packages/flame/lib/src/cache/images.dart

+  ///
+  /// If the [imageGenerator] is used, the resulting [Image] is stored with
+  /// [name] in the cache.
+  Future<Image> fetchOrGenerate(


Is this meant to fetch and if it fails to do so then, it builds an image and also caches it?

Yes, is that not clear from the Dartdocs? 😅

nathanaelneveux added 3 commits May 14, 2022 21:59

test without picturerecorder

3b3f457

build atlas with bytes

170f032

working version

7ca33d6

spydon reviewed May 18, 2022

View reviewed changes

packages/flame/lib/src/sprite_batch.dart Outdated Show resolved Hide resolved

st-pasha reviewed May 18, 2022

View reviewed changes

spydon reviewed May 18, 2022

View reviewed changes

packages/flame/pubspec.yaml Outdated Show resolved Hide resolved

remove bitmap dependency

9f0932e

spydon requested review from erickzanardo, renancaraujo, luanpotter and wolfenrain May 19, 2022 12:18

st-pasha suggested changes May 19, 2022

View reviewed changes

nathanaelneveux added 3 commits May 19, 2022 23:19

add asyncronous caching & retrieval

80206a6

add generateAtlas variations

6e3da16

added flag for _generateAtlas

9c582bc

vetically aligned images in both for easier testing

35e08d8

st-pasha reviewed May 20, 2022

View reviewed changes

packages/flame/lib/src/sprite_batch.dart Outdated Show resolved Hide resolved

st-pasha reviewed May 20, 2022

View reviewed changes

packages/flame/lib/src/sprite_batch.dart Outdated Show resolved Hide resolved

nathanaelneveux and others added 2 commits May 20, 2022 21:19

Update packages/flame/lib/src/sprite_batch.dart

725e8ae

Co-authored-by: Pasha Stetsenko <stpasha@gmail.com>

Merge branch 'main' into generateAtlas-Performance

aefd775

st-pasha reviewed May 21, 2022

View reviewed changes

packages/flame/lib/src/sprite_batch.dart Outdated Show resolved Hide resolved

Merge branch 'main' into generateAtlas-Performance

fcce7d2

attempt to lazy initialize flipped atlas

c8933f2

fix for larger atlases

2f52d09

spydon added 3 commits June 2, 2022 07:46

Merge branch 'main' into generateAtlas-Performance

e21e7cf

Remove print

2af8e16

Merge branch 'main' into generateAtlas-Performance

006824c

more robust flipping and testing

37b2dd5

correct flips for drawImageRect

ad0b1b3

Fix analyze issues

c410689

spydon approved these changes Jun 3, 2022

View reviewed changes

Merge branch 'main' into generateAtlas-Performance

a904de5

spydon reviewed Jun 3, 2022

View reviewed changes

spydon changed the title ~~fix: performance of _generateAtlas WIP~~ fix: performance of _generateAtlas Jun 3, 2022

wolfenrain changed the title ~~fix: performance of _generateAtlas~~ fix: performance improvements on SpriteBatch APIs Jun 3, 2022

wolfenrain approved these changes Jun 3, 2022

View reviewed changes

packages/flame/lib/src/sprite_batch.dart Outdated Show resolved Hide resolved

spydon and others added 3 commits June 3, 2022 14:56

Update packages/flame/lib/src/sprite_batch.dart

d7b0940

Co-authored-by: Jochum van der Ploeg <jochum@vdploeg.net>

Merge branch 'main' into generateAtlas-Performance

4528470

Remove _defaultScale

4b8c71c

alestiago reviewed Jun 4, 2022

View reviewed changes

packages/flame/lib/src/sprite_batch.dart Outdated Show resolved Hide resolved

alestiago reviewed Jun 4, 2022

View reviewed changes

packages/flame/lib/src/cache/images.dart Outdated Show resolved Hide resolved

Fix comments

9207e77

alestiago reviewed Jun 4, 2022

View reviewed changes

Merge branch 'main' into generateAtlas-Performance

89fdca1

spydon enabled auto-merge (squash) June 4, 2022 15:17

spydon merged commit 4b19a1b into flame-engine:main Jun 4, 2022

Hwan-seok mentioned this pull request Jun 8, 2022

[flame_tiled] Tile flip makes performance issue. #1714

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: performance improvements on `SpriteBatch` APIs #1637

fix: performance improvements on `SpriteBatch` APIs #1637

nathanaelneveux commented May 18, 2022 •

edited

Loading

st-pasha left a comment

st-pasha left a comment

nathanaelneveux commented May 20, 2022

spydon commented May 20, 2022

nathanaelneveux commented May 20, 2022

spydon commented May 20, 2022

spydon commented May 24, 2022

nathanaelneveux commented May 26, 2022

nathanaelneveux commented May 26, 2022

nathanaelneveux commented Jun 2, 2022

spydon commented Jun 2, 2022

nathanaelneveux commented Jun 3, 2022 •

edited

Loading

nathanaelneveux commented Jun 3, 2022

nathanaelneveux commented Jun 3, 2022

spydon left a comment

spydon Jun 3, 2022

st-pasha Jun 3, 2022

spydon Jun 3, 2022 •

edited

Loading

nathanaelneveux Jun 4, 2022

nathanaelneveux commented Jun 4, 2022

alestiago Jun 4, 2022

spydon Jun 4, 2022

alestiago Jun 4, 2022

spydon Jun 4, 2022

fix: performance improvements on SpriteBatch APIs #1637

fix: performance improvements on SpriteBatch APIs #1637

Conversation

nathanaelneveux commented May 18, 2022 • edited Loading

Description

Checklist

Breaking Change

Related Issues

st-pasha left a comment

Choose a reason for hiding this comment

st-pasha left a comment

Choose a reason for hiding this comment

nathanaelneveux commented May 20, 2022

spydon commented May 20, 2022

nathanaelneveux commented May 20, 2022

spydon commented May 20, 2022

spydon commented May 24, 2022

nathanaelneveux commented May 26, 2022

nathanaelneveux commented May 26, 2022

nathanaelneveux commented Jun 2, 2022

spydon commented Jun 2, 2022

nathanaelneveux commented Jun 3, 2022 • edited Loading

nathanaelneveux commented Jun 3, 2022

nathanaelneveux commented Jun 3, 2022

spydon left a comment

Choose a reason for hiding this comment

spydon Jun 3, 2022

Choose a reason for hiding this comment

st-pasha Jun 3, 2022

Choose a reason for hiding this comment

spydon Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

nathanaelneveux Jun 4, 2022

Choose a reason for hiding this comment

nathanaelneveux commented Jun 4, 2022

alestiago Jun 4, 2022

Choose a reason for hiding this comment

spydon Jun 4, 2022

Choose a reason for hiding this comment

alestiago Jun 4, 2022

Choose a reason for hiding this comment

spydon Jun 4, 2022

Choose a reason for hiding this comment

fix: performance improvements on `SpriteBatch` APIs #1637

fix: performance improvements on `SpriteBatch` APIs #1637

nathanaelneveux commented May 18, 2022 •

edited

Loading

nathanaelneveux commented Jun 3, 2022 •

edited

Loading

spydon Jun 3, 2022 •

edited

Loading