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

Replace NativeFile to use SDL IO functions. #2133

Merged
merged 8 commits into from
Jan 14, 2025

Conversation

MikuAuahDark
Copy link
Contributor

This removes any platform-specific code from NativeFile and rely on SDL3 IO functions diretly.

One issue I found with this is that SDL3 doesn't expose any function to set the IO buffering mode. This is why this change goes into a PR to discuss how should the buffering function behaves.

@MikuAuahDark MikuAuahDark force-pushed the nativefile-sdl branch 3 times, most recently from f4f950a to c8e90f1 Compare January 2, 2025 06:21

if (setvbuf(file, nullptr, vbufmode, (size_t) size) != 0)
return false;
// FIXME: SDL doesn't have option to set buffering.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should make an issue in their repo about this? I feel like it's pretty useful / important, but at the very least we'd get their opinion.

Copy link
Member

Choose a reason for hiding this comment

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

That being said, for now we could probably implement BUFFER_LINE ourselves (like what's done in physfs/File.cpp) and leave the rest as a todo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I opened ticket in SDL for this. libsdl-org/SDL#11832

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering that SDL is just low-level wrapper to OS-specific API (such as Windows file HANDLE or POSIX file descriptor) and there's no explicit way to set buffering mode on those handle/file descriptors, then the best but unfortunate way to solve this is to implement our own buffering.

@MikuAuahDark
Copy link
Contributor Author

One more thing I forgot to mention is the behavior of file:getSize(). It only returns meaningful values (if applicable) once the file has been opened. If the file is closed, it will always return -1. This is because it uses SDL_GetIOSize which requires valid SDL_IOStream* handle.

@slime73
Copy link
Member

slime73 commented Jan 5, 2025

Our physfs File backend temporarily opens the file to determine its size in that situation:

if (file == nullptr)
{
open(MODE_READ);
int64 size = (int64) PHYSFS_fileLength(file);
close();
return size;
}

@slime73
Copy link
Member

slime73 commented Jan 14, 2025

Looks good, thanks!

@slime73 slime73 merged commit 176a3c1 into love2d:main Jan 14, 2025
7 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.

2 participants