makeColorKeyTexture with position bug

You discovered a bug in the engine, and you are sure that it is not a problem of your code? Just post it in here. Please read the bug posting guidelines first.
matgaw
Posts: 45
Joined: Sat Oct 06, 2007 11:33 am

makeColorKeyTexture with position bug

Post by matgaw »

In SVN 1981 and SVN2010 there's a bug with makeColorKeyTexture. Example:

Code: Select all

bool makeTransparent=false;
    if (!device->getVideoDriver()->findTexture(file.c_str()))
        makeTransparent=true;
Texture=device->getVideoDriver()->getTexture(file.c_str());

if (makeTransparent)
    device->getVideoDriver()->makeColorKeyTexture(Texture,position2d<s32>(0,0));
This code stopped working with Irrlicht SVN 1981 and 2010 (probably 1.5 too). In 1.4.2 there's no bug.

After running this code, everything works fine but calling it second time makes also all RGB(0,0,0) pixels transparent too! (pixel on pos(0,0) is RGB(0,252,252) so far away from it)

This code always called makeColorKeyTexture only once, but it seems that it stopped working with Irrlicht 1.5 (something changed in findTexture?).
rogerborg
Admin
Posts: 3590
Joined: Mon Oct 09, 2006 9:36 am
Location: Scotland - gonnae no slag aff mah Engleesh
Contact:

Post by rogerborg »

Thanks for the report. I can replicate this, and am investigating it now.
Please upload candidate patches to the tracker.
Need help now? IRC to #irrlicht on irc.freenode.net
How To Ask Questions The Smart Way
rogerborg
Admin
Posts: 3590
Joined: Mon Oct 09, 2006 9:36 am
Location: Scotland - gonnae no slag aff mah Engleesh
Contact:

Post by rogerborg »

That was a great catch, and a fun one to fix. Instead of just setting the alpha component to 0, we were setting the whole colour of matching texels to 0, so the second call was keying against a colour of 0.

We're now just setting the alpha for matching texels. That should be fixed on the 1.5 branch in SVN 2011, and it will get merged down to the trunk later.
Please upload candidate patches to the tracker.
Need help now? IRC to #irrlicht on irc.freenode.net
How To Ask Questions The Smart Way
vitek
Bug Slayer
Posts: 3919
Joined: Mon Jan 16, 2006 10:52 am
Location: Corvallis, OR

Post by vitek »

It seems that this would break existing behavior for code using makeColorKeyTexture() and scene nodes with the material type EMT_TRANSPARENT_ADD_COLOR. Does it not? Might be worth noting the change in the readme.

Travis
rogerborg
Admin
Posts: 3590
Joined: Mon Oct 09, 2006 9:36 am
Location: Scotland - gonnae no slag aff mah Engleesh
Contact:

Post by rogerborg »

Spoilsport. :P

I'll have a look into that, but I'm leaning against reverting makeColorKeyTexture() back to its previous behaviour. Actually setting texel colours to 0 is (at least should be) very unexpected behaviour; there's nothing in the API that implies it.
Please upload candidate patches to the tracker.
Need help now? IRC to #irrlicht on irc.freenode.net
How To Ask Questions The Smart Way
vitek
Bug Slayer
Posts: 3919
Joined: Mon Jan 16, 2006 10:52 am
Location: Corvallis, OR

Post by vitek »

I'm definitely not suggesting you revert to the incorrect behavior. If it is a breaking change for those users, a simple note in the readme should be sufficient.

Travis
rogerborg
Admin
Posts: 3590
Joined: Mon Oct 09, 2006 9:36 am
Location: Scotland - gonnae no slag aff mah Engleesh
Contact:

Post by rogerborg »

Hmm, I'll tell you what it does break: 16 bit software blending (e.g. example 06 with the software driver), which seems to rely on the colour being 0. I'm investigating the reason for that now. Anyone who wants to join the party might start with PixelBlend16_simd().

In the meantime, SVN 2013 adds an optional parameter to makeColorKeyTexture() to use the old behaviour. changes.txt has also been updated.

[Updated]
OK, I'm baffled. PixelBlend16_simd() is only used from executeBlit_TextureBlend_16_to_16(), which is explicitly used for A1R5G5B5 source and destination pixels. It should therefore deal with alpha.

However, it doesn't appear to do so. We can probably ignore the alpha of the destination, but if the alpha of a source pixel is zero, I'd expect it to be ignored rather than blended. At the moment, we do mask out the alpha bits, but don't appear to use them as binary flags. Instead, there's... maths. ;)

I'm loathe to dive in and change such a fundamental (and performance critical) function, but it really doesn't do what I'd expect it to do. Any ideas?
Please upload candidate patches to the tracker.
Need help now? IRC to #irrlicht on irc.freenode.net
How To Ask Questions The Smart Way
Dorth
Posts: 931
Joined: Sat May 26, 2007 11:03 pm

Post by Dorth »

Make it do what's it supposed to do and (maybe) put an optional way for the old behavior like you did? I like this strategy ^^
rogerborg
Admin
Posts: 3590
Joined: Mon Oct 09, 2006 9:36 am
Location: Scotland - gonnae no slag aff mah Engleesh
Contact:

Post by rogerborg »

Thanks, I think I will go with that. However, this 16 bit blending is really puzzling me. Here's the code in question. My description of what's going on is in //comments

Code: Select all

// 1 - Bit Alpha Blending
inline u16 PixelBlend16 ( const u16 c2, const u16 c1 )
{
	// c1 is the "source" pixel that we're "blending" with c2, the destination
	u16 c = c1 & 0x8000; // Extract the alpha bit in the source. It's either 0 (fully transparent) or 1 (fully opaque).
	
	c >>= 15; // Move it down to the low bit
	c += 0x7fff; // c is now either 0x8000 (if the source has an alpha bit) or 0x7fff (if it doesn't).
	
	c &= c2; // So that's either 0x8000 & c2 (ignore the destination colour) or 0x7fff & c2 (use all of the destination colour)
	c |= c1; // But then we add all of the source colour, even if its alpha is 0!
	
	return c;
}
It's that last c |= c1 that confuses me. If the source pixel has an alpha bit, i.e. is solid, then we have ignored the destination and we're using only the source. Fair enough.

But if the source has alpha 0, i.e. it should be fully transparent, instead of ignoring it, we still use its colour. That's why boolean alpha keying only works in 16 bit if the whole colour has been set to 0, not just the alpha.

Pop quiz: what's wrong with this (logically, I haven't looked into the efficiency of the compiled instructions)?

Code: Select all

inline u16 PixelBlend16 ( const u16 destination, const u16 source )
{
	if(source & 0x8000 == 0x8000)
		return source; // The source is visible, so use it.
	else
		return destination; // The source is transparent, so use the destination.
}

I'll try it now, of course, but the Flying Spaghetti Monster Herself knows what else that'll break.
Please upload candidate patches to the tracker.
Need help now? IRC to #irrlicht on irc.freenode.net
How To Ask Questions The Smart Way
hybrid
Admin
Posts: 14143
Joined: Wed Apr 19, 2006 9:20 pm
Location: Oldenburg(Oldb), Germany
Contact:

Post by hybrid »

We should get the functionality changing fixes into trunk, though, instead of fixing a bug for one person and breaking the code of all the others. So I'd favor such changes in the 1.5 branch if we keep backwards compatible or if there's no useful behavior at all.
rogerborg
Admin
Posts: 3590
Joined: Mon Oct 09, 2006 9:36 am
Location: Scotland - gonnae no slag aff mah Engleesh
Contact:

Post by rogerborg »

Given the creeping scope of the changes here, I'm inclined to agree. I wouldn't have started addressing this on 1.5 if I'd known that it was going to snowball from what looked like a simple isolated bugfix. :(

For consistency, I've gone ahead and committed the above change to PixelBlend16() / PixelBlend16_simd() on 1.5 in SVN 2014. That appears to work, for the limited amount of testing that I've done.

However, I'd suggest that we merge 2011 - 2014 down to the trunk, then revert 1.5 back to SVN 2010 and just eat the problem with makeColorKeyTexture() on that branch.

I'll hold off on doing that until you've had a chance to look at it. If you're OK with us reverting 1.5 back to 2010, I'd suggest just going ahead and doing it.
Please upload candidate patches to the tracker.
Need help now? IRC to #irrlicht on irc.freenode.net
How To Ask Questions The Smart Way
hybrid
Admin
Posts: 14143
Joined: Wed Apr 19, 2006 9:20 pm
Location: Oldenburg(Oldb), Germany
Contact:

Post by hybrid »

I'll merge later today, then we can do whatever seems best for the branches. But I also think that reverting this in the branch is best, because the color key methods were always weird, but heavily used. And fixing the default behavior to something we cannot really talk of as consistent is also not really desireable.
But we might consider adding alpha map creation methods which create images with the mentioned features, such as preparing alpha values for ADD_COLOR. Everyone, please add requests to the feature request trackers 8)
vitek
Bug Slayer
Posts: 3919
Joined: Mon Jan 16, 2006 10:52 am
Location: Corvallis, OR

Post by vitek »

rogerborg wrote:

Code: Select all

inline u16 PixelBlend16 ( const u16 destination, const u16 source )
{
	if(source & 0x8000 == 0x8000)
		return source; // The source is visible, so use it.
	else
		return destination; // The source is transparent, so use the destination.
}
That seems right. You could avoid checking the result of the mask as the bit is either set or it isn't. I'm sure the burning video guy (Thomas?) will want to avoid the conditional. He's already got an example of how to do a conditional without the pipeline stall (see if_c_a_else_b() in irrMath.h). I'm not certain of the performance benefits, but I'd venture to guess they are pretty significant given that this function is potentially called thousands of times per frame.

Anyways, here is the above code reworked to use the trick...

Code: Select all

u16 pixel_blend_16 (u16 dst, u16 src)
{
  // return src & 0x8000 ? src : dst;
  return ( ( -(src & 0x8000) >> 15 ) & ( src ^ dst ) ) ^ dst;
}
Travis
hybrid
Admin
Posts: 14143
Joined: Wed Apr 19, 2006 9:20 pm
Location: Oldenburg(Oldb), Germany
Contact:

Post by hybrid »

Ok, I've reverted the changes in the 1.5 branch.
We should also concentrate on the initial problem now, which is a problem with the naming changes. Seems like findTexture doesn't honor the full path names. This looks like a necessary change for the 1.5 branch :)
BTW: I've now also checked the code and your changes. And I think that the method was solely developed for the font preparation. IIRC the old font tool even requires this "blackening". But in Irrlicht 1.6 we should be able to make the method behave more logical and useful for the common case as default.
burningreggae
Posts: 66
Joined: Wed Oct 04, 2006 2:07 pm

Post by burningreggae »

yeah guys!. pixelblend16 can be redone.

hybrid is right. older version needed the complete color 0, not just the alpha. i remember this...

and vitek is right. i don't like conditional branches;-) and will never use it it my code..


Also Destination alpha is supported now since 1.6 and should be in the 16 bit software drivers.
i will have a look on this.
burningreggae
Post Reply