Skip to content

Remove incorrect equality comparisons for asset load error types #14428

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

Closed
wants to merge 14 commits into from
34 changes: 17 additions & 17 deletions crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ mod tests {
let a_text = get::<CoolText>(app.world(), a_id);
let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap();
assert!(a_text.is_none(), "a's asset should not exist yet");
assert_eq!(a_load, LoadState::Loading, "a should still be loading");
assert!(a_load.is_loading(), "a should still be loading");
assert_eq!(
a_deps,
DependencyLoadState::Loading,
Expand All @@ -759,23 +759,23 @@ mod tests {
let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap();
assert_eq!(a_text.text, "a");
assert_eq!(a_text.dependencies.len(), 2);
assert_eq!(a_load, LoadState::Loaded, "a is loaded");
assert!(a_load.is_loaded(), "a is loaded");
assert_eq!(a_deps, DependencyLoadState::Loading);
assert_eq!(a_rec_deps, RecursiveDependencyLoadState::Loading);

let b_id = a_text.dependencies[0].id();
let b_text = get::<CoolText>(world, b_id);
let (b_load, b_deps, b_rec_deps) = asset_server.get_load_states(b_id).unwrap();
assert!(b_text.is_none(), "b component should not exist yet");
assert_eq!(b_load, LoadState::Loading);
assert!(b_load.is_loading());
assert_eq!(b_deps, DependencyLoadState::Loading);
assert_eq!(b_rec_deps, RecursiveDependencyLoadState::Loading);

let c_id = a_text.dependencies[1].id();
let c_text = get::<CoolText>(world, c_id);
let (c_load, c_deps, c_rec_deps) = asset_server.get_load_states(c_id).unwrap();
assert!(c_text.is_none(), "c component should not exist yet");
assert_eq!(c_load, LoadState::Loading);
assert!(c_load.is_loading());
assert_eq!(c_deps, DependencyLoadState::Loading);
assert_eq!(c_rec_deps, RecursiveDependencyLoadState::Loading);
Some(())
Expand All @@ -789,23 +789,23 @@ mod tests {
let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap();
assert_eq!(a_text.text, "a");
assert_eq!(a_text.dependencies.len(), 2);
assert_eq!(a_load, LoadState::Loaded);
assert!(a_load.is_loaded());
assert_eq!(a_deps, DependencyLoadState::Loading);
assert_eq!(a_rec_deps, RecursiveDependencyLoadState::Loading);

let b_id = a_text.dependencies[0].id();
let b_text = get::<CoolText>(world, b_id)?;
let (b_load, b_deps, b_rec_deps) = asset_server.get_load_states(b_id).unwrap();
assert_eq!(b_text.text, "b");
assert_eq!(b_load, LoadState::Loaded);
assert!(b_load.is_loaded());
assert_eq!(b_deps, DependencyLoadState::Loaded);
assert_eq!(b_rec_deps, RecursiveDependencyLoadState::Loaded);

let c_id = a_text.dependencies[1].id();
let c_text = get::<CoolText>(world, c_id);
let (c_load, c_deps, c_rec_deps) = asset_server.get_load_states(c_id).unwrap();
assert!(c_text.is_none(), "c component should not exist yet");
assert_eq!(c_load, LoadState::Loading);
assert!(c_load.is_loading());
assert_eq!(c_deps, DependencyLoadState::Loading);
assert_eq!(c_rec_deps, RecursiveDependencyLoadState::Loading);
Some(())
Expand All @@ -824,14 +824,14 @@ mod tests {
assert_eq!(a_text.text, "a");
assert_eq!(a_text.embedded, "");
assert_eq!(a_text.dependencies.len(), 2);
assert_eq!(a_load, LoadState::Loaded);
assert!(a_load.is_loaded());

let b_id = a_text.dependencies[0].id();
let b_text = get::<CoolText>(world, b_id)?;
let (b_load, b_deps, b_rec_deps) = asset_server.get_load_states(b_id).unwrap();
assert_eq!(b_text.text, "b");
assert_eq!(b_text.embedded, "");
assert_eq!(b_load, LoadState::Loaded);
assert!(b_load.is_loaded());
assert_eq!(b_deps, DependencyLoadState::Loaded);
assert_eq!(b_rec_deps, RecursiveDependencyLoadState::Loaded);

Expand All @@ -840,7 +840,7 @@ mod tests {
let (c_load, c_deps, c_rec_deps) = asset_server.get_load_states(c_id).unwrap();
assert_eq!(c_text.text, "c");
assert_eq!(c_text.embedded, "ab");
assert_eq!(c_load, LoadState::Loaded);
assert!(c_load.is_loaded());
assert_eq!(
c_deps,
DependencyLoadState::Loading,
Expand All @@ -858,15 +858,15 @@ mod tests {
assert_eq!(sub_text.text, "hello");
let (sub_text_load, sub_text_deps, sub_text_rec_deps) =
asset_server.get_load_states(sub_text_id).unwrap();
assert_eq!(sub_text_load, LoadState::Loaded);
assert!(sub_text_load.is_loaded());
assert_eq!(sub_text_deps, DependencyLoadState::Loaded);
assert_eq!(sub_text_rec_deps, RecursiveDependencyLoadState::Loaded);

let d_id = c_text.dependencies[0].id();
let d_text = get::<CoolText>(world, d_id);
let (d_load, d_deps, d_rec_deps) = asset_server.get_load_states(d_id).unwrap();
assert!(d_text.is_none(), "d component should not exist yet");
assert_eq!(d_load, LoadState::Loading);
assert!(d_load.is_loading());
assert_eq!(d_deps, DependencyLoadState::Loading);
assert_eq!(d_rec_deps, RecursiveDependencyLoadState::Loading);

Expand Down Expand Up @@ -900,11 +900,11 @@ mod tests {
assert_eq!(d_text.text, "d");
assert_eq!(d_text.embedded, "");

assert_eq!(c_load, LoadState::Loaded);
assert!(c_load.is_loaded());
assert_eq!(c_deps, DependencyLoadState::Loaded);
assert_eq!(c_rec_deps, RecursiveDependencyLoadState::Loaded);

assert_eq!(d_load, LoadState::Loaded);
assert!(d_load.is_loaded());
assert_eq!(d_deps, DependencyLoadState::Loaded);
assert_eq!(d_rec_deps, RecursiveDependencyLoadState::Loaded);

Expand Down Expand Up @@ -1089,17 +1089,17 @@ mod tests {
assert_eq!(d_rec_deps, RecursiveDependencyLoadState::Failed);

assert_eq!(a_text.text, "a");
assert_eq!(a_load, LoadState::Loaded);
assert!(a_load.is_loaded());
assert_eq!(a_deps, DependencyLoadState::Loaded);
assert_eq!(a_rec_deps, RecursiveDependencyLoadState::Failed);

assert_eq!(b_text.text, "b");
assert_eq!(b_load, LoadState::Loaded);
assert!(b_load.is_loaded());
assert_eq!(b_deps, DependencyLoadState::Loaded);
assert_eq!(b_rec_deps, RecursiveDependencyLoadState::Loaded);

assert_eq!(c_text.text, "c");
assert_eq!(c_load, LoadState::Loaded);
assert!(c_load.is_loaded());
assert_eq!(c_deps, DependencyLoadState::Failed);
assert_eq!(c_rec_deps, RecursiveDependencyLoadState::Failed);

Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_asset/src/server/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ impl AssetInfos {
info.loading_rec_dependencies.remove(&loaded_id);
if info.loading_rec_dependencies.is_empty() && info.failed_rec_dependencies.is_empty() {
info.rec_dep_load_state = RecursiveDependencyLoadState::Loaded;
if info.load_state == LoadState::Loaded {
if info.load_state.is_loaded() {
sender
.send(InternalAssetEvent::LoadedWithDependencies { id: waiting_id })
.unwrap();
Expand Down
57 changes: 31 additions & 26 deletions crates/bevy_asset/src/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use info::*;
use loaders::*;
use parking_lot::RwLock;
use std::future::Future;
use std::{any::Any, path::PathBuf};
use std::path::PathBuf;
use std::{any::TypeId, path::Path, sync::Arc};
use thiserror::Error;

Expand Down Expand Up @@ -930,7 +930,7 @@ impl AssetServer {
/// Returns true if the asset and all of its dependencies (recursive) have been loaded.
pub fn is_loaded_with_dependencies(&self, id: impl Into<UntypedAssetId>) -> bool {
let id = id.into();
self.load_state(id) == LoadState::Loaded
self.load_state(id).is_loaded()
&& self.recursive_dependency_load_state(id) == RecursiveDependencyLoadState::Loaded
}

Expand Down Expand Up @@ -1327,7 +1327,7 @@ pub(crate) enum InternalAssetEvent {
}

/// The load state of an asset.
#[derive(Component, Clone, Debug, PartialEq, Eq)]
#[derive(Component, Clone, Debug)]
pub enum LoadState {
/// The asset has not started loading yet
NotLoaded,
Expand All @@ -1339,6 +1339,33 @@ pub enum LoadState {
Failed(Box<AssetLoadError>),
}

impl LoadState {
/// Returns `true` if this instance is [`LoadState::Loading`]
pub fn is_loading(&self) -> bool {
matches!(self, Self::Loading)
}
/// Returns `true` if this instance is [`LoadState::Loaded`]
pub fn is_loaded(&self) -> bool {
matches!(self, Self::Loaded)
}
// 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.
Comment on lines +1351 to +1354
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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()
}
Comment on lines +1356 to +1361
Copy link
Member

Choose a reason for hiding this comment

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

Is this a common thing yo check for? I imagine you'd generally want to handle these two cases separately, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

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


/// Returns `true` if this instance is [`LoadState::Failed`]
pub fn is_failed(&self) -> bool {
matches!(self, Self::Failed(_))
}
}

/// The load state of an asset's dependencies.
#[derive(Component, Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)]
pub enum DependencyLoadState {
Expand Down Expand Up @@ -1366,7 +1393,7 @@ pub enum RecursiveDependencyLoadState {
}

/// An error that occurs during an [`Asset`] load.
#[derive(Error, Debug, Clone, PartialEq, Eq)]
#[derive(Error, Debug, Clone)]
pub enum AssetLoadError {
#[error("Requested handle of type {requested:?} for asset '{path}' does not match actual asset type '{actual_asset_name}', which used loader '{loader_name}'")]
RequestedHandleTypeMismatch {
Expand Down Expand Up @@ -1429,18 +1456,6 @@ pub struct AssetLoaderError {
error: Arc<dyn std::error::Error + Send + Sync + 'static>,
}

impl PartialEq for AssetLoaderError {
/// Equality comparison for `AssetLoaderError::error` is not full (only through `TypeId`)
#[inline]
fn eq(&self, other: &Self) -> bool {
self.path == other.path
&& self.loader_name == other.loader_name
&& self.error.type_id() == other.error.type_id()
}
}

impl Eq for AssetLoaderError {}

impl AssetLoaderError {
pub fn path(&self) -> &AssetPath<'static> {
&self.path
Expand All @@ -1453,16 +1468,6 @@ pub struct AddAsyncError {
error: Arc<dyn std::error::Error + Send + Sync + 'static>,
}

impl PartialEq for AddAsyncError {
/// Equality comparison is not full (only through `TypeId`)
#[inline]
fn eq(&self, other: &Self) -> bool {
self.error.type_id() == other.error.type_id()
}
}

impl Eq for AddAsyncError {}

/// An error that occurs when an [`AssetLoader`] is not registered for a given extension.
#[derive(Error, Debug, Clone, PartialEq, Eq)]
#[error("no `AssetLoader` found{}", format_missing_asset_ext(.extensions))]
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_gltf/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2063,7 +2063,7 @@ mod test {
app.update();
run_app_until(&mut app, |_world| {
let load_state = asset_server.get_load_state(handle_id).unwrap();
if load_state == LoadState::Loaded {
if load_state.is_loaded() {
Some(())
} else {
None
Expand Down
10 changes: 7 additions & 3 deletions examples/3d/pbr.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! This example shows how to configure Physically Based Rendering (PBR) parameters.

use bevy::{asset::LoadState, prelude::*};
use bevy::prelude::*;

fn main() {
App::new()
Expand Down Expand Up @@ -142,8 +142,12 @@ fn environment_map_load_finish(
label_query: Query<Entity, With<EnvironmentMapLabel>>,
) {
if let Ok(environment_map) = environment_maps.get_single() {
if asset_server.load_state(&environment_map.diffuse_map) == LoadState::Loaded
&& asset_server.load_state(&environment_map.specular_map) == LoadState::Loaded
if asset_server
.load_state(&environment_map.diffuse_map)
.is_loaded()
&& asset_server
.load_state(&environment_map.specular_map)
.is_loaded()
{
if let Ok(label_entity) = label_query.get_single() {
commands.entity(label_entity).despawn();
Expand Down
3 changes: 1 addition & 2 deletions examples/3d/skybox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
mod camera_controller;

use bevy::{
asset::LoadState,
core_pipeline::Skybox,
prelude::*,
render::{
Expand Down Expand Up @@ -147,7 +146,7 @@ fn asset_loaded(
mut cubemap: ResMut<Cubemap>,
mut skyboxes: Query<&mut Skybox>,
) {
if !cubemap.is_loaded && asset_server.load_state(&cubemap.image_handle) == LoadState::Loaded {
if !cubemap.is_loaded && asset_server.load_state(&cubemap.image_handle).is_loaded() {
info!("Swapping to {}...", CUBEMAPS[cubemap.index].0);
let image = images.get_mut(&cubemap.image_handle).unwrap();
// NOTE: PNGs do not have any metadata that could indicate they contain a cubemap texture,
Expand Down
5 changes: 3 additions & 2 deletions examples/shader/array_texture.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
//! uniform variable.

use bevy::{
asset::LoadState,
prelude::*,
reflect::TypePath,
render::render_resource::{AsBindGroup, ShaderRef},
Expand Down Expand Up @@ -57,7 +56,9 @@ fn create_array_texture(
mut materials: ResMut<Assets<ArrayTextureMaterial>>,
) {
if loading_texture.is_loaded
|| asset_server.load_state(loading_texture.handle.id()) != LoadState::Loaded
|| !asset_server
.load_state(loading_texture.handle.id())
.is_loaded()
{
return;
}
Expand Down
8 changes: 5 additions & 3 deletions examples/tools/scene_viewer/scene_viewer_plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
//! - Insert an initialized `SceneHandle` resource into your App's `AssetServer`.

use bevy::{
asset::LoadState, gltf::Gltf, input::common_conditions::input_just_pressed, prelude::*,
scene::InstanceId,
gltf::Gltf, input::common_conditions::input_just_pressed, prelude::*, scene::InstanceId,
};

use std::f32::consts::*;
Expand Down Expand Up @@ -92,7 +91,10 @@ fn scene_load_check(
) {
match scene_handle.instance_id {
None => {
if asset_server.load_state(&scene_handle.gltf_handle) == LoadState::Loaded {
if asset_server
.load_state(&scene_handle.gltf_handle)
.is_loaded()
{
let gltf = gltf_assets.get(&scene_handle.gltf_handle).unwrap();
if gltf.scenes.len() > 1 {
info!(
Expand Down