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

make file loading async on desktop #405

Closed
wants to merge 1 commit into from

Conversation

lenscas
Copy link
Contributor

@lenscas lenscas commented Aug 10, 2023

Sadly, this requires an extra bound on the function but I think it is worth it as it means that loading files behaves similar on both web and desktop.

I don't have a way to test android on hand so I left that one as is.

I also don't think that spawning threads for this is problematic, IIRC libraries like tokio also spawn threads internally for this.

@not-fl3
Copy link
Owner

not-fl3 commented Aug 10, 2023

I am not really sure if spawning a thread overhead is worth the potential benefits of async file loading.
Its a very common pattern to load a lot of relatevely small files in the beginning of the game, and spawning so much threads would be noticably slower.

While having a thread would help a lot for non-blocking read of very large files, I am not quite sure if this is the most common file reading pattern in games.

Tho I do not really have an opinion strong enough to either merge it or not, let's just keep it open for now.

@lenscas
Copy link
Contributor Author

lenscas commented Aug 10, 2023

My main motivation for this is related to making the behavior both consistent between all platforms (at least the ones I can test) and match with what is expected. I myself already spent a good chunk of time trying to figure out why loading files didn't behave as I expected the first time I used the file loading API and I am not the only one.

Before this PR there is also no API available to actually do async file loading, which at least one person in the discord already wanted/needed.

Perhaps, the API can be altered so the user can pass in an enum that tells if they prefer an async operation or a sync one? With the correct wording in the types it should be more obvious that the file gets loaded in another mode if the platform doesn't implement their preferred mode while also making it easy for the user to select what mode they want?

Something like

pub enum PreferMode {
    Async,
    Sync
}

load_file("my_awesome_file.txt", PreferMode::Async).await.unwrap(); //will load the file async on desktop and the web
load_file("my_awesome_file.txt",PreferMode::Sync).await.unwrap();   //will load the file sync on desktop, async on web

Alternatively, it is possible to split the 2 functions up so desktop has both a sync and an async way of loading files while web only has async, but that to me sounds more annoying to use when people want to support multiple platforms.

@not-fl3
Copy link
Owner

not-fl3 commented Aug 10, 2023

Oh, I did not noticed that this is in miniqud repo, my bad.

For miniquad fs::load is laoding files in the simpliest way possible, all the higher level abstractions are upon the user.

@not-fl3 not-fl3 closed this Aug 10, 2023
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