Skip to content
New issue

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

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

Already on GitHub? # to your account

Move file-static RenderWindow and RenderTexture instances inside functions to fix GL race condition on startup #677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nater0000
Copy link

This change is not required on all platforms, but currently when compiling against SFML 2.6.2 static libraries, at runtime, the GL layer on Windows is encountering an Access Violation attempting to lock a critical section before main() is reached.

@nater0000
Copy link
Author

I did not mean to add the vcxproj or sln files. Should I close and re-open a new PR without those changes?

@NQNStudios
Copy link
Collaborator

You can get the VS file changes out of it without closing this PR. From the command line of your local branch:

git reset --soft HEAD^     # This undoes the commit but leaves your changes staged
git restore --staged proj     # This unstages the changes to the project files
git commit -m "Move file-static RenderWindow/RenderTexture instances into functions

this fixes a GL race condition on startup" # This makes your commit again, with a shorter headline and more info split to lower lines, which will display better in the history
git push --force          # This rewrites the history of your branch, automatically updating the pull request without requiring you to make a new one

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Mar 4, 2025

A couple of small amendments to NQN's suggestion.

Before running any of that, run the following command:

git show --stat HEAD

That will give you a line starting with "commit HASH", where HASH looks like a random string. Copy-paste the hash. Then, instead of the commit command NQN suggested, use this commit one:

git commit -C HASH

Where HASH is the string you just copy-pasted. I think you can also add --edit on the end if you want to change the commit message.

@nater0000 nater0000 force-pushed the global-render-objs branch from 463f657 to 9d99644 Compare March 4, 2025 01:52
@nater0000
Copy link
Author

I think it worked

Copy link
Member

@CelticMinstrel CelticMinstrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually pretty nice. It might be a huge change, but it's nice and simple – practically a find-and-replace for the most part. There are a couple of small issues, but once those are dealt with (and once I've worked out the CI problem in #674 / #675), I'll go ahead and merge it.

Thanks for the help!

Comment on lines +24 to +34
sf::RenderTexture& toolbar_gworld()
{
static sf::RenderTexture instance;
return instance;
}
cToolbar toolbar;
}
sf::RenderTexture& cToolbar::cache()
{
return UI::toolbar_gworld();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any need to have two different functions here. I'd recommend declaring cToolbar::cache() static in the header, and moving the cntent of toolbar_gworld() into it.

Also, I don't think it needs to be made public?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way just to be consistent with the other instances of sf::RenderWorld/Texture but please make any desired changes!

@@ -4,7 +4,7 @@
#include "pc.menus.hpp"

OpenBoEPCEditMenu::OpenBoEPCEditMenu(sf::RenderWindow& window)
: mainPtr { window }
: mainPtr() { window }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. I'm pretty sure it won't even compile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch! It looks like this class is only used in pc.menus.linux.cpp so I didn’t try compiling it on Win32.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CelticMinstrel I think the CI would have shown this error if you approve the workflow to run?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a Linux setup right now so I can't check directly.

@CelticMinstrel
Copy link
Member

Note: I meant #676 not #675.

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

Successfully merging this pull request may close these issues.

3 participants