-
Sometimes when I release a key, 3 events are fired: KeyReleased, KeyPressed and KeyReleased in this order. I could reproduce the problem on Ubuntu 12.04 LTS, on Windows it doesn't seem to appear. I use the Git revision dd48427.
Small and complete example:
#include <SFML/Window.hpp>
#include <iostream>
int main()
{
sf::Window window(sf::VideoMode(640, 480), "SFML Test");
window.setKeyRepeatEnabled(false);
sf::Clock clock;
int i = 0;
for (;;)
{
sf::Event event;
while (window.pollEvent(event))
{
switch (event.type)
{
case sf::Event::Closed:
return 0;
case sf::Event::KeyPressed:
if (event.key.code == sf::Keyboard::F)
std::cout << ++i << "\tPressed \t"
<< clock.getElapsedTime().asMilliseconds() << std::endl;
break;
case sf::Event::KeyReleased:
if (event.key.code == sf::Keyboard::F)
std::cout << "\tReleased\t"
<< clock.getElapsedTime().asMilliseconds() << std::endl;
break;
}
}
window.display();
}
}
Output (this was luck, sometimes you need really long until the bug appears). You see the problem at the 6th release event.
1 Pressed 3583
Released 4355
2 Pressed 5262
Released 6740
3 Pressed 7244
Released 8220
4 Pressed 8782
Released 9740
5 Pressed 10222
Released 11339
6 Pressed 11781
Released 13100
7 Pressed 13100
Released 13100
8 Pressed 13650
Released 14547
-
Linux (X11) doesn't natively handle the repeated key events the same way as SFML, so I have to use a
ugly hack workaround to get the expected results. I wouldn't be surprised if the corresponding code was not 100% robust, but I can't find where the mistake could be.
In case you want to give it a try, the code is in WindowImplX11.cpp, lines 556 to 593.
-
Linux (X11) doesn't natively handle the repeated key events the same way as SFML
If you mean setKeyRepeatEnabled(), it is not the problem -- I disabled key repetition in my code.
Or should I nevertheless take a look at the lines you wrote?
-
Yes, this piece of code handles everything; it also applies the KeyRepeatEnabled flag, since there's no native X11 function that automatically does it.
-
Okay, I have come to a few results. First, I have realized how much I love Visual Studio, when I once have to work (and debug) with Code::Blocks. I'm really glad I don't have to develop with it...
This is the original code:
// Detect repeated key events
if ((windowEvent.type == KeyPress) || (windowEvent.type == KeyRelease))
{
if (windowEvent.xkey.keycode < 256)
{
// To detect if it is a repeated key event, we check the current state of the key.
// - If the state is "down", KeyReleased events must obviously be discarded.
// - KeyPress events are a little bit harder to handle: they depend on the EnableKeyRepeat state,
// and we need to properly forward the first one.
char keys[32];
XQueryKeymap(m_display, keys);
if (keys[windowEvent.xkey.keycode / 8] & (1 << (windowEvent.xkey.keycode % 8)))
{
// KeyRelease event + key down = repeated event --> discard
if (windowEvent.type == KeyRelease)
{
m_lastKeyReleaseEvent = windowEvent;
return false;
}
// KeyPress event + key repeat disabled + matching KeyRelease event = repeated event --> discard
if ((windowEvent.type == KeyPress) && !m_keyRepeat &&
(m_lastKeyReleaseEvent.xkey.keycode == windowEvent.xkey.keycode) &&
(m_lastKeyReleaseEvent.xkey.time == windowEvent.xkey.time))
{
return false;
}
}
}
}
The debugger showed that duplicate events occur outside the 3rd if statement, that is when the key is not held down. Additionally, I think it would be correct to set m_lastKeyReleaseEvent always to the last release event, not only if it is discarded, but I might be wrong here.
I have tried to adapt your code. According to your comment, the KeyPressed condition is independent of heldDown (variable saying that the key is currently held down). In the KeyReleased condition, I discard the event if the key is down or it's a duplicate (which may occur when the key is already up again). I also assign the last key event always. The body of the 2nd if statement then looks like this:
char keys[32];
XQueryKeymap(m_display, keys);
bool duplicate = (m_lastKeyReleaseEvent.xkey.keycode == windowEvent.xkey.keycode) &&
(m_lastKeyReleaseEvent.xkey.time == windowEvent.xkey.time);
bool heldDown = keys[windowEvent.xkey.keycode / 8] & (1 << (windowEvent.xkey.keycode % 8));
// KeyRelease event + key down = repeated event --> discard
if (windowEvent.type == KeyRelease)
{
m_lastKeyReleaseEvent = windowEvent;
if (heldDown || duplicate)
return false;
}
// KeyPress event + key repeat disabled + matching KeyRelease event = repeated event --> discard
if ((windowEvent.type == KeyPress) && !m_keyRepeat && duplicate)
{
return false;
}
It seems to work so far, but there is one situation where duplicates still occur: If the timestamp is minimally different. It happens sometimes that the time differs by 1 or 2 milliseconds. You could use an inequation for the duplicate expression, just to make code even uglier :D
By the way, you should really split your processEvent() function up :P
Also, the first two if statements in the original code can be merged using operator&&.
-
Ok, I see. I'll rewrite that to include your fix (including the 2 ms lag in timestamps -- SDL does it too so it's not that bad) and clean this function a little bit :P
Thanks a lot for your help.
-
No problem, I'm always glad if I can contribute to bugfixes :)
About the 2ms, I don't know how high the delay may be, it's just the highest one I have seen with a few tests. But SDL should know it ;)
-
Since you already have a test environment ready to work, could you quickly test this new code and tell me if it works as expected?
// Detect repeated key events
if (((windowEvent.type == KeyPress) || (windowEvent.type == KeyRelease)) && (windowEvent.xkey.keycode < 256))
{
// To detect if it is a repeated key event, we check the current state of the key:
// - If the state is "down", KeyReleased events must obviously be discarded
// - KeyPress events are a little bit harder to handle: they depend on the KeyRepeatEnabled state,
// and we need to properly forward the first one
// Check if the key is currently down
char keys[32];
XQueryKeymap(m_display, keys);
bool isDown = keys[windowEvent.xkey.keycode / 8] & (1 << (windowEvent.xkey.keycode % 8));
// Check if it's a duplicate event
bool isDuplicate = (windowEvent.xkey.keycode == m_lastKeyReleaseEvent.xkey.keycode) &&
(windowEvent.xkey.time - m_lastKeyReleaseEvent.xkey.time <= 2);
// KeyRelease event + key down or duplicate event = repeated event --> discard
if ((windowEvent.type == KeyRelease) && (isDown || isDuplicate))
return false;
// KeyPress event + matching KeyRelease event = repeated event --> discard if key repeat is disabled
if ((windowEvent.type == KeyPress) && isDuplicate && !m_keyRepeat)
return false;
// Keep track of the last KeyRelease event
if (windowEvent.type == KeyRelease)
m_lastKeyReleaseEvent = windowEvent;
}
I still don't get how a duplicate KeyRelease event can occur when the key is not down, but if it solves the problem, wonderful ;D
About the 2ms, I don't know how high the delay may be, it's just the highest one I have seen with a few tests. But SDL should know it
SDL uses < 2, so they may not know better than us ;D
-
The code doesn't work; it fires repeated events, when m_keyRepeat is false.
You need to store the last release event also if it is discarded, i.e. reorder the if statements:
// Keep track of the last KeyRelease event
if (windowEvent.type == KeyRelease)
m_lastKeyReleaseEvent = windowEvent;
// KeyRelease event + key down or duplicate event = repeated event --> discard
if ((windowEvent.type == KeyRelease) && (isDown || isDuplicate))
return false;
// KeyPress event + matching KeyRelease event = repeated event --> discard if key repeat is disabled
if ((windowEvent.type == KeyPress) && isDuplicate && !m_keyRepeat)
return false;
This code works as expected. I have however managed to have a delay of 3 or even 4 ms ;D
(very rarely, and that computer is already half a decade old...)
-
Oops! I forgot that the function returned immediately when it found an event to discard.
I have however managed to have a delay of 3 or even 4 ms
Let's use 5 ms then, it is very unlikely (impossible?) that a human can trigger two events that close.
In case you have extra free time (and I know you don't ;D), it would be interesting to test SDL (latest 1.3 sources) on your PC and see if you can break their code with your 4 ms delay.
-
In case you have extra free time (and I know you don't ;D), it would be interesting to test SDL (latest 1.3 sources) on your PC and see if you can break their code with your 4 ms delay.
It would indeed be interesting, but at the moment I really don't have the time, already this took a lot of time (especially setting everything up and debugging on Linux).
Maybe in February ;)