axmolengine / axmol

Axmol Engine – A Multi-platform Engine for Desktop, XBOX (UWP) and Mobile games. (A fork of Cocos2d-x-4.0)
https://axmol.dev
MIT License
716 stars 181 forks source link

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

Closed tkzcfc closed 1 month ago

tkzcfc commented 1 month ago

Describe your changes

Issue ticket number and link

Checklist before requesting a review

For each PR

For core/new feature PR

rh101 commented 1 month ago

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 commented 1 month ago

removeUnusedSpriteFrames may release unused sprite frames

tkzcfc commented 1 month ago

We need to reload

rh101 commented 1 month ago

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 commented 1 month ago

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

rh101 commented 1 month ago

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 commented 1 month ago

Let me try

rh101 commented 1 month ago

@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.