Don't use SDL filesystem in DInput joystick driver#15845
Conversation
| char path[128]; | ||
| WIN32_FIND_DATAA file; | ||
| SDL_snprintf(path, sizeof(path), "%s%s", directories[i], "/EZFRD64.DLL"); | ||
| HANDLE search_handle = FindFirstFileA(path, &file); |
There was a problem hiding this comment.
I don't mind switching to Win32 filesystem code here, but this isn't equivalent, and won't find e.g. "C:\Windows\USB_Vibration\791847\EZFRD64.DLL"
There was a problem hiding this comment.
Changed the approach to use where command ran from the command line to simplify the code
d044515 to
33721aa
Compare
|
I'm sorry, but why are we doing this? Shouldn't the internal use of |
|
It's done in case an external project only uses the joystick/gamepad subsystems of SDL and not the filesystem one. So I replaced the filesystem subsystem usage here so external projects don't have to include the filesystem subsystem just for this check, if that's okay. |
|
I'm genuinely unsure how you use this code without the rest of SDL being available. Are you just adding a subset of SDL source files directly to the project instead of dynamically linking against the DLL? I think adding a If there's some sort of integration friction here (i.e why are we picking subsystems / manually including source files), I'd much rather that was addressed instead. |
Yes, we do that in Godot to decrease the binary size so we don't have quite a lot of unused code: Similar approach is done in QGroundControl:
See also godotengine/godot#87925 (comment)
|
In this case we can include the filesystem subsystem in Godot, but I think we should wait for slouken's answer |
|
Okay, I understand. IMO, if you want to do it that way, that's fine; But maintaining that sort of build should fall squarely on Godot's shoulders. This shifts some of the 'burden' back to the SDL project. I think this is a bad change for SDL, and just looking at it, it doesn't seem very onerous for Godot to just also include the FS code, even if it adds a few KiB. I've said my piece. Ultimately it's up to Sam and the real maintainers. |
|
We shouldn't add a system() call here. If you want to avoid using SDL filesystem code, that's fine, but you should write custom findfirst/findnext code instead. |
|
... or just link with SDL filesystem code. It's not that big. :) |
|
I see, thank you for your answer, I think we will include the filesystem code in Godot, that seems to be the better approach for now! 😅 |
Description
This PR makes the DirectInput joystick driver optionally not use the SDL filesystem subsystem in case an external project that intergrates SDL (e.g. Godot Engine) doesn't use its filesystem subsystem.
Please let me know if this change is acceptable!
Existing Issue(s)
None