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] - Fix hot reloading for read_asset_bytes #6797

Conversation

SpecificProtagonist
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist commented Nov 29, 2022

Objective

Fixes #6780

Solution

  • record the asset path of each watched file
  • call AssetIo::watch_for_changes in LoadContext::read_asset_bytes

Changelog

Fixed

  • fixed hot reloading for LoadContext::read_asset_bytes

Changed

  • AssetIo::watch_path_for_changes allows watched path and path to reload to differ

Migration Guide

  • for custom AssetIos, differentiate paths to watch and asset paths to reload as a consequence

@SpecificProtagonist SpecificProtagonist force-pushed the read_asset_bytes branch 2 times, most recently from a46a80d to 468f2f7 Compare November 29, 2022 18:11
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds labels Nov 29, 2022
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jan 11, 2023
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.

Good comments and a nice fix.

@bors
Copy link
Contributor

bors bot commented Jan 11, 2023

try

Build failed:

@SpecificProtagonist
Copy link
Contributor Author

Had updated the wasm code but forgot about Android; fixed now.

@alice-i-cecile
Copy link
Member

Awesome thank you

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Feb 6, 2023
Comment on lines 71 to 73
/// Otherwise, if `to_watch` gets modified, trigger a reload for `to_reload`.
/// Note that multiple `to_reload` paths can map to the same `to_watch` and
/// multiple `to_watch` paths can map to the same `to_reload`.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add more details on why someone calling this method would want to provide two different paths?

Copy link
Contributor Author

@SpecificProtagonist SpecificProtagonist Feb 6, 2023

Choose a reason for hiding this comment

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

Does

The paths differ when an asset found at `to_reload` depends on data found at `to_watch`.

suffice?

Comment on lines 76 to 77
to_watch: &Path,
to_reload: PathBuf,
Copy link
Member

Choose a reason for hiding this comment

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

It seems they will often be the same path, could to_reload be an option in that case?

Copy link
Contributor Author

@SpecificProtagonist SpecificProtagonist Feb 6, 2023

Choose a reason for hiding this comment

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

Done 👍

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Example alien_cake_addict failed to run, please try running it locally and check the result.

@SpecificProtagonist
Copy link
Contributor Author

Example alien_cake_addict failed to run, please try running it locally and check the result.

Managed to miss the change in the custom_asset_io example, but I'm not sure what this is referring to. There are no alien_cake_addict errors in the github actions log and it runs fine locally.

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Feb 6, 2023
@cart
Copy link
Member

cart commented Mar 2, 2023

bors r+

bors bot pushed a commit that referenced this pull request Mar 2, 2023
# Objective

Fixes #6780

## Solution

- record the asset path of each watched file
- call `AssetIo::watch_for_changes` in `LoadContext::read_asset_bytes`

---

## Changelog

### Fixed
- fixed hot reloading for `LoadContext::read_asset_bytes`

### Changed
- `AssetIo::watch_path_for_changes` allows watched path and path to reload to differ

## Migration Guide
- for custom `AssetIo`s, differentiate paths to watch and asset paths to reload as a consequence

Co-authored-by: Vincent Junge <specificprotagonist@posteo.org>
@bors bors bot changed the title Fix hot reloading for read_asset_bytes [Merged by Bors] - Fix hot reloading for read_asset_bytes Mar 2, 2023
@bors bors bot closed this Mar 2, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
# Objective

Fixes bevyengine#6780

## Solution

- record the asset path of each watched file
- call `AssetIo::watch_for_changes` in `LoadContext::read_asset_bytes`

---

## Changelog

### Fixed
- fixed hot reloading for `LoadContext::read_asset_bytes`

### Changed
- `AssetIo::watch_path_for_changes` allows watched path and path to reload to differ

## Migration Guide
- for custom `AssetIo`s, differentiate paths to watch and asset paths to reload as a consequence

Co-authored-by: Vincent Junge <specificprotagonist@posteo.org>
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hot reloading doesn't trigger on files read via read_asset_bytes
4 participants