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.

Sign up for GitHub

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

Jump to bottom

Error info has been added to LoadState::Failed #12709

Merged
merged 6 commits into from
Apr 4, 2024
Merged

Error info has been added to LoadState::Failed #12709

merged 6 commits into from
Apr 4, 2024

Conversation

bugsweeper
Copy link
Contributor

@bugsweeper bugsweeper commented Mar 25, 2024 •
edited
Loading

Objective

Fixes #12667.

Solution

Migration guide

Added AssetLoadError to LoadState::Failed option
Removed Copy, Ord and PartialOrd implementations for LoadState enum
Added Eq and PartialEq implementations for MissingAssetSourceError, MissingProcessedAssetReaderError, DeserializeMetaError, LoadState, AssetLoadError, MissingAssetLoaderForTypeNameError and MissingAssetLoaderForTypeIdError

alice-i-cecile
alice-i-cecile requested changes Mar 25, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really pleased to see this! I think we should stick with the derived PartialEq and Eq implementations (and derive down the tree as needed), but otherwise I'm pleased.

We should also derive Debug and Error (via thiserror) while we're here.

The migration guide will need to cover the new variant, and the removed traits (Copy, Ord and PartialOrd).

crates/bevy_asset/src/server/mod.rs Outdated
}

impl PartialEq for LoadState {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want this: IMO we want to use the standard derived equality here. If users want this behavior it's easy for them to use it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Standard derived equality requires recursive equality implementations for all nested structures, should I add derived equality for all of them?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please :) Being able to check that the error matches the reason you expect is really valuable for writing tests, both internally and externally.

@alice-i-cecile alice-i-cecile 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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@bugsweeper
Copy link
Contributor Author

bugsweeper commented Mar 26, 2024 •
edited
Loading

I ran localy cargo run -p ci and got no errors
Looks like failed checks related to #12205 , but my code doesn't have relation to it, how can I resolve problem @alice-i-cecile ?

@bugsweeper
Copy link
Contributor Author

Sorry for unnecessary pinging, looks like we are waiting for #12730

@bugsweeper
Copy link
Contributor Author

@alice-i-cecile I made some changes after your review. And now all checks have passed. I hope you will find time for a feedback.

alice-i-cecile
alice-i-cecile reviewed Mar 27, 2024
View reviewed changes
crates/bevy_asset/src/server/mod.rs

impl PartialEq for AssetLoaderError {
#[inline]
fn eq(&self, other: &Self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

We should call out in a doc string for this implementation that we're only comparing the TypeId of the underlying error.

This is somewhat surprising, so we should mention it to save people time when debugging.

@alice-i-cecile
Copy link
Member

Two small notes, but we're almost there :)

@bugsweeper bugsweeper force-pushed the loadstate_improvement branch 2 times, most recently from f243e36 to d6761ed Compare March 27, 2024 21:50
@bugsweeper
Copy link
Contributor Author

@alice-i-cecile this PR is ready for your review

alice-i-cecile
alice-i-cecile approved these changes Apr 2, 2024
View reviewed changes
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.

Thank you very much!

cart
cart approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I'm on board for this!

crates/bevy_asset/src/server/mod.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Apr 2, 2024
@bugsweeper bugsweeper force-pushed the loadstate_improvement branch from d6761ed to b75a8e6 Compare April 3, 2024 07:53
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 4, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Apr 4, 2024
Merged via the queue into bevyengine:main with commit 4da4493 Apr 4, 2024
28 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Jun 3, 2024
Closed
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1297 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@cart cart cart approved these changes

Assignees
No one assigned
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact
Projects
None yet
Milestone
0.14
Development

Successfully merging this pull request may close these issues.

bevy_asset LoadState should contain more useful information in its Failed arm
3 participants
@bugsweeper @alice-i-cecile @cart