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

Check asset-path existence. Previously App just crashed if not #345

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Check asset-path existence. Previously App just crashed if not #345

merged 4 commits into from
Aug 26, 2020

Conversation

Telzhaak
Copy link
Contributor

AssetServer.load_untyped does not check the existence of a provided path, only whether the path itself has a valid format (e.g. the file-extension).
This leads to the application crashing if the filename is missspelled (Fire <-> Fira):

let wrong_path = std::path::Path::new("assets/fonts/FireSans-Bold.ttf");
let correct_path = std::path::Path::new("assets/fonts/FiraSans-Bold.ttf");

let maybe_font = asset_server.load(wrong_path);
ERROR bevy_asset::loader > Failed to load asset: Io(Os { code: 2, kind: NotFound, message: "Das System kann die angegebene Datei nicht finden." })
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', E:\Workspaces\Rust\Bevy\clone\crates\bevy_text\src\font_atlas_set.rs:53:42

This might be intended/desired behaviour, such that the typo is fixed right away.


My Issue with this:

  1. The error-message does not provide information on which file could not be found, which makes it difficult to find the typo in case of a lot of assets.
  2. Crashing is ungraceful and pretty demotivating for newcomers

My Fix:

  1. Check existence and Return Error-Code with File-path (Much slower performance, so not feasible if the function is used in asset-streaming for large worlds?)
  2. User can choose to internally handle error by e.g. having fall-back assets (could also be used to allow modding by having a default_asset/ and mod_asset/ folder?)

@karroffel karroffel added A-Assets Load files from disk to use for things like images, models, and sounds P-Crash A sudden unexpected crash labels Aug 25, 2020
@cart
Copy link
Member

cart commented Aug 25, 2020

I think I would prefer to not do file io when queuing up load tasks. Instead, can we add path information to the error message in ChannelAssetHandler by extending AssetLoadError::Io?

@Telzhaak
Copy link
Contributor Author

Telzhaak commented Aug 26, 2020

You mean something like this?

The Original Io-Error is propagated, and the file-path is displayed in the error message.

ERROR bevy_asset::loader > Failed to load asset: Io(Custom { kind: NotFound, error: "assets/fonts/FireSans-Bold.ttf" })
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', E:\Workspaces\Rust\Bevy\clone\crates\bevy_text\src\font_atlas_set.rs:53:42

However, this still crashes the application.

ChannelAssetHandler::handle_request() is the only place the load_asset() function is used, which sends (apparently to font_atlas_set.rs in this case) an

AssetResult {
    handle: Handle::from(load_request.handle_id),
    result: load_asset(LoadRequest),
    path: load_request.path.clone(),
    version: load_request.version,
}

It look like the recipient of the message has to deal with the error (instead of using unwrap)?

@cart
Copy link
Member

cart commented Aug 26, 2020

Yeah thats what I was thinking of.

And yeah the other issue is your talking about is "how should we handle asset load failures"? I think crashing forcing developers to solve problems in their code, so its a reasonable short term solution.

But that is probably something that deserves its own issue / discussion. Allowing developers to define error handling behavior is maybe the right approach here.

@cart cart merged commit 93040ef into bevyengine:master Aug 26, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
…ngine#345)

* Check asset-path existence. Previously App just crashed if not

* rustfmt

* Relegated Error-message to ChannelAssetHandler

* Removed needless return statement
@DJMcNab
Copy link
Member

DJMcNab commented Jul 6, 2021

@Telzhaak please respond in #2373 for the relicense to MIT/Apache 2.0. Thanks!

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 P-Crash A sudden unexpected crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants