Sign in to follow this  
Steve-B

I wonder why this small piece of code fails to work??

Recommended Posts

Steve-B    150
Hi I've written a piece of code which shifts some RGB colour values four bits to the left. First it stores a backup of each RGB value just incase the shift results in a zero. If a zero is produced then the original values are restored. Here is that piece of code:
redStore = scratchPalette.red;		// Save a backup of the Red component
greenStore = scratchPalette.green;	// Save a backup of the Green component
blueStore = scratchPalette.blue;	// Save a backup of the Blue component

scratchPalette.red <<= 4;		// Shift the Red value 4-bits to the left
scratchPalette.green <<= 4;		// Shift the Green value 4-bits to the left
scratchPalette.blue <<= 4;		// Shift the Blue value 4-bits to the left

if((scratchPalette.red) == 0) { scratchPalette.red = redStore; }
if((scratchPalette.green) == 0) { scratchPalette.green = greenStore; }
if((scratchPalette.blue) == 0) { scratchPalette.blue = blueStore; }


As you can see it's a little long winded so I thought I could make it a bit smaller and a bit more optimised by changing the code to this:
if((scratchPalette.red << 4) != 0) { scratchPalette.red <<= 4; }
if((scratchPalette.green << 4) != 0) { scratchPalette.green <<= 4; }
if((scratchPalette.blue << 4) != 0) { scratchPalette.blue <<= 4; }


Well of course, it doesn't want to work. Instead of seeing what value scratchPalette.red green and blue would have had if they had been shifted all the program sees is the value they already hold. Is there any way of fixing this?? Steve-B

Share this post


Link to post
Share on other sites
Guest Anonymous Poster   
Guest Anonymous Poster
Take the brackets off inside the if's, you are doing a boolean type compare with the brackets there.

Share this post


Link to post
Share on other sites
if((scratchPalette.red << 4) != 0) { scratchPalette.red <<= 4; }

it isn't optimization, it's deoptimization. Instead of one bit shift you are performing one or two bit shifts and conditional evaluation

Share this post


Link to post
Share on other sites
Guest Anonymous Poster   
Guest Anonymous Poster
What is the type of the color channel components?
In 'scratchPalette.red << 4', the cannel will be casted to int/uint before and then shifted. If your cannel components are of shorter type (like short or unsigned char) the results will differ.
Try (example in case of chars):
if(((scratchPalette.red << 4) & 0xff) != 0) { scratchPalette.red <<= 4; }
what actually means:
if ((scratchPalette.red & 0xf) != 0) { scratchPalette.red <<= 4; }

Share this post


Link to post
Share on other sites
schue    176

A value can only be 0 after shifting 4 bits to left if all but the 4 leftmost bits are 0, too. So assuming byte datatype, you could make it much simplier like so:


byte red;

if( (red & 0x0f ) != 0 ) red <<= 4;

// or for int (assuming int = 4 byte)

int green;

if( (green & 0x0fffffff) != 0 ) green <<= 4;



I am not sure if it is safe to assume that shifting by 4 is always faster than multiplying by 16.

Share this post


Link to post
Share on other sites
Michael K    122
In the first piece of code, you're checking to see if scratchPalette.red << 4, etc. is equal to zero. In the second piece of code, you're checking to see if it's *not* equal to zero. This is why you're getting different results for the two pieces of code.

Share this post


Link to post
Share on other sites
Steve-B    150
Quote:
Original post by Anonymous Poster
What is the type of the color channel components?


The type of each compontent is 8-bit unsigned char:

typedef struct
{
unsigned char red : 8;
unsigned char green : 8;
unsigned char blue : 8;
}scratchPalette;

Quote:
Original post by tomek_zielinski
it isn't optimization, it's deoptimization. Instead of one bit shift you are performing one or two bit shifts and conditional evaluation

Interesting. Yes I didn't think of it like that. Maybe the old way is the more optimised way. :)

Quote:
Original post by Michael K
In the first piece of code, you're checking to see if scratchPalette.red << 4, etc. is equal to zero. In the second piece of code, you're checking to see if it's *not* equal to zero. This is why you're getting different results for the two pieces of code.

No it's not. Read the code again. :P

Share this post


Link to post
Share on other sites
Steve-B    150
Quote:
Original post by schue
A value can only be 0 after shifting 4 bits to left if all but the 4 leftmost bits are 0, too. So assuming byte datatype, you could make it much simplier like so:

Yes very clever, I didn't think of like that. :)
Nice one schue!
This looks like maybe a better way of doing things. I'll give it a try and see how it goes. Thanks everyone for all your help and advice.

Share this post


Link to post
Share on other sites
iMalc    2466
Quote:
Original post by Anonymous Poster
Take the brackets off inside the if's, you are doing a boolean type compare with the brackets there.
Utter rubbish!

Correct Explanation:
"scratchPalette.red <<= 4" puts the result back into a byte which chops the high nibble off making the number zero at times.

However this "if((scratchPalette.red << 4) != 0)" actually stores a temporary result of "scratchPalette.red << 4" in an int, hence the high nibble of the byte is never discarded.

Use the solution schue mentioned.
"if(static_cast<unsigned char>(scratchPalette.red << 4) != 0)"
would also work though.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this