Strange ~CS() / adjust_surface_color() implementation quirk

[irydacea]

While working on Wespal and trying to figure out why a unit test of mine was failing for no obvious reason using Wesnoth's --render-image output as a reference, I found out that the Wesnoth 1.18 implementation of the ~CS() image path function, sdl::adjust_surface_color() function, has a surprising quirk regarding the way it shifts individual pixels.

Namely, it checks for the alpha component and if the alpha component is zero (fully transparent), it leaves pixels entirely unchanged:

uint32_t* beg = lock.pixels();
uint32_t* end = beg + nsurf->w*surf->h;

while(beg != end) {
	uint8_t alpha = (*beg) >> 24;

	if(alpha) {
		uint8_t r, g, b;
		r = (*beg) >> 16;
		g = (*beg) >> 8;
		b = (*beg) >> 0;

		r = std::max<int>(0,std::min<int>(255,static_cast<int>(r)+red));
		g = std::max<int>(0,std::min<int>(255,static_cast<int>(g)+green));
		b = std::max<int>(0,std::min<int>(255,static_cast<int>(b)+blue));

		*beg = (alpha << 24) + (r << 16) + (g << 8) + b;
	}

	++beg;
}

I'm not sure if this is intentional or useful, or why it was made this way. It looks rather deliberate, even though it's not documented at all currently.

The problem with this approach is that if the alpha channel is modified by another image path function following ~CS(), the unmodified pixels will be revealed. Obviously this can be avoided by having ~CS() follow any alpha-modifying functions, but this is a non-obvious gotcha that doesn't make a lot of sense even if it were documented in the wiki.

[Vultraz]

I imagine “fixing” this would just be removing the check?

[irydacea]

I'm not sure why you're using quotation marks, but yes.

[gfgtdf]

git blame gives b4d40ef, so it seems like it was indeed intended as an optisation for images with a lot of trasparent space.

[irydacea]

I guess that made sense back then because there was no way to edit the alpha channel and no way to write images to disk either.