Fill out some missing docs for bevy_assets #17829
Conversation
alice-i-cecile
added
C-Docs
alice-i-cecile
force-pushed
the
assorted-assets-docs
branch
from
5c82d76 to
8e0f2ee
Compare
alice-i-cecile
added
X-Uncontroversial
|
@Carter0, want to take a look at this PR too? |
| @@ -26,15 +26,20 @@ pub enum AssetId<A: Asset> { | |||
| /// | |||
| /// [`Assets`]: crate::Assets | |||
| Index { | |||
| /// The unstable, opaque index of the asset. | |||
There was a problem hiding this comment.
This comment sent me down a giant rabbit whole lol.
I get that opaque means that there is some data known to the compiler but not to the user. I don't really know what unstable means in this context though.
Maybe it's worth linking to the AssetIndex struct for users to read more?
There was a problem hiding this comment.
Opaque means you can't "see inside" it, or e.g. do arithmetic with it, but only use it by giving it to APIs. The compiler helps us enforce this but that's not a requirement for using the term.
Unstable means you can't expect to get the same index between restarts of the application, so it doesn't make sense to serialize it for example.
In all cases I can think of where doc links are rendered you'd also have the type right there, so a link is probably redundant.
| @@ -24,6 +24,7 @@ pub struct EmbeddedWatcher { | |||
| } | |||
|
|
|||
| impl EmbeddedWatcher { | |||
| /// Creates a new `EmbeddedWatcher` that watches for changes to the embedded assets in the given `dir`. | |||
There was a problem hiding this comment.
I don't really think you need this. The comment above the struct is better. I guess you might want it for the future lints though.
There was a problem hiding this comment.
Yeah this is lint-proofing.
| @@ -15,6 +15,8 @@ use std::path::{Path, PathBuf}; | |||
| #[cfg(feature = "embedded_watcher")] | |||
| use alloc::borrow::ToOwned; | |||
|
|
|||
| /// The name of the `embedded` [`AssetSource`], | |||
| /// as stored in the [`AssetSourceBuilders`] resource. | |||
| pub const EMBEDDED: &str = "embedded"; | |||
There was a problem hiding this comment.
What does "embedding" an asset in this context mean? I looked at the example and it didn't really explain it either.
There was a problem hiding this comment.
It means putting the asset inside the game executable file, in contrast to other assets that are normally loaded from disk to memory when needed. It's normally only used for small engine-related assets so you can use them without having them in your own asset directory.
It sounds like the example could use a line or 2 describing that.
| pub watcher: Option< | ||
| Box< | ||
| dyn FnMut(crossbeam_channel::Sender<AssetSourceEvent>) -> Option<Box<dyn AssetWatcher>> | ||
| + Send | ||
| + Sync, | ||
| >, | ||
| >, | ||
| /// The [`ErasedAssetReader`] to use for processed assets. |
There was a problem hiding this comment.
Might be worth linking to this somewhere? That way people know what processing is. https://github.com/bevyengine/bevy/blob/main/crates/bevy_asset/src/processor/mod.rs
| @@ -32,8 +32,10 @@ pub struct ProcessorTransactionLog { | |||
| /// An error that occurs when reading from the [`ProcessorTransactionLog`] fails. | |||
| #[derive(Error, Debug)] | |||
There was a problem hiding this comment.
Also might be worth linking to this above the ProcessorTransactionLog
struct. https://github.com/bevyengine/bevy/blob/main/crates/bevy_asset/src/processor/mod.rs
There was a problem hiding this comment.
As a new user to the bevy_assets crate, there is a lot of docs here for fields in structs. That's good and I think that its a step towards requiring docs in the lints, which is also good!
Personally though, I find the docs over the small stuff is only useful if you already know the big picture stuff. Thats why I like links to the big docs in mod files or above structs because its a place for people like me to learn the concept first, then dig into the specifics.
Idk if Bevy has a documentation roadmap or plan already. Documentation is hard because there are several different kinds of documentation and they all serve different purposes.
Anyway, Thanks for doing this and I do like it!
alice-i-cecile
added
S-Ready-For-Final-Review
Objective
bevy_assetshas long been unapproachable for contributors and users. More and better documentation would help that.We're gradually moving towards globally denying missing docs (#3492)!
However, writing all of the hundreds of missing doc strings in a single go will be miserable to review.
Solution
Remove the allow for missing docs temporarily, and then pick some easy missing doc warnings largely at random to tackle.
Stop when the change set is starting to feel intimidating.