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

This fixes TD being stuck running in the background #925

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

tore29
Copy link
Contributor

@tore29 tore29 commented May 24, 2023

Currently TD stays running after clicking on "exit game".

It gets stuck on the do/while loop that checks the keyboard for something. Not sure what the purpose of this is, but it does prevent the game from quitting.

Should probably check RA too and see if a non SDL build gets similarly stuck.

@Gerwin2k
Copy link

Gerwin2k commented May 24, 2023

See my comment here:
#917

This pull request may very well fix it, practically. But it does not revert the change that broke it in the first place.

I suspect when there is an SDL build it should not call "PostMessageA(MainWindow, WM_DESTROY, 0, 0);" at all, but use SDL functions to do that.

On line 300 there is a similar situation with starting up. Where one either uses the SDL functions OR the windows functions. But not both:

#if defined(_WIN32) && !defined(SDL_BUILD)
        Create_Main_Window(ProgramInstance, ScreenWidth, ScreenHeight);

@Gerwin2k
Copy link

This is how I fixed it for my local build:

#if defined(SDL_BUILD)
        Reset_Video_Mode();
        Sound_End();
#elif defined(_WIN32)
        Sound_End();
        PostMessageA(MainWindow, WM_DESTROY, 0, 0);
        do {
            Keyboard->Check();
        } while (ReadyToQuit == 1);
#endif

@tore29
Copy link
Contributor Author

tore29 commented May 24, 2023

I see in the RA code there is a similar idea.

#if defined(_WIN32) && !defined(SDL_BUILD)
        PostMessage(MainWindow, WM_DESTROY, 0, 0);

        /*
        ** Wait until the message handler has dealt with the message
        */
        do {
            Keyboard->Check();
        } while (ReadyToQuit == 1);
#endif

Perhaps it might be a good idea to standardize?

This will pretty much return it to the state before the commit that broke it.

There is a separate question though if the keyboard check has any purpose and if a WIN32 build without SDL will suffer the same issue by not closing correctly.

@Gerwin2k
Copy link

Gerwin2k commented May 24, 2023

That guilty commit introduced Sound_End(). That was the main purpose.
It also changed the compiler directives logic, and I am pretty sure that was a mistake / not on purpose.
So what I did was change the compiler directives back to practically the original situation, but retain Sound_End().

The original guilty commit authors can clarify maybe?

As for WIN32 (non-SDL) DirectDraw build. I used such builds in the earlier stages of the project, but they seemed crash-prone in a multi-core environment. Whereas the SDL build had no such instability. Don't recall any failure to exit properly.

@tore29
Copy link
Contributor Author

tore29 commented May 25, 2023

#if defined(SDL_BUILD)
        Reset_Video_Mode();
#endif

        Sound_End();

        /*
        ** Flag that this is a clean shutdown (not killed with Ctrl-Alt-Del)
        */
        ReadyToQuit = 1;

        /*
        ** Post a message to our message handler to tell it to clean up.
        */
#if defined(_WIN32) && !defined(SDL_BUILD)
        PostMessage(MainWindow, WM_DESTROY, 0, 0);

        /*
        ** Wait until the message handler has dealt with the message
        */
        do {
            Keyboard->Check();
        } while (ReadyToQuit == 1);
#endif

In the latest commit I changed the solution to be like in VanillaRA. This keeps the PostMessage call out of SDL builds, and keeps the keyboard check for non-SDL builds.

@Gerwin2k
Copy link

Your latest version of the pull request looks good to me.

@OmniBlade
Copy link
Contributor

There appears to be an unintended ascii > utf8 replacement in the PR which needs reverting, you you revert that and rebase the PR onto the latest vanilla branch?

@tore29 tore29 force-pushed the fix-td-exit branch 5 times, most recently from 5aff963 to cfb91a9 Compare December 19, 2023 17:57
@tore29
Copy link
Contributor Author

tore29 commented Dec 19, 2023

There appears to be an unintended ascii > utf8 replacement in the PR which needs reverting, you you revert that and rebase the PR onto the latest vanilla branch?

This should now be fixed and the branch is rebased with the latest vanilla branch.

@OmniBlade OmniBlade merged commit 83c8956 into TheAssemblyArmada:vanilla Dec 23, 2023
10 checks passed
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.

3 participants