-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 tokio / async-io optional task execution engines 2 #6137
Conversation
Hello, I can see that both older and newer PRs are reviewed/commented, is there any particular reason for this PR not being reviewed? Can I do anything to speed it up? |
This PR is harder to review. While the code change is small, reviewing it properly would mean cloning the PR locally, write an example with tokio compatibility enabled and tokio tasks, and looking at how it behaves, comparing to similar tasks without tokio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments on the earlier PR still remain:
This seems like a great change for those looking for tokio support or in general better async task performance.
However, I don't see any initialization code at all or anything that enqueues tasks added to TaskPools to the global executors. If the intent is just to allow global executors to run alongside the Bevy TaskPools, this change is sufficient, though I'm concerned with how we're adding yet more threads to the engine by default, but if we want to leverage them for running the core of bevy itself, there will need to be changes with how TaskPool is structured.
My whole intent was being able to use tokio in bevy in a way that will not force me to use async_compat which is not as convenient to use and probably not maintained anymore. I didn't analyze impact of adding new threads, if you believe it will negatively impact bevy then feel free to close it this PR. |
Related: #11995 |
This is a copy of abandoned PR: #2624, I hope every original assumption is kept. All credit should go to @hanabi1224 as he/she is the author of 99% of the code present here and I just resolved conflicts so this could be reviewed again and possibly merged. Quick test shows that added features are not exclusive.