Skip to content

Don't panic in AssetPath deserialization#23935

Merged
alice-i-cecile merged 3 commits intobevyengine:mainfrom
greeble-dev:asset-path-serialize-panic
Apr 22, 2026
Merged

Don't panic in AssetPath deserialization#23935
alice-i-cecile merged 3 commits intobevyengine:mainfrom
greeble-dev:asset-path-serialize-panic

Conversation

@greeble-dev
Copy link
Copy Markdown
Contributor

Objective

AssetPath deserialization panics if the path is malformed. This seems a bit rude, and prevents the user from getting deserialization errors that would help track down the problem.

Solution

Instead of calling AssetPath::parse, call AssetPath::try_parse and handle any errors.

Now, calling ron::de::from_str on "a/b.test#" returns a useful error:

Err(SpannedError {
    code: Message(
        "Asset label must be at least one character. Either specify the label after the '#' or remove the '#'",
    ),
    span: Span {
        start: Position { line: 1, col: 2, },
        end: Position { line: 1, col: 12 },
    },
})

The PR also removes the visit_string method of the AssetPath deserializer. visit_string is an optimization that avoids copies when the deserializer can take ownership of the string (see serde docs). But AssetPath didn't take ownership of the string, so there's no reason to implement it.

Testing

cargo test -p bevy_asset
cargo run --example world_serialization

@greeble-dev greeble-dev added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 22, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Assets Apr 22, 2026
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 22, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 22, 2026
Merged via the queue into bevyengine:main with commit f794d8a Apr 22, 2026
49 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Assets Apr 22, 2026
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants