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

Jump to bottom

Fixes for modal progress and NewGRF scan issues #8828

Closed
wants to merge 4 commits into from
Closed

Fixes for modal progress and NewGRF scan issues #8828

wants to merge 4 commits into from

Conversation

JGRennison
Copy link
Contributor

Motivation / Problem

Fix: #8760
Fix the 3rd data race in: #8712

Description

  • Add a mutex/CV pair for changes to the modal progress state.
  • Use the mutex/CV pair to wait for the modal progress state to become false before exiting.
  • Use the mutex/CV pair for MODAL_PROGRESS_REDRAW_TIMEOUT timed waits instead of CSleep.
  • Add an abort flag for earlier conclusion of the NewGRF scan thread when exiting.

Limitations

The abort flag could conceivably be made more forceful, but that would require more invasive changes and head into the realm of diminishing returns.

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')

@LordAro LordAro requested a review from TrueBrain March 9, 2021 09:22
@TrueBrain TrueBrain mentioned this pull request Mar 9, 2021
Merged
@TrueBrain
Copy link
Member

I went a slightly different route for these problems with #8830. Not because there is anything wrong with this PR (in fact, if #8830 is not approved, I think we should approve this for 1.11), but because for a while now I am kinda annoyed how much of a hack the modal window implementation is, and how crappy (UX-wise) it works.

Turns out that with the recent gameloop-thread addition, it is rather simple to simplify the situation, and remove the threads and all the issues that come from using them.

For the fourth commit I also took another approach, and hooked into the already existing _exit_game. I am not sure if that was already possible, or now possible because of the work I did on the modal windows, but I think they signal the same. And if I can avoid a global, I really do love to avoid a global :D If you disagree, please do let me know!

@TrueBrain TrueBrain added this to the 1.11.0 milestone Mar 9, 2021
@JGRennison JGRennison deleted the modal-progress-fixes branch January 9, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TrueBrain TrueBrain Awaiting requested review from TrueBrain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.

Game asserts/crashes when closed during newgrf scan
2 participants
@JGRennison @TrueBrain