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

Fix: don't propagate shift/ctrl state till next game-tick #9381

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 17, 2021

Motivation / Problem

Another bug found with ThreadSanitizer. It is a bit unlikely someone really runs into this, but what we do is also not correct. So I fixed it anyway.

Heavily inspired by JGRennison/OpenTTD-patches@59daa57

Description

When the game-loop is very slow, it was easily possible to start
the loop with _shift_pressed being false, but end with
_shift_pressed being true. This doesn't hurt the game as such,
but for the user this can be very weird: I pressed "Buy Vehicle",
pressed shift a bit later, and I still get a cost indication.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain changed the title Fix: don't propegate shift/ctrl state till next game-tick Fix: don't propagate shift/ctrl state till next game-tick Jun 17, 2021
When the game-loop is very slow, it was easily possible to start
the loop with _shift_pressed being false, but end with
_shift_pressed being true. This doesn't hurt the game as such,
but for the user this can be very weird: I pressed "Buy Vehicle",
pressed shift a bit later, and I still get a cost indication.
@michicc
Copy link
Member

michicc commented Jun 17, 2021

There isn't really much threading left with this, is it?

@TrueBrain
Copy link
Member Author

TrueBrain commented Jun 17, 2021

There isn't really much threading left with this, is it?

Well, yes and no :) Those 2 functions are basically a no-op .. just some shuffling around of a few variables. So nothing really changes with this PR on that balance.
But in general, there is a way too much inside the game-state-lock in my opinion. But .. that needs more work and attention to detail to fix.

The most important things for now is that the Paint and taking the video-lock are outside the game-state-lock. The first is rather expensive on non-OpenGL .. the last is expensive with vsync on. So there is still some benefit, luckily enough :D

@TrueBrain TrueBrain merged commit cf86559 into OpenTTD:master Jun 17, 2021
@TrueBrain TrueBrain deleted the shift-flipping-is-fun branch June 17, 2021 18:23
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 this pull request may close these issues.

2 participants