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

When loading csb files, prevent repeated loading of plist files #1844

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

tkzcfc
Copy link
Contributor

@tkzcfc tkzcfc commented Apr 22, 2024

Describe your changes

Issue ticket number and link

Checklist before requesting a review

For each PR

  • Add Copyright if it missed:
    - "Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."

  • I have performed a self-review of my code.

    Optional:

    • I have checked readme and add important infos to this PR.
    • I have added/adapted some tests too.

For core/new feature PR

  • I have checked readme and add important infos to this PR.
  • I have added thorough tests.

core/2d/SpriteFrameCache.cpp Outdated Show resolved Hide resolved
@rh101
Copy link
Contributor

rh101 commented Apr 22, 2024

Is there any reason that you can't use SpriteFrameCache::isSpriteFramesWithFileLoaded to check if a sprite sheet is already loaded?

For instance, you've added _loadedPlists, and use it like this:

if (!_loadedPlists.contains(plist + png))
{
    // ...
}

Does the following not work?

if (!SpriteFrameCache::getInstance()->isSpriteFramesWithFileLoaded(plist + png))
{
    // ...
}

@tkzcfc
Copy link
Contributor Author

tkzcfc commented Apr 22, 2024

removeUnusedSpriteFrames may release unused sprite frames

@tkzcfc
Copy link
Contributor Author

tkzcfc commented Apr 22, 2024

We need to reload

@rh101
Copy link
Contributor

rh101 commented Apr 22, 2024

removeUnusedSpriteFrames may release unused sprite frames

Sorry, I'm not quite sure that I understand what you mean.

isSpriteFramesWithFileLoaded is implemented as follows:

bool SpriteFrameCache::isSpriteFramesWithFileLoaded(std::string_view plist) const
{
    return isSpriteSheetInUse(plist) && isPlistFull(plist);
}

It checks if the sprite sheet entry is full via isPlistFull, so if any frames were removed through removeUnusedSpriteFrames, then isPlistFull would return false, which means isSpriteFramesWithFileLoaded returns false. Isn't that the case?

@tkzcfc
Copy link
Contributor Author

tkzcfc commented Apr 22, 2024

ok, so we don’t even need the _loadedPlists variable?

@rh101
Copy link
Contributor

rh101 commented Apr 22, 2024

ok, so we don’t even need the _loadedPlists variable?

Sorry, yes, that is what I meant. Have you checked if SpriteFrameCache::getInstance()->isSpriteFramesWithFileLoaded works for you? If it does work, then you do not need _loadedPlists.

@tkzcfc
Copy link
Contributor Author

tkzcfc commented Apr 22, 2024

Let me try

@rh101
Copy link
Contributor

rh101 commented Apr 22, 2024

@halx99 I can't quite figure out how to set the unresolved conversation as resolved. I thought it was the "review changes", but that wasn't it, so please ignore the review approval listed above.

core/2d/SpriteFrameCache.h Outdated Show resolved Hide resolved
core/2d/SpriteFrameCache.cpp Outdated Show resolved Hide resolved
@halx99 halx99 added this to the 2.1.3 milestone Apr 22, 2024
@halx99 halx99 added the enhancement New feature or request label Apr 22, 2024
@halx99 halx99 merged commit 16cb4b5 into axmolengine:dev Apr 23, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants