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

elaborate on TaskPool and bevy tasks #8750

Merged
merged 9 commits into from
Aug 11, 2023

Conversation

hmeine
Copy link
Contributor

@hmeine hmeine commented Jun 4, 2023

Objective

I found it very difficult to understand how bevy tasks work, and I concluded that the documentation should be improved for beginners like me.

Solution

These changes to the documentation were written from my beginner's perspective after
some extremely helpful explanations by nil on Discord.

I am not familiar enough with rustdoc yet; when looking at the source, I found the documentation at the very top of usages.rs helpful, but I don't know where they are rendered. They should probably be linked to from the main bevy_tasks README.

I found it very difficult to understand how bevy
tasks work, and these changes to the documentation
were written from my beginner's perspective after
some extremely helpful explanations by nil on Discord.
@hmeine
Copy link
Contributor Author

hmeine commented Jun 4, 2023

Note that I have not checked if the docs render correctly, but I hope this PR is useful nonetheless.

@hmeine
Copy link
Contributor Author

hmeine commented Jun 4, 2023

I just realized that I may even have been mislead by the spawn() docs:

Spawns a static future onto the thread pool. The returned Task is a future. It can also be cancelled and “detached” allowing it to continue running without having to be polled by the end-user.

"allowing it to continue running without having to be polled by the end-user" actually sounds as if tasks on which one did not call detach() require polling to be executed, don't you agree? I am not trying to nit-pick here, I hope that's clear – I am just trying to debug why I struggled so much with building a mental model of what bevy_tasks does.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Tasks Tools for parallel and async work labels Jun 4, 2023
@hmeine
Copy link
Contributor Author

hmeine commented Jun 4, 2023

I have fixed the style and tried to address the spawn() docs issue. I still think that the blurp at the top of usages.rs would also be helpful, but it does not seem to be rendered anywhere.

@alice-i-cecile
Copy link
Member

So, those are module level docs for the usages module. But it looks like we're not publicly exporting that module, so they're only visible to people reading the source code :(

In this case, I think that the right fix is actually to grab all of those docs and move them to the top-level bevy_tasks docs.

The non-public `usages` module contained useful documentation that is
missing from the published docs, and that is in fact good for the
`bevy_tasks` front page.
@hmeine
Copy link
Contributor Author

hmeine commented Jun 5, 2023

I did so, and also rendered the docs locally and did some final polishing.

@alice-i-cecile alice-i-cecile requested a review from mockersf June 5, 2023 18:54
@hmeine
Copy link
Contributor Author

hmeine commented Jun 6, 2023

Hmm, stupid question: Should I fix typos in extra commits or force-push a squashed version?

@alice-i-cecile
Copy link
Member

Extra commits are my preference, but we don't enforce commit style. I tend to find it makes things easier for reviewers to follow, and the commit history on main is squashed by PR anyways.

I noticed that I accidentally "fixed" what I thought were missing full
stops after the summary phrase / paragraph at the beginning of the pool
struct documentation.  However, these summaries appear on the top level
bevy_tasks docs in some kind of table, where those periods would not be
desirable.
crates/bevy_tasks/README.md Outdated Show resolved Hide resolved
crates/bevy_tasks/src/usages.rs Show resolved Hide resolved
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Generally LGTM but a few points need to be addressed/reworded.

@hmeine
Copy link
Contributor Author

hmeine commented Jul 1, 2023

Generally LGTM but a few points need to be addressed/reworded.

"A few points" sounds as if there was more to be done than the WASM sentence? :-)

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

tiny nit. looks good otherwise.

crates/bevy_tasks/src/task_pool.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 11, 2023
Co-authored-by: Mike <mike.hsu@gmail.com>
@mockersf mockersf added this pull request to the merge queue Aug 11, 2023
Merged via the queue into bevyengine:main with commit 1abb6b0 Aug 11, 2023
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-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants