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

[Merged by Bors] - docs: Full documentation for bevy_asset #3536

Closed
wants to merge 32 commits into from

Conversation

manokara
Copy link
Contributor

@manokara manokara commented Jan 3, 2022

Objective

This PR aims to document the bevy_asset crate to complete coverage, while also trying to improve some bits of UX.

Progress

  • Root items
  • handle module
  • info module
  • path module
  • loader module
  • io and filesystem_watcher module
  • assets module
  • asset_server module
  • diagnostic module
  • debug_asset_server module
  • Crate level documentation
  • Add #![warn(missing_docs)] lint

Coverage: 100%

Migration Guide

  • Rename FileAssetIo::get_root_path uses to FileAssetIo::get_base_path

    FileAssetIo::root_path() is a getter for the root_path field, while FileAssetIo::get_root_path returned the parent directory of the asset root path, which was the executable's directory unless CARGO_MANIFEST_DIR was set. This change solves the ambiguity between the two methods.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 3, 2022
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Docs An addition or correction to our documentation and removed S-Needs-Triage This issue needs to be labelled labels Jan 3, 2022
@mockersf
Copy link
Member

mockersf commented Jan 3, 2022

#3348 is also adding some docs in bevy_assets

@manokara
Copy link
Contributor Author

manokara commented Jan 3, 2022

Oh, and it's more detailed than mine! I just focused on giving them short summaries for the coverage. We should merge that one first and I'll rebase my work on top of it.

@alice-i-cecile
Copy link
Member

Awesome. I'll work on getting #3348 merged now then.

@manokara manokara marked this pull request as ready for review January 11, 2022 19:08
@manokara
Copy link
Contributor Author

All is done! Now the PR is up for full review. Everything is documented except for things that are self-expanatory, such as AssetEvent variants. And I think the crate level docs need some work.

There were a few code changes:

Each one was made as a separate commit, so if they're out of scope for this PR it'll be easy to revert them.

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/assets.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/filesystem_watcher.rs Outdated Show resolved Hide resolved
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

These are really solid: detailed and technical but covering how and why you may want to use these.

I have a few nits, and want to chat a bit more about the as_weak vs cast decision.

crates/bevy_asset/src/assets.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/diagnostic/mod.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/handle.rs Show resolved Hide resolved
@manokara
Copy link
Contributor Author

I was thinking of renaming AssetStage::LoadAssets as well, because it is not quite what that stage does as the docstring says. UpdateAssets?

@alice-i-cecile
Copy link
Member

I was thinking of renaming AssetStage::LoadAssets as well, because it is not quite what that stage does as the docstring says. UpdateAssets?

Yes please.

@alice-i-cecile
Copy link
Member

Migration Guide:

  • Renamed AssetStage::LoadAssets to UpdateAssets
  • Renamed Handle::as_weak to Handle::cast

@manokara
Copy link
Contributor Author

All done! I reverted the breaking changes as @alice-i-cecile suggested, but kept the get_root_path() one because it fixes the ambiguity with root_path(). I will make a separate PR for the others when this one is merged.

@alice-i-cecile alice-i-cecile removed the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jul 11, 2022
@alice-i-cecile
Copy link
Member

@bevyengine/docs-team I would really like to land this for 0.8.

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/asset_server.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jul 12, 2022
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I didn't review everything with a lot of details but it all seemed pretty good.

@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 Jul 12, 2022
@alice-i-cecile
Copy link
Member

Did a final review pass, everything checks out :) Thanks y'all!

bors r+

bors bot pushed a commit that referenced this pull request Jul 12, 2022
# Objective

This PR aims to document the `bevy_asset` crate to complete coverage, while also trying to improve some bits of UX.

### Progress

- [x] Root items
- [x] `handle` module
- [x] `info` module
- [x] `path` module
- [x] `loader` module
- [x] `io` and `filesystem_watcher` module
- [x] `assets` module
- [x] `asset_server` module
- [x] `diagnostic` module
- [x] `debug_asset_server` module
- [x] Crate level documentation
- [x] Add `#![warn(missing_docs)]` lint

Coverage: 100%

## Migration Guide

- Rename `FileAssetIo::get_root_path` uses to `FileAssetIo::get_base_path`

    `FileAssetIo::root_path()` is a getter for the `root_path` field, while `FileAssetIo::get_root_path` returned the parent directory of the asset root path, which was the executable's directory unless `CARGO_MANIFEST_DIR` was set. This change solves the ambiguity between the two methods.
@bors bors bot changed the title docs: Full documentation for bevy_asset [Merged by Bors] - docs: Full documentation for bevy_asset Jul 12, 2022
@bors bors bot closed this Jul 12, 2022
bors bot pushed a commit that referenced this pull request Jul 21, 2022
# Objective

