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

feat: Use cached image when creating single source TiledAtlas if available #2348

Merged
merged 8 commits into from
Feb 22, 2023

Conversation

Hwan-seok
Copy link
Contributor

@Hwan-seok Hwan-seok commented Feb 17, 2023

Description

Detailed use-case examples are described in #2337.

As-is

  • A single source TiledAtlas doesn't check if there is a cached image that is already loaded in the memory.
  • It re-loads the image into the memory.

To-be

  • A single source TiledAtlas now uses the cached image if it exists.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • 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 or docs.

Breaking Change?

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

Related Issues

Closes: #2337

(await Flame.images.load(tiledImage.source!, key: key)).clone();

final image = (await Flame.images.load(tiledImage.source!)).clone();
Flame.images.add(key, image);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will also be two image objects added to the cache now right?

Copy link
Contributor Author

@Hwan-seok Hwan-seok Feb 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. But they share an image reference.

At present, TiledAtlas has a rule for storing keys.

  1. No image: atlas{empty} => Actually this is not used as a key because there is no image to store.
  2. A image: atlas{$source}
  3. Images: atlas{$source1,$source2}

To keep this rule, I added this: Flame.images.add(key, image);.
But If there is no need to keep this rule, the line can be eliminated.

I prefer removing this but it's totally on your call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the image is cloned before it is added to the cache again they don't share the reference.
I don't think there is a need to keep that rule, what do you think? :)

Copy link
Contributor Author

@Hwan-seok Hwan-seok Feb 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the image is cloned before it is added to the cache again they don't share the reference.

Oh, I was totally missed it. Yes, you are right again!

I don't think there is a need to keep that rule, what do you think? :)

I absolutely agree with you since there is a key property to TiledAtlas so that it could get a cached image if anyone wanted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do you think the clone is needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, feels like there must have been a reason for someone to put the clone there? Can you check who did it and we can ask? I'm currently on my phone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Hi @jtmcdole, can I ask a reason for the clone exists? I could roughly assume but curious about the exact reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spydon Maybe he's unavailable now. We should leave the clone and ask him later :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can keep using the image so long as it is in the cache. We don't go back to the cache each time we want to reference the image and instead use final Image? atlas;. If the cache is cleared for any reason, it disposes of the image and we now have an invalid image.

I remember there being a crash due to this... there should have been a test as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably some of your use cases are evicting the cache. Thanks for the answer 😄

@spydon spydon enabled auto-merge (squash) February 22, 2023 09:29
@spydon spydon merged commit 73467c9 into flame-engine:main Feb 22, 2023
@Hwan-seok Hwan-seok deleted the hwanseok.atlas-cache branch February 22, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the tileset image from the cache if it's already loaded
3 participants