Remove incorrect equality comparisons for asset load error types #14428
Conversation
joseph-gio
added
C-Bug
alice-i-cecile
added
the
M-Needs-Migration-Guide
| // NOTE: an `is_not_loaded` method is intentionally not included, as it may mislead some users | ||
| // into thinking it is complementary to `is_loaded`. | ||
| // `NotLoaded` is a very specific failure mode and in most cases it is not necessary to directly check for it. | ||
| // If this is necessary the `matches!` macro can be used instead of a helper method. |
There was a problem hiding this comment.
Unrelated to this PR but it might be worth renaming the NotLoaded variant to Idle or something to help clarify the distinction on the enum itself.
There was a problem hiding this comment.
There might be a better name, but I like how NotLoaded avoids making any statements about the asset. If an asset handle is NotLoaded, it could mean that it hasn't started loading yet, or it could mean that there is no asset associated with the handle and never will be, or that the handle is invalid. A name like Idle implies that the asset does exist in some form which isn't necessarily true
There was a problem hiding this comment.
Oh good point, I hadn't considered that 🤔
| /// Returns `true` if the asset is loaded or in the process of being loaded. If true true, | ||
| /// then the asset can be considered to be in a "normal" state: the asset either exists | ||
| /// or will exist, and no errors have been encountered yet. | ||
| pub fn is_loaded_or_loading(&self) -> bool { | ||
| self.is_loaded() || self.is_loading() | ||
| } |
There was a problem hiding this comment.
Is this a common thing yo check for? I imagine you'd generally want to handle these two cases separately, right?
There was a problem hiding this comment.
I'm not tied to this method, I'd be okay with removing it. I just imagined that some users would want a quick way of checking if an asset is in a valid state or not, and allowing you to call is_loaded and is_loading in a single call allows you to avoid binding a LoadState to a local
alice-i-cecile
added
the
S-Needs-Review
alice-i-cecile
added
S-Ready-For-Final-Review
|
@JoJoJet if you resolve merge conflicts I'll get this in. |
alice-i-cecile
added
S-Adopt-Me
|
Adopted in #15890 |
Objective
The type
AssetLoadErrorhasPartialEqandEqimpls, which is problematic due to the fact that theAssetLoaderErrorandAddAsyncErrorvariants lie in their impls: they will returntruefor anyBox<dyn Error>with the sameTypeId, even if the actual value is different. This can lead to subtle bugs if a user relies on the equality comparison to ensure that two values are equal.More generally, it is an anti-pattern for large error types involving dynamic dispatch, such as
AssetLoadError, to have equality comparisons. Directly comparing two errors for equality is usually not desired -- if some logic needs to branch based on the value of an error, it is usually more correct to check for specific variants and inspect their fields.As far as I can tell, the only reason these errors have equality comparisons is because the
LoadStateenum wrapsAssetLoadErrorfor itsFailedvariant. This equality comparison is only used to check for== LoadState::Loaded, which we can easily replace with anis_loadedmethod.Solution
Remove the
{Partial}Eqimpls fromLoadState, which also allows us to remove it from the error types.Migration Guide
The types
bevy_asset::AssetLoadErrorandbevy_asset::LoadStateno longer support equality comparisons. If you need to check for an asset's load state, consider checking for a specific variant usingLoadState::is_loadedor thematches!macro. Similarly, consider using thematches!macro to check for specific variants of theAssetLoadErrortype if you need to inspect the value of an asset load error in your code.