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.

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

Performance is dramatically decreased After #1610 #1614

Closed
Hwan-seok opened this issue May 10, 2022 · 15 comments · Fixed by #1637
Closed

Performance is dramatically decreased After #1610 #1614

Hwan-seok opened this issue May 10, 2022 · 15 comments · Fixed by #1637
Assignees
Labels

Comments

@Hwan-seok
Copy link
Contributor

Hwan-seok commented May 10, 2022

Current bug behaviour

I was very looked forward to #1605 and I upgraded flame to this.
But I found the performance Is Very slow after upgrade. (see videos at the end).
Android can check the performance at least but IOS cannot even see the app launches because it shutting down immediately after launched due to memory issue.
So I bisected what commit went wrong.
After some reverts, I found #1610 is the problem.

It is very normal at #1596 (commit hash 60df3b9)
But after #1610 (b4ad498), It has dramatically slown performance.

I carefully assume that it has some memory leak and GCs constantly.

Expected behaviour

Fix the performance issue.

Flutter doctor output


[✓] Flutter (Channel stable, 2.10.5, on macOS 12.1 21C52 darwin-arm, locale ko-KR)
    • Flutter version 2.10.5 at /Users/lafity101/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 5464c5bac7 (3 weeks ago), 2022-04-18 09:55:37 -0700
    • Engine revision 57d3bac3dd
    • Dart version 2.16.2
    • DevTools version 2.9.2

[✓] Android toolchain - develop for Android devices (Android SDK version 32.0.0)
    • Android SDK at /Users/lafity101/Library/Android/sdk
    • Platform android-32, build-tools 32.0.0
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7249189)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 13.3.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2020.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7249189)

[✓] IntelliJ IDEA Ultimate Edition (version 2021.3)
    • IntelliJ at /Applications/IntelliJ IDEA.app
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart

[✓] VS Code (version 1.67.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.40.0

[✓] Connected device (3 available)
    • SM G977N (mobile)           •                • android-arm64  • Android 11 (API 30)
    • iPhone 12mini (mobile) •  • ios            • iOS 14.8 18H17

[✓] HTTP Host Availability
    • All required HTTP hosts are available

More environment information

  • Explained at the beginning

More information

Before #1610

Screen_Recording_20220510-143933_tagroom.mp4

After #1610 - I didn't manipulated video speed

Screen_Recording_20220510-144535_tagroom.mp4
@Hwan-seok Hwan-seok added the bug label May 10, 2022
@spydon
Copy link
Member

spydon commented May 10, 2022

@nathanaelneveux must be the PictureRecorder that is being created?

@nathanaelneveux
Copy link
Contributor

@nathanaelneveux must be the PictureRecorder that is being created?

That does seem to be the case - I was able to replicate this by loading my test map x30 and then turning off and on the _generateAtlas code and there is a fps drop with it on.

I'm not sure the PictureRecorder can be made reusable since it's suppose to invalidate itself and the canvas it's attached to on endRecording() so it will probably need to be replaced?

At worse this shouldn't be too difficult to remake with a round trip through image.toByteData(). I had also looked at the image package when first working on this but didn't know how you all felt about adding another package?

@nathanaelneveux
Copy link
Contributor

flutter/flutter#28912

@st-pasha
Copy link
Contributor

But _generateAtlas only runs once, when the SpriteBatch is loaded -- how can it affect the performance? Unless it generates the image in some kind of inefficient format that needs to be recoded on the fly on every game tick...

@nathanaelneveux
Copy link
Contributor

But _generateAtlas only runs once, when the SpriteBatch is loaded -- how can it affect the performance? Unless it generates the image in some kind of inefficient format that needs to be recoded on the fly on every game tick...

It produces an Image but it doesn't currently cache it in Images

@nathanaelneveux
Copy link
Contributor

Just to put some numbers behind it - using test map x30:
• on 60df3b - FPS 21-23
• on b4ad49 - FPS 4-5
• with _generateAtlas reduced to

static Future<Image> _generateAtlas(Images? images, String path) async {
    final _images = images ?? Flame.images;
    final image = await _images.load(path);
    return image;
  }
  • FPS 20-24

In all cases once the spriteBatch has been loaded images are not recreated - _generateAtlas is not called again after the initial load.

@nathanaelneveux
Copy link
Contributor

Ok so I have a proof of concept _generateAltas that creates the new image with byte operations instead of a PictureRecorder and FPS on my test file is back over 20. I'll package this up into a PR tomorrow.

I'd like to add caching to this final atlas image since I could see situations where a tileset would be reused and so shouldn't be re-generated as a flipped image but I think I'll need some kind of Images.exists function as well since the image key wouldn't be based solely on the file path. I'll try tackling that in the PR as well.

I did rely heavily on this project for converting the raw image bytes back into a Image but I don't think adding that package will be an issue :)

@spydon
Copy link
Member

spydon commented May 14, 2022

I'd like to add caching to this final atlas image since I could see situations where a tileset would be reused and so shouldn't be re-generated as a flipped image but I think I'll need some kind of Images.exists function as well since the image key wouldn't be based solely on the file path. I'll try tackling that in the PR as well.

Coincidentally I think I fixed what you need in #1624?

@nathanaelneveux
Copy link
Contributor

I'd like to add caching to this final atlas image since I could see situations where a tileset would be reused and so shouldn't be re-generated as a flipped image but I think I'll need some kind of Images.exists function as well since the image key wouldn't be based solely on the file path. I'll try tackling that in the PR as well.

Coincidentally I think I fixed what you need in #1624?

Nice! That should work perfect

@spydon
Copy link
Member

spydon commented May 16, 2022

Any progress on this issue @nathanaelneveux?
Is there anything more that you'd need from the Flame engine that I could assist with? :)

@nathanaelneveux
Copy link
Contributor

nathanaelneveux commented May 17, 2022

Here is the branch I've been working in. I should have posted that sooner so you could track where I was at.

My original proof of concept had a discoloring issue that took me an embarrassingly long time to realize that I was flipping the RGBA to ABGR.

My most recent commit to the above branch passes all test and maintains performance but I'm not totally happy with it yet.

  1. I feel like I can be more efficient with object creation here and do the entire operation on the first Uint8List
  2. After finding decodeImageFromPixels() I believe I can do the whole operation without adding the Bitmap package
  3. 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.

@spydon
Copy link
Member

spydon commented Jun 3, 2022

Can you test the updated PR @Hwan-seok?
It should be solved now. :)

@Hwan-seok
Copy link
Contributor Author

Hwan-seok commented Jun 3, 2022

@spydon
My app shows blank game widget in the main branch so I need some time to investigate what is causing this.

Edit: Removing FPSCounter solved this.

I tested d7b0940 and it works fine :)

@st-pasha
Copy link
Contributor

st-pasha commented Jun 3, 2022

My app shows blank game widget in the main branch. Edit: Removing FPSCounter solved this.

This sounds disturbing... What if you replace FPSCounter with FpsComponent or FpsTextComponent?

@Hwan-seok
Copy link
Contributor Author

@st-pasha
It works after replacing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants