Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move buttons are queuing in the keyboard buffer #3948

Closed
raindropworks opened this issue Oct 29, 2013 · 13 comments · Fixed by #4006
Closed

Move buttons are queuing in the keyboard buffer #3948

raindropworks opened this issue Oct 29, 2013 · 13 comments · Fixed by #4006

Comments

@raindropworks
Copy link
Contributor

If I'm in a high computer lag situation or moving a long distance, I have a tendency to hold down the move button to go from place to place. With the recent patches, it's now buffering every single move at the keyboard repeat rate instead of checking for the move at completion of the last move. This means that when i let go of the arrow, I could still move 10, 20, even 50 tiles away from where I want to be. Very annoying and has gotten me in trouble with hostile mobs.

@Falconne
Copy link
Contributor

What type of build does this happen in (SDL or ncurses) and what OS?

@axujen
Copy link
Contributor

axujen commented Oct 29, 2013

It happens to me too.
Linux ncurses 0.8-2054-g0eaaafd

@kevingranade
Copy link
Member

Would this happen to only occur when it's raining? I noticed this just
recently, but the obvious fix causes massive breakage on windows for some
reason.

@axujen
Copy link
Contributor

axujen commented Oct 29, 2013

@kevingranade Yes, more specifically when rain animation is displaying. Disabling the animation causes it to disappear.

@Falconne
Copy link
Contributor

I think I know why it happens, I noticed a potential issue with input polling during animations when I was adding mouse support. It only affects ncurses.

I can do a fix, if someone on Linux who compiles the game themselves (and has this problem) can test it to confirm it resolves the issue.

@raindropworks
Copy link
Contributor Author

I believe you and I spoke before @Falconne ... if you can link a PR for it, and then I guess i would compile with all defaults, i can test it

@kevingranade
Copy link
Member

The fix is relatively simple, I did it as #3796
Unfortunately as I said, this breaks input handling on windows horribly.
We need someone that knows their way around a debugger to test that patch
on windows and figure out why it's so broken. Alternately if anyone sees
what the issue is by code inspection, please make a PR that addresses both
issues.

On Wed, Oct 30, 2013 at 10:38 AM, minakitty notifications@github.comwrote:

I believe you and I spoke before @Falconne https://github.com/Falconne... if you can link a PR for it, and then I guess i would compile with all
defaults, i can test it


Reply to this email directly or view it on GitHubhttps://github.com//issues/3948#issuecomment-27400967
.

@Falconne
Copy link
Contributor

The problem is that the input polling for curses in Linux works differently to the SDL and wincurses polling. In the latter two cases, queued messages are always discarded (due to SDL_PollEvent and PeekMessage looping until the event queue is empty and only caring about the last entry). The Linux curses codepath only discards queued events if you call input(). However that function can't handle timeouts, so currently when there's rain animation going on the game calls getch(), which handles timeouts but doesn't discard queued events (when using curses). That's why everything is fine till it rains. And changing timeouts in game.cpp may fix one platform but break another.

The solution is to make Linux curses input always discard queued events, even when animation timeouts are involved. That way all 3 input methods work the same way (and it's the way that makes sense). I'll fix that as part of #3883... already in the process of fixing it there because I need timeouts to work properly to handle mouse move events. Also, that PR pulls in CIB's input refactor, so it makes sense to fix it there. Otherwise the work will have to be done twice and there'll be conflicts.

@kevingranade
Copy link
Member

If the immediate fix is at all straightforward, we need to just deal with
the confliccts, because that refactor is a bit big to land before the
release, but I'd dearly like to have this fixed for it.

@Falconne
Copy link
Contributor

Ok I should be able to come up with a small fix just for the main game loop to prevent input queuing when it's raining. Just need to have a good think about it to make sure the change is safe. Also someone like @minakitty will have to confirm the fix, as even on my Linux VM running a debug build, I don't get enough lag to really notice.

@Abalieno
Copy link
Contributor

Err, can't we get things fixed in tile mode like the (V)iew menu before the next release? These things should have the priority...

@Falconne
Copy link
Contributor

Well the fix is trivial anyway, if it works. I just submitted a PR, if someone can test it.

@kevingranade
Copy link
Member

Please don't lobby in unrelated issues for changes you want.
On Oct 31, 2013 2:55 AM, "Abalieno" notifications@github.com wrote:

Err, can't we get things fixed in tile mode like the (V)iew menu before
the next release? These things should have the priority...


Reply to this email directly or view it on GitHubhttps://github.com//issues/3948#issuecomment-27467822
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants