[BUG+FIX] Blitting wrong alpha values (Burnings and IImage)

[chronologicaldot]

@burningreggae - I'd certainly like that new channel. But what would it be named? Maybe the blitter operation could be labeled BLEND_TEXTURE_COLOR_COMBINE_ALPHA.

[chronologicaldot]

If we make a new pixel blending operation a second function, we could name it "PixelCombine32".
The IImage function to access it could be called "copyToCombineAlpha", so we can leave the old "copyToWithAlpha" alone.

I'd be happy to implement what's necessary to get this functionality and then send post the .diff.

[CuteAlien]

@chronologicaldot: So you have one blitter function which works for you? I'm still unclear on that.
If you have - which patch would that one exactly need?
And then - yeah - I guess we can make a new blitter function - if "combine" makes sense - sure. But have to check then if it's ok to implement new blitter operations only for one color format (32-bit). On first view again it looks like that might be fine. And adding a parameter to copyToWithAlpha should be enough - at that place an additional check to select another blitter shouldn't matter.

edit (yeah again...): Should probably be BLITTER_TEXTURE_ALPHA_COMBINE and/or BLITTER_TEXTURE_ALPHA_COLOR_COMBINE. As copyToWithAlpha seems to use the BLITTER_TEXTURE_ALPHA_BLEND and BLITTER_TEXTURE_ALPHA_COLOR_BLEND.

[chronologicaldot]

@cutealien - The blitter function that works for me is the one whereby it ends with:

Code: Select all

 
 u32 sa = c1 >> 24;
 u32 da = c2 >> 24;
 // Commented out because it's done earlier in the function:
 //alpha = sa + ( sa >> 7 ); // stretch [0;255] to [0;256] to avoid division by 255. use division 256 == shr 8
 u32 blendAlpha_fix8 = (sa*256 + da*(256-alpha))>>8;
 return blendAlpha_fix8 << 24 | rb | xg;
 

When I tested this version, I got the result I wanted. However, I only had one visual test, so I want to perform more tests to make sure it is correct. Admittedly, this version of PixelBlend32 isn't in accordance with the true corresponding OpenGL version, which is what burningreggae was saying. That's why, if we wanted to keep SoftwareDriver2/Burnings as being the same as OpenGL, then we would leave the current implementation of PixelBlend32 alone and just create a new, separate function chain for combining alpha values the way I would prefer.

If we want to put it in copyToWithAlpha, that's fine. We could call the parameter "bool combineAlpha" and set it default to "false" for backward compatibility.

If you want me to implement this alpha-combining functionality for more color types, I can try to do that, but it'll take me longer to create a patch.

[CuteAlien]

I'd be glad to get a patch. And "bool combineAlpha" would also be fine by me. Taking longer is no problem, you are the person who needs that right now - so unless you run out of time you have zero pressure. Take all the time you need :-)

edit (just because edits are fun now ^_^): If you know a concrete algorithm name for this (like this kind of handling alpha's is know as the chronologicaldotAlphaCombine in literature) it's even better than using a generic name like combine. But if not - "combineAlpha" and quick comment that this parameter flips between using source-alpha and combining source and destination alpha is also OK.

[chronologicaldot]

Given what it says here: https://en.wikipedia.org/wiki/Painter%27s_algorithm
I suppose we could call it "paintAlpha", "painterAlpha", or "paintersAlpha". Any one of these would be sufficient I suppose. Do you think this is descriptive enough?

[CuteAlien]

Hm, don't think it fits for this. Let's stay with combineAlpha.

[chronologicaldot]

Ok.

[chronologicaldot]

I added the "combine alpha" functionality and made it accessible via IImage::copyToWithAlpha(). I didn't do it for the software drivers because that would require adding the parameter bool combineAlpha to the draw2DImage function AND maybe adding a listing to the video driver features enum, and I didn't know what to call it.

Turned out to be easier than I expected to implement the "combine alpha" functionality. A number of existing functions were used when no alpha was available in the source image.
Here are the diff files:
CBlit.h - https://pastebin.com/J0ruYqY0
IImage.h - https://pastebin.com/zYBsMUg2
CImage.h - https://pastebin.com/YPdSL48d
CImage.cpp - https://pastebin.com/M32beLu2
SoftwareDriver2_helper.h - https://pastebin.com/GAmsVA6z

I didn't have any 16-bit images on hand to test the 16-bit version, so no guarantees. I hope the simplicity of their alpha channels made it less likely for me to have messed them up.

[CuteAlien]

Oh wow, you did 16-bit formats as well? Nice work! On which Irrlicht version is the diff based - current Irrlicht trunk? (thought probably not much has changed anyway in those files between 1.8 and trunk ...)

[CuteAlien]

Patch looks good, applied to svn trunk r5590. I hope it works out like that, otherwise you can just send more patches (before Irrlicht 1.9 release it's no problem changing it further). Thanks!

[chronologicaldot]

For confirmation, yes, it was the current trunk.

[CuteAlien]

Reworked the patch a bit more in r5998 as there was a compile warning in executeBlit_TextureCombineColor_16_to_16 and looking at it I think we really lost a byte there in the conversion.

[chronologicaldot]

Thanks. I'll have to check out what you did. It's been quite awhile since I've looked at that code.