fix: refresh packer for direct referenced sprites resolving

[dragoncoder047]

also some trivial optimizations

this is so trivial it probably doesn't even need changelog since it was fine in a27 so the bug will be fixed by the next release if this is merged before then

[imaginarny]

Ok so, is this then a follow up on recent fixes that got merged in master as intended a27.1 while master currently contains only fixes for a27? Since you say its fine in a27 and imply it isn't in master anymore. If so, should be merged before that as well.

Second question, how do I test it to confirm fix and what would be the real use case for directly referencing sprites to see? (like in #1075?)

[dragoncoder047]

this would be things like where you would use

const bean = loadBean();

add([
    sprite(bean);
]);

instead of

loadBean();

add([
    sprite("bean");
]);

[lajbel]

@dragoncoder047 I think this use case could use an automated test?

[dragoncoder047]

how the heck would you automate a test like that? do you mean another non-automated playtest?

[lajbel]

Use vitest. Anyway I don't know if it will work, just saying as we're pointing to have more stuff tested

[dragoncoder047]

it randomly fails random files, but i think i got something that works

[imaginarny]

Well, I realized this also changes behavior of when sprites are added to the texture. Initially I thought it was like it is now, but it wasn't (unlike fonts) and I was somewhat relieved then.

So, before a sprite was added as soon as you did loadSprite. Now it is added only if you really draw it. So the question is, should there be another perf concern and more testing done for that? Like is tex packer fast enough when sprites are added on fly? Like a whole spritesheet too. You already mentioned that for the text there is a frame drop since letters worked like that. Would like to see a playtest with measures when a lot is loaded and then sprites are drawn after some delay, one by one or multiple at once, some heavier ones as well as dungeon spriteAtlas we have. What will performance then look fps-wise.

[dragoncoder047]

No, it loads the sprite data on to the canvas as they're loaded. The thing I was optimizing was cutting how often it synced that canvas to the GPU, which was really, really slow compared to adding another image to the canvas.

[imaginarny]

Alright, so they actually are in the texture, but then my saved ref spriteData is outdated now. Heh, this is something similar to what MF said before (for changes in frames/quad, which don't happen), but tests showed it wasn't needed.

So when you reference spriteData directly and e.g. use its texture, it will get outdated after you load more stuff. That's where I saw it wasn't updating and thought new sprites weren't added to the texture. After calling getSprite again, I see they are there - if you say repack already happened on loadSprite and not on my second getSprite call itself. So if that's the case, there isn't real life usage for something like that, since the sprite itself, its frame/quad doesn't change anyway, and this is where optimization is coming from?

[dragoncoder047]

but then my saved ref spriteData is outdated now.

No, it won't be. This PR doesn't change anything about repacking!! The texture packer never automatically moves or deletes things after they've been packed. It just defers syncing the texture data to the GPU until it knows you might want to draw the sprite (i.e. calling getSprite directly, and this worked for "named" sprites since the sprite component calls getSprite when you give it a string, but I forgot about directly referencing sprite data, which addKaboom does).

[imaginarny]

It won't be? I didn't say it's repacking itself, I even said why it made me think in the first place and then question what's happening. So idk if I wasn't clear, but why, when I do this, Mark isn't drawn:

loadBean();

onLoad(() => {
    let bean = getSprite("bean");

    loadSprite("mark", "/sprites/mark.png");

    onDraw(() => {
        drawUVQuad({
            tex: bean.tex || bean.data.tex,
            width: 2000,
            height: 2000,
        });
    });
});

He is there in a27, and now another interesting part, he would be there on master too if there was some text component added as well (with the nearest filter obviously), BUT not on this branch, that is another difference.

So I have to call it again for mark to appear, e.g.:

wait(3, () => bean = getSprite("bean"));

Which I asked if it's the resulting optimization in question.

play.kaplayjs.com/?code=eJx1T00Lgk...&version=master

[dragoncoder047]

Well, for something like what you're doing (drawing the ENTIRE texture) it doesn't work, but who does that. If you wanted to use mark, you'd getSprite("mark") in order to have the quad that you can pass to particles() etc.

[imaginarny]

So, this is what I summed up in my reply. So your reply could actually be Yes, as you said. Instead "No, it won't be.". :)

[dragoncoder047]

It's not outdated if you mean it surreptitiously references the wrong quad on the texture, that never changes. The texture itself may be updated later but never in a place that an existing sprite data references.

[imaginarny]

Right, so this is done.

Also, MF has another packer optimization idea that you might be interested in, since we are already discussing here, that could or not end up in a27.1 as well, if you are up for it. Currently, it's the whole texture that is sent on sync to the gpu and since texSubImage2D is used, you could keep track of the union of the rects and sync only the sub image instead and save a lot of bandwidth. E.g., for a 32x32 sprite you are now sending 16.777.216 bytes instead of 4096 bytes.

[dragoncoder047]

Hmm, I just looked at that, and because we're sending an ImageSource to texSubImage2D() we can't use the form with width and height, since that only works with a typedarray and not an <img> or <canvas>. Perhaps there is another way.