- `#![warn(missing_docs)]` was added to bevy_asset in #3536
- A method was not documented when targeting wasm

## Solution

- Add documentation for it
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

This PR aims to document the `bevy_asset` crate to complete coverage, while also trying to improve some bits of UX.

### Progress

- [x] Root items
- [x] `handle` module
- [x] `info` module
- [x] `path` module
- [x] `loader` module
- [x] `io` and `filesystem_watcher` module
- [x] `assets` module
- [x] `asset_server` module
- [x] `diagnostic` module
- [x] `debug_asset_server` module
- [x] Crate level documentation
- [x] Add `#![warn(missing_docs)]` lint

Coverage: 100%

## Migration Guide

- Rename `FileAssetIo::get_root_path` uses to `FileAssetIo::get_base_path`

    `FileAssetIo::root_path()` is a getter for the `root_path` field, while `FileAssetIo::get_root_path` returned the parent directory of the asset root path, which was the executable's directory unless `CARGO_MANIFEST_DIR` was set. This change solves the ambiguity between the two methods.
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

- `#![warn(missing_docs)]` was added to bevy_asset in bevyengine#3536
- A method was not documented when targeting wasm

## Solution

- Add documentation for it
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

This PR aims to document the `bevy_asset` crate to complete coverage, while also trying to improve some bits of UX.

### Progress

- [x] Root items
- [x] `handle` module
- [x] `info` module
- [x] `path` module
- [x] `loader` module
- [x] `io` and `filesystem_watcher` module
- [x] `assets` module
- [x] `asset_server` module
- [x] `diagnostic` module
- [x] `debug_asset_server` module
- [x] Crate level documentation
- [x] Add `#![warn(missing_docs)]` lint

Coverage: 100%

## Migration Guide

- Rename `FileAssetIo::get_root_path` uses to `FileAssetIo::get_base_path`

    `FileAssetIo::root_path()` is a getter for the `root_path` field, while `FileAssetIo::get_root_path` returned the parent directory of the asset root path, which was the executable's directory unless `CARGO_MANIFEST_DIR` was set. This change solves the ambiguity between the two methods.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- `#![warn(missing_docs)]` was added to bevy_asset in bevyengine#3536
- A method was not documented when targeting wasm

## Solution

- Add documentation for it
bors bot pushed a commit that referenced this pull request Oct 28, 2022
# Objective

Following discussion on #3536 and #3522, `Handle::as_weak()` takes a type `U`, reinterpreting the handle as of another asset type while keeping the same ID. This is mainly used today in font atlas code. This PR does two things:

- Rename the method to `cast_weak()` to make its intent more clear
- Actually change the type uuid in the handle if it's not an asset path variant.

## Migration Guide

- Rename `Handle::as_weak` uses to `Handle::cast_weak`

    The method now properly sets the associated type uuid if the handle is a direct reference (e.g. not a reference to an `AssetPath`), so adjust you code accordingly if you relied on the previous behavior.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

This PR aims to document the `bevy_asset` crate to complete coverage, while also trying to improve some bits of UX.

### Progress

- [x] Root items
- [x] `handle` module
- [x] `info` module
- [x] `path` module
- [x] `loader` module
- [x] `io` and `filesystem_watcher` module
- [x] `assets` module
- [x] `asset_server` module
- [x] `diagnostic` module
- [x] `debug_asset_server` module
- [x] Crate level documentation
- [x] Add `#![warn(missing_docs)]` lint

Coverage: 100%

## Migration Guide

- Rename `FileAssetIo::get_root_path` uses to `FileAssetIo::get_base_path`

    `FileAssetIo::root_path()` is a getter for the `root_path` field, while `FileAssetIo::get_root_path` returned the parent directory of the asset root path, which was the executable's directory unless `CARGO_MANIFEST_DIR` was set. This change solves the ambiguity between the two methods.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- `#![warn(missing_docs)]` was added to bevy_asset in bevyengine#3536
- A method was not documented when targeting wasm

## Solution

- Add documentation for it
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Following discussion on bevyengine#3536 and bevyengine#3522, `Handle::as_weak()` takes a type `U`, reinterpreting the handle as of another asset type while keeping the same ID. This is mainly used today in font atlas code. This PR does two things:

- Rename the method to `cast_weak()` to make its intent more clear
- Actually change the type uuid in the handle if it's not an asset path variant.

## Migration Guide

- Rename `Handle::as_weak` uses to `Handle::cast_weak`

    The method now properly sets the associated type uuid if the handle is a direct reference (e.g. not a reference to an `AssetPath`), so adjust you code accordingly if you relied on the previous behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Docs An addition or correction to our documentation M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

7 participants