Sign in to follow this  
Erius

[C++] Need help on optimizing a function

Recommended Posts

Hi. There is a function in my mouse input handler that has always bothered me, and now that the whole manager is done, I want to optimize it a bit.
	if (input->header.dwType == RIM_TYPEMOUSE) 
	{


		// left mouse button
		if(input->data.mouse.usButtonFlags & RI_MOUSE_LEFT_BUTTON_DOWN)
		{ 
			in_event.button = 1 | mouse_offset;
			in_event.buttonstate = 1;
			update(in_event);
		}
		else if(input->data.mouse.usButtonFlags & RI_MOUSE_LEFT_BUTTON_UP)
		{  
			in_event.button = 1 | mouse_offset;
			in_event.buttonstate = 0;
			update(in_event);
		}

		// Middle mouse button
		if(input->data.mouse.usButtonFlags & RI_MOUSE_MIDDLE_BUTTON_DOWN)
		{
			in_event.button = 2 | mouse_offset;
			in_event.buttonstate = 1;
			update(in_event);
		}
		else if(input->data.mouse.usButtonFlags & RI_MOUSE_MIDDLE_BUTTON_UP)

		{
			in_event.button = 2 | mouse_offset;
			in_event.buttonstate = 0;
			update(in_event);
		}


		// Right mouse button
		if(input->data.mouse.usButtonFlags & RI_MOUSE_RIGHT_BUTTON_DOWN)
		{  
			in_event.button = 3 | mouse_offset;
			in_event.buttonstate = 1;
			update(in_event);
		}
		else if(input->data.mouse.usButtonFlags & RI_MOUSE_RIGHT_BUTTON_UP)
		{  
			in_event.button = 3 | mouse_offset;
			in_event.buttonstate = 0;
			update(in_event);

		}
		// Mouse movement
		if(input->data.mouse.lLastX)
		{
			in_event.mouse_x = input->data.mouse.lLastX;
		}
		if(input->data.mouse.lLastY)
		{
			in_event.mouse_y = input->data.mouse.lLastY;
		}
		if (in_event.mouse_x || in_event.mouse_y)
		{
			update(in_event);
		}

		// Mouse button 4
		if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_4_DOWN)
		{	
			in_event.button = 4 | mouse_offset;
			in_event.buttonstate = 1;
			update(in_event);
		}
		else if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_4_UP)
		{	in_event.button = 4 | mouse_offset;
		in_event.buttonstate = 0;
		update(in_event);
		}

		// Mouse button 5
		if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_5_DOWN)
		{	in_event.button = 5 | mouse_offset;
		in_event.buttonstate = 1;
		update(in_event);
		}
		else if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_5_UP)
		{	in_event.button = 5 | mouse_offset;
		in_event.buttonstate = 0;
		update(in_event);

		}

		if(input->data.mouse.usButtonFlags & RI_MOUSE_WHEEL)
		{
			HandleMouseWheel(input);
		}
		return;
	
	} 



It reeks of some nasty things, like code duplication and spaghetti else-if-heimer, however I can't really think of too many optimizations, since the class is setting values depending on other values, and it has to be reasonably quick, so I'm not sure if a generic way of doing it is possible, but who knows? Profiling showed that in the current state, the function is fast enough, but I still don't like my implementation, and I want to improve my style. So, can I rewrite this to reduce code duplication, while keeping the amount of calls as low as possible? Edit: What I've been thinking of so far: -Those "offset" calculations will be replaced by #defines -more returns -extracting each section into a respective inline function for readability -Rearrange more stuff, but that's the vague part :P

Share this post


Link to post
Share on other sites
For starters, performance isn't a concern in a function that's called once per frame.

About improving duplicate code, you can
a) refactor common stuff into a function
b) use a table for the data (bitmasks etc.) of the different mouse buttons and loop through it

Share this post


Link to post
Share on other sites
Thank you for the reply.
Of course, a loop makes sense, seeing how the tests are pretty much done sequentially, d'oh, I'm blind!

And well, I suppose a one frame function can slack a little...hm.

Cheers~!

Share this post


Link to post
Share on other sites
At the very least, you can turn all those sections for different buttons into a loop, by using an auxiliary array that contains what you need to know about each button. Your code would then look something like this:
    if (input->header.dwType == RIM_TYPEMOUSE) 
{
// Buttons
for (int b = 0; b < number_of_buttons; ++b)
{
ButtonDescription const &bd = button_description[b];
if (input->data.mouse.usButtonFlags & bd.button_down)
{
in_event.button = bd.code | mouse_offset;
in_event.buttonstate = 1;
update(in_event);
}
else if(input->data.mouse.usButtonFlags & bd.button_up)
{
in_event.button = bd.code | mouse_offset;
in_event.buttonstate = 0;
update(in_event);
}
}

// Mouse movement
if(input->data.mouse.lLastX)
{
in_event.mouse_x = input->data.mouse.lLastX;
}
if(input->data.mouse.lLastY)
{
in_event.mouse_y = input->data.mouse.lLastY;
}
if (in_event.mouse_x || in_event.mouse_y)
{
update(in_event);
}

// Wheel
if(input->data.mouse.usButtonFlags & RI_MOUSE_WHEEL)
{
HandleMouseWheel(input);
}
return;

}




Also, you shouldn't avoid function calls in general. Performance is not as bad as you think (not bad at all if they can be inlined), and most of the time performance is not a good reason to not do something that makes your code more clear anyway.

Stay away from macros (#define).

Share this post


Link to post
Share on other sites
Thanks for the reply, yeah, I turned the spaghetti into a loop and the values in a table.


DWORD button = input->data.mouse.usButtonFlags;
for (int i = 0; i < 10; ++i )
{
in_event.buttonstate = !in_event.buttonstate;
if (button & mousestates[i])
{
in_event.button = mousebuttons[i];
update(in_event);
}
}



Looks like this, now.

Since the button down and up checks were done in an alternating fashion, I'm letting the buttonstate alternate as well, should work as intended, I hope. Hehe.

And again, thanks for the pointers about performance concerns, I suppose it's because I'm learning it mainly on my own, and thus I am usually considering the worst case first :P.

Oh, and I pretty much never use self made macros of any kind, but I figured naming the constant while experimenting would make things more manageable. Hehe.

Well, cheers!

Share this post


Link to post
Share on other sites
Quote:
Original post by Erius
And again, thanks for the pointers about performance concerns, I suppose it's because I'm learning it mainly on my own, and thus I am usually considering the worst case first :P.

Yeah, premature optimization can make one paranoid. I'm guilty of that as well (and micro-optimization is a habit that's hard to get rid of).

Share this post


Link to post
Share on other sites
Writing less code is usually a good idea, and in most cases it will even improve performance. You get good results if you split your code up into functions, but reference the same function as few times as possible. Your revised code calls update(in_event) in a loop, and it will be called just as many times as before, but now the compiler only needs to produce one copy of the register push instructions, so your object code is smaller. And the benefit is magnified if the compiler decides to inline the update function.

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