(fix) Set bpp for WithMask

[ahyangyi]

The current implementation breaks AlternativeSprites.get_sprite().

I am not sure if simply copying the bpp is the best design, but seeing that the mask is always 8bpp and is not what we usually refer to as the actual sprite bpp, I feel this might be justified?

Anyways, I feel I’m more like asking a question than proposing a change, but this PR is what I came up with.

[ldpl]

Well, it's a good question. I guess main issue here is that bpp actually means many slightly different things. 1) Image file bpp. If sprite bpp is None it will take bpp of the image file. 2) Forced conversion bpp. If sprite bpp is set and doesn't match file bpp loaded image will be converted to the required bpp. 3) Some mark to distinguish sprites in the AlternativeSprites. It's purely for external use as grf-py doesn't need that distinction. 4) GRF sprite bpp. GRF format works with 3 layers (RGB, A, M), not bpp per se. And it's ambiguous at this point whether 32bpp means RGBA or RGBM. Or is 16bpp (AM layers) even a thing.

So as 2 is the only bpp that actually does something and in all other cases bpp isn't even known until the image is loaded I'm thinking that maybe bpp shouldn't even be a property of the Sprite and should be moved to FileSprite. And 3 just needs some other solution. Perhaps replace get_sprite method of AlternativeSprites with get_image with the same parameters and let it navigate individual sprites on its own?

[ahyangyi]

Well, I would argue that bpp isn’t purely for external use in AlternativeSprites. The following assertion currently doesn’t completely work as intended either:

        assert all(isinstance(s, Sprite) for s in sprites), sprites
        if len(set((s.zoom, s.bpp) for s in sprites)) != len(sprites):
            sprite_list = ", ".join(f'{s.name}<zoom={s.zoom}, bpp={s.bpp}>' for s in sprites)
            raise ValueError(f'AlternativeSprites need to have different zooms or bpp: {sprite_list}')

And I would consider the above assertion an essential part of AlternativeSprites, made necessary by the GRF spec.

So, accessing the bpp from AlternativeSprites has both internal and external uses...?

[ldpl]

Hm, yeah... But that can and probably should be moved to the sprite writing stage where bpp is actually known.

[ldpl]

Oh, and bpp is also used in sprite fingerprint for caching. Though maybe it can be removed from it as even FileSprite will either have different position or image.

[ahyangyi]

I'm thinking about it this way: everything else wants to not care about image bpp until necessary to improve speed, but the hypothetical user who creates an AlternativeSprites then attempts to call get_sprite to get one sprite out of the multiple they provided wants to care. But they really already know which sprite is which, just don't have a means to store their own metadata inside AlternativeSprites.

So, what if we allow passing a dictionary into AlternativeSprites and change get_sprite to a simple lookup?

[ldpl]

I've been experimenting with this thing I called SpriteCollection recently: https://github.com/lukaskrepel/BonkyGFX/blob/78396764197128db06be08f9f095fa10874dd6ab/lib.py#L115

It's similar to AlternativeSprites but allows to add custom keywords to sprites. And then you can do operations on whole groups. I.e. you can have two variations of ground sprites - with and without grid lines. Then you compose water overlay on top of it and get two variations of shore tiles, with all the zooms and bpp variations. It's still highly experimental as I need to figure out a good logic for how collection operations should work and make it more generic. But this may actually be the future of AlternativeSprites.

[ldpl]

Looking at this PR again and while the question still remains the change doesn't really hurt anything so I'll juts merge it for now. But it's all likely to change it the future to better represent different kinds of bpp.