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.

Sign up for GitHub

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

De-SDL voodoo threading #2568

Merged
merged 1 commit into from Jun 3, 2023
Merged

De-SDL voodoo threading #2568

merged 1 commit into from Jun 3, 2023

Conversation

kklobe
Copy link
Collaborator

@kklobe kklobe commented Jun 2, 2023 •
edited

Migrate all Voodoo SDL threading-related code to C++17. Add a simple semaphore implementation to replace SDL's semaphore.

Mostly this is just an excuse for me to get a bit more experience with C++ threading; I didn't notice any tangible performance benefits.

@kklobe kklobe added voodoo Issues related to the 3dfx Voodoo 1 cleanup Non-functional changes that simplify, improve maintainability, or squash warnings labels Jun 2, 2023
@kklobe kklobe self-assigned this Jun 2, 2023
@kklobe kklobe force-pushed the kk/voodoo-deSDL-threading-1 branch from 12fa18a to eda428a Compare June 3, 2023 03:00
@kcgen kcgen self-requested a review June 3, 2023 12:34
@kklobe kklobe force-pushed the kk/voodoo-deSDL-threading-1 branch from eda428a to 9003b6d Compare June 3, 2023 12:35
kcgen
kcgen reviewed Jun 3, 2023
View reviewed changes
Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

Suggest adding a couple unit tests to exercise it.

The rwqueue has a couple test cases that access a queue using multiple threads, I think this would be a good template to use here too.

@kklobe kklobe force-pushed the kk/voodoo-deSDL-threading-1 branch from 9003b6d to 502e1f9 Compare June 3, 2023 12:51
@kklobe
De-SDL voodoo threading
e98b469 
Migrate all SDL threading-related code to C++17. Add a simple semaphore
implementation to replace SDL's semaphore.
@kklobe kklobe force-pushed the kk/voodoo-deSDL-threading-1 branch from 502e1f9 to e98b469 Compare June 3, 2023 12:53
@kklobe
Copy link
Collaborator Author

kklobe commented Jun 3, 2023

Suggest adding a couple unit tests to exercise it.

The rwqueue has a couple test cases that access a queue using multiple threads, I think this would be a good template to use here too.

I put a very basic unit test in there, will try to get more creative if desired.

@kklobe
Copy link
Collaborator Author

kklobe commented Jun 3, 2023

P.S. I consider the semaphore class a short-term hack until we can use C++20's std::counting_semaphore

@kcgen kcgen marked this pull request as ready for review June 3, 2023 13:05
@kcgen kcgen self-requested a review June 3, 2023 13:05
kcgen
kcgen approved these changes Jun 3, 2023
View reviewed changes
Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

Looks great, @kklobe!

Merging shortly after I can get a couple positive tests completed on my side.

@kcgen
Copy link
Member

kcgen commented Jun 3, 2023

Nice to have an all C++17 implementation, @kklobe !
I'm not seeing any regression in testing, so let's get this merged 🚀

@kcgen kcgen merged commit 4747609 into main Jun 3, 2023
53 checks passed
@kcgen kcgen deleted the kk/voodoo-deSDL-threading-1 branch June 7, 2023 14:13
@Grounded0 Grounded0 mentioned this pull request Nov 17, 2023
Closed
4 tasks
@johnnovak johnnovak added the video Graphics and video related issues label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kcgen kcgen kcgen approved these changes

Assignees

@kklobe kklobe

Labels
cleanup Non-functional changes that simplify, improve maintainability, or squash warnings video Graphics and video related issues voodoo Issues related to the 3dfx Voodoo 1
Projects
0.81 Project
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kklobe @kcgen @johnnovak