feat: Return cached assets instead of replacing them on reload

[nojaf]

Summary

Problem

When switching scenes using go(), calling loadSprite() with the same asset name would replace the already-loaded cached asset with a new pending Asset object. This caused issues when:

1. Scene A loads sprites and _k.assets.loaded becomes true
2. User triggers go("sceneA") to restart the scene
3. loadSprite() is called again with the same names
4. AssetBucket.add() replaces cached assets with new pending ones
5. onLoad() fires immediately (because _k.assets.loaded is still true)
6. getSprite(name).data returns null because the new Promise hasn't resolved yet

This resulted in game objects being created with missing sprite data on scene restart.

Solution

Cache hit optimization: AssetBucket.add() now returns the existing cached asset if it's already loaded, instead of replacing it with a new pending asset.

add(name: string | null, loader: Promise<D>): Asset<D> {
    const id = name ?? this.lastUID++ + "";

    // If asset already exists and is loaded, return the cached version
    const existing = this.assets.get(id);
    if (existing && existing.loaded && existing.data) {
        return existing;
    }
    // ... create new asset
}

Escape hatch: For the edge case where you actually want to replace an asset with a different URL, a new unloadSprite(name) function is provided:

// Replace a sprite with a different image
unloadSprite("player");
loadSprite("player", "/sprites/v2/player.png");

• Changeloged

[dragoncoder047]

I would say hold off merging this until at least some part of #1021 is merged, since while it unregisters the sprite, the old one is still hanging around in the texture and isn't reclaimed. #1021 should in theory allow the sprite to be removed from there and the now-unused space made available to load other sprites.

[lajbel]

@nojaf #1021 was updated, could you check your PR and update it if needed? Then request review for me. Thank you

[dragoncoder047]

There's two ways to do things here:

1. explicit unloading - what you are adding, where a second loadSprite() with the same ID gets blocked/discarded, and an explicit unloadSprite() is needed to free the slot before a sprite can be re-loaded.

2. implicit unloading - what I would suggest as the other asset systems also do it this way (you can loadSound() with the same ID and it will replace the existing one when it loads... however I would suggest also fixing those so that while the 2nd one is being loaded, you can still access the 1st one's data). With implicit unloading, once an asset with id X is loaded, loading another asset of the same kind with id X causes the onLoad callback of the new asset to be set up so that it will be addLoaded()'ed when it finished, but it does not replace the existing one with the pending one until then, so data will never be null.

Also, several files have only formatting-related changes, please put them back to what they were before / what they are on master, if pnpm fmt doesn't do that already.

[dragoncoder047]

@nojaf once you merge in master, this is one place where the actual TexPacker unload code can be added.

[dragoncoder047]

It just completely discards the second-time loader promise here. Why not create a new asset for it, and set it up to replace the existing one once it loads?

[dragoncoder047]

2nd place to add the TexPacker unload code. Make a subclass of AssetBucket that overrides this, and then use it for the sprites AssetBucket.

[dragoncoder047]

Forgot to mention here initially, the add() and addLoaded() should probably already be calling this, so that the sprite subclass can handle when the sprite is loaded for the 2nd time, once the 2nd sprite loads it calls remove() on the old one and then addLoaded()'s the new one.

[nojaf]

Hey @dragoncoder047 , thanks for the review!

I ran pnpm fmt, so I'm not sure what it up here.
CI should run against PRs to catch these things.

Anyway, let me know if I got everything.