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

Platform independent wrappers for TaskPool and Scope #4304

Open
MiniaczQ opened this issue Mar 23, 2022 · 2 comments
Open

Platform independent wrappers for TaskPool and Scope #4304

MiniaczQ opened this issue Mar 23, 2022 · 2 comments
Labels
A-Tasks Tools for parallel and async work C-Feature A new feature, making something new possible

Comments

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Mar 23, 2022

What problem does this solve or what need does it fill?

Currently TaskPool and Scope (from bevy_tasks) are implemented completely separately for wasm and non-wasm platforms, the correct ones are selected during re-export of the module.
This means that their documentation depends on the platform in use, but in practice the wasm one was unattended for more than a year.
It also means that mismatch in implementations will leak out into other creates rather than be contained within bevy_tasks.

Another reason is that tests can be centralized. Currently only non-wasm has tests.

What solution would you like?

As suggested by @TheRawMeatball, we should create wrappers for both types that contain all the user-oriented documentation,
while the hidden platform specific ones can be dev-oriented.

What alternative(s) have you considered?

I originally suggested creating traits for both types.
Both methods allow us to scope implementation problems inside the bevy_tasks crate, instead of leaking them outside,
as well as centralize user-oriented documentation,
but traits create a weird dependency chain of:
platform independent trait -> platform dependent impl -> platform independent usage
while the wrapper way provides us with:
platform dependent impl -> platform independent wrapper -> platform independent usage
which is much more straightforward.

Questions

Assuming we keep the names for the wrappers, what would the platform specific types be called?

More context

Ideally we wanna get this done before #4078 comes out with PR for wasm tasks.

@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 23, 2022
@MiniaczQ
Copy link
Contributor Author

I should mention, this is quite the opposite case to #4286, where the Task was already platform independent, yet had a wrapper.

@alice-i-cecile alice-i-cecile added A-Tasks Tools for parallel and async work and removed S-Needs-Triage This issue needs to be labelled labels Mar 23, 2022
@MiniaczQ
Copy link
Contributor Author

this is a problem
wasm implementation just straight up hardcoded the return type to fit the single use case of asset server
Trying to return an actual Task that is unrelated to the closure is completely wrong, so I'll fix this by:

  • removing TaskPool::spawn, it's never used
  • changing TaskPool::spawn_local to spawn_local_detached so there is no return type
    this can be easily reverted during wasm tasks rework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tasks Tools for parallel and async work C-Feature A new feature, making something new possible
Projects
None yet
Development

No branches or pull requests

2 participants