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;
}
[C++] Need help on optimizing a function
Author
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.
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
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
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
Author
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~!
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~!
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:
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).
if (input->header.dwType == RIM_TYPEMOUSE) { // Buttons for (int b = 0; b < number_of_buttons; ++b) { ButtonDescription const &bd = button_description; 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).
Author
Thanks for the reply, yeah, I turned the spaghetti into a loop and the values in a table.
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!
DWORD button = input->data.mouse.usButtonFlags;for (int i = 0; i < 10; ++i ) { in_event.buttonstate = !in_event.buttonstate; if (button & mousestates) { in_event.button = mousebuttons; 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!
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).
Naming constants is perfectly fine, but that should be done using `const', not `#define'.
unsigned const mouse_offset = ...
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.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
