Conversation
rm5248
left a comment
There was a problem hiding this comment.
This looks pretty good to me, I have no real concerns.
| #if defined(OPENW3D_WIN32) | ||
| #include <windows.h> | ||
| #elif defined(OPENW3D_SDL3) | ||
| #include <SDL3/SDL_thread.h> |
There was a problem hiding this comment.
any preference on using std::thread vs the SDL thread? It probably doesn't matter that much which one we use.
There was a problem hiding this comment.
std::thread does not support thread priorities and, std::thread::id has an awkward way to convert to an integer (std::hash<std::thread::id>{}(std::this_thread::get_id())).
I'd prefer to use win32/SDL3, and perhaps switch later to c++ threads when we actually have the thing working on Linux: change 1 thing per commit.
There was a problem hiding this comment.
Do priorities even work correctly on Linux? A process needs to have root permissions to elevate its own priority, it can only reduce them IIRC and does Renegade really make use of them? Also, why SDL3 threads rather than native pthreads which are supported pretty much everywhere none windows (except getting integer thread IDs which seems oddly platform specific)? I only ask as I've attempted to write a pthread versions previously when reversing Zero Hour https://github.com/TheAssemblyArmada/Thyme/blob/develop/src/w3d/lib/thread.cpp
There was a problem hiding this comment.
posix pthreads provides a pthread_setschedprio.
It's probably used as a suggestion for the scheduler.
disclaimer: I am a SDL contributor so I am probably a bit biased.
I chose SDL3 to limit the number of dependencies as we'll already need it for creating windows and input handling. SDL uses the optimal threading libraries for each platform. Looking at SDL3's `src/thread directory, it contains implementations for n3ds, ps2, psp, pthreads, vita and windows.
I also remember this SDL timer pr wanting to replace ps2-specific code with generic code. The penalty would've been 25x.
I don't think we should reinvent the wheel here.
There was a problem hiding this comment.
I came across this project a while ago, perhaps it would be useful for loading DLLs? What you have here is good, I'm just thinking of the future.
| StringClass cPathUtil::GetWorkingDirectory(bool trailing_separator) | ||
| { | ||
| std::error_code ec; | ||
| auto path = std::filesystem::current_path(ec); | ||
| if (!ec) { | ||
| path = "."; | ||
| } | ||
| if (trailing_separator) { | ||
| path += std::filesystem::path::preferred_separator; | ||
| } | ||
| return StringClass{path.generic_string().c_str()}; | ||
| } | ||
|
|
There was a problem hiding this comment.
CODEX:
Medium: Working-directory helper always collapses to ["."] on success.
[pathutil.cpp (line 9)] uses [if (!ec) path = ".";] (likely meant if (ec)), so successful current_path is discarded.
caseychaos1212
left a comment
There was a problem hiding this comment.
I had CODEX take a look.
Let me know if any of this is hallucinations or just missing context. It's a good learning experience for me.
| #if defined(OPENW3D_WIN32) | ||
| #include <windows.h> | ||
| #elif defined(OPENW3D_SDL3) | ||
| #include <SDL3/SDL_thread.h> |
There was a problem hiding this comment.
Do priorities even work correctly on Linux? A process needs to have root permissions to elevate its own priority, it can only reduce them IIRC and does Renegade really make use of them? Also, why SDL3 threads rather than native pthreads which are supported pretty much everywhere none windows (except getting integer thread IDs which seems oddly platform specific)? I only ask as I've attempted to write a pthread versions previously when reversing Zero Hour https://github.com/TheAssemblyArmada/Thyme/blob/develop/src/w3d/lib/thread.cpp
b2b617a to
2abc833
Compare
OmniBlade
left a comment
There was a problem hiding this comment.
I think sdl3 should be added to the vcpkg config as well as part of this PR, but other than that I don't have any other concerns.
The function measure CPU speed using win32-only functions. It's also unreliable.
2abc833 to
a3052b5
Compare
I added |
caseychaos1212
left a comment
There was a problem hiding this comment.
I'm good with this.
This PR starts introducing SDL3 and DXVK by replacing win32-only function calls with abstractions on top of either win32 or SDL3.
Testing on Windows is limited.