Jump to content
  • Advertisement
Sign in to follow this  
Steve-B

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

This topic is 5092 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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
Advertisement
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
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

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
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
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
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
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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!