Skip to content

Commit

Permalink
Revert "allow asset loader pre-registration (bevyengine#9429)"
Browse files Browse the repository at this point in the history 
This reverts commit b30ff2a.
  • Loading branch information
@tguichaoua
tguichaoua committed Aug 17, 2023
1 parent b6a9d8e commit 131c2ae
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 138 deletions.
1 change: 0 additions & 1 deletion crates/bevy_asset/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ downcast-rs = "1.2.0"
fastrand = "1.7.0"
notify = { version = "6.0.0", optional = true }
parking_lot = "0.12.1"
async-channel = "1.4.2"

[target.'cfg(target_os = "android")'.dependencies]
bevy_winit = { path = "../bevy_winit", version = "0.12.0-dev" }
Expand Down
134 changes: 23 additions & 111 deletions crates/bevy_asset/src/asset_server.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,6 @@ pub(crate) struct AssetRefCounter {
pub(crate) mark_unused_assets: Arc<Mutex<Vec<HandleId>>>,
}

#[derive(Clone)]
enum MaybeAssetLoader {
Ready(Arc<dyn AssetLoader>),
Pending {
sender: async_channel::Sender<()>,
receiver: async_channel::Receiver<()>,
},
}

/// Internal data for the asset server.
///
/// [`AssetServer`] is the public API for interacting with the asset server.
Expand All @@ -79,7 +70,7 @@ pub struct AssetServerInternal {
pub(crate) asset_ref_counter: AssetRefCounter,
pub(crate) asset_sources: Arc<RwLock<HashMap<SourcePathId, SourceInfo>>>,
pub(crate) asset_lifecycles: Arc<RwLock<HashMap<Uuid, Box<dyn AssetLifecycle>>>>,
loaders: RwLock<Vec<MaybeAssetLoader>>,
loaders: RwLock<Vec<Arc<dyn AssetLoader>>>,
extension_to_loader_index: RwLock<HashMap<String, usize>>,
handle_to_path: Arc<RwLock<HashMap<HandleId, AssetPath<'static>>>>,
}
Expand Down Expand Up @@ -166,28 +157,6 @@ impl AssetServer {
Assets::new(self.server.asset_ref_counter.channel.sender.clone())
}

/// Pre-register a loader that will later be added.
///
/// Assets loaded with matching extensions will be blocked until the
/// real loader is added.
pub fn preregister_loader(&self, extensions: &[&str]) {
let mut loaders = self.server.loaders.write();
let loader_index = loaders.len();
for extension in extensions {
if self
.server
.extension_to_loader_index
.write()
.insert(extension.to_string(), loader_index)
.is_some()
{
warn!("duplicate preregistration for `{extension}`, any assets loaded with the previous loader will never complete.");
}
}
let (sender, receiver) = async_channel::bounded(1);
loaders.push(MaybeAssetLoader::Pending { sender, receiver });
}

/// Adds the provided asset loader to the server.
///
/// If `loader` has one or more supported extensions in conflict with loaders that came before
Expand All @@ -197,50 +166,14 @@ impl AssetServer {
T: AssetLoader,
{
let mut loaders = self.server.loaders.write();
let next_loader_index = loaders.len();
let mut maybe_existing_loader_index = None;
let mut loader_map = self.server.extension_to_loader_index.write();
let mut maybe_sender = None;

let loader_index = loaders.len();
for extension in loader.extensions() {
if let Some(&extension_index) = loader_map.get(*extension) {
// replacing an existing entry
match maybe_existing_loader_index {
None => {
match &loaders[extension_index] {
MaybeAssetLoader::Ready(_) => {
// replacing an existing loader, nothing special to do
}
MaybeAssetLoader::Pending { sender, .. } => {
// the loader was pre-registered, store the channel to notify pending assets
maybe_sender = Some(sender.clone());
}
}
}
Some(index) => {
// ensure the loader extensions are consistent
if index != extension_index {
warn!("inconsistent extensions between loader preregister_loader and add_loader, \
loading `{extension}` assets will never complete.");
}
}
}

maybe_existing_loader_index = Some(extension_index);
} else {
loader_map.insert(extension.to_string(), next_loader_index);
}
}

if let Some(existing_index) = maybe_existing_loader_index {
loaders[existing_index] = MaybeAssetLoader::Ready(Arc::new(loader));
if let Some(sender) = maybe_sender {
// notify after replacing the loader
let _ = sender.send_blocking(());
}
} else {
loaders.push(MaybeAssetLoader::Ready(Arc::new(loader)));
self.server
.extension_to_loader_index
.write()
.insert(extension.to_string(), loader_index);
}
loaders.push(Arc::new(loader));
}

/// Gets a strong handle for an asset with the provided id.
Expand All @@ -255,7 +188,7 @@ impl AssetServer {
HandleUntyped::strong(id.into(), sender)
}

fn get_asset_loader(&self, extension: &str) -> Result<MaybeAssetLoader, AssetServerError> {
fn get_asset_loader(&self, extension: &str) -> Result<Arc<dyn AssetLoader>, AssetServerError> {
let index = {
// scope map to drop lock as soon as possible
let map = self.server.extension_to_loader_index.read();
Expand All @@ -271,8 +204,7 @@ impl AssetServer {
fn get_path_asset_loader<P: AsRef<Path>>(
&self,
path: P,
include_pending: bool,
) -> Result<MaybeAssetLoader, AssetServerError> {
) -> Result<Arc<dyn AssetLoader>, AssetServerError> {
let s = path
.as_ref()
.file_name()
Expand All @@ -291,9 +223,7 @@ impl AssetServer {
ext = &ext[idx + 1..];
exts.push(ext);
if let Ok(loader) = self.get_asset_loader(ext) {
if include_pending || matches!(loader, MaybeAssetLoader::Ready(_)) {
return Ok(loader);
}
return Ok(loader);
}
}
Err(AssetServerError::MissingAssetLoader {
Expand Down Expand Up @@ -424,21 +354,12 @@ impl AssetServer {
};

// get the according asset loader
let mut maybe_asset_loader = self.get_path_asset_loader(asset_path.path(), true);

// if it's still pending, block until notified and refetch the new asset loader
if let Ok(MaybeAssetLoader::Pending { receiver, .. }) = maybe_asset_loader {
let _ = receiver.recv().await;
maybe_asset_loader = self.get_path_asset_loader(asset_path.path(), false);
}

let asset_loader = match maybe_asset_loader {
Ok(MaybeAssetLoader::Ready(loader)) => loader,
let asset_loader = match self.get_path_asset_loader(asset_path.path()) {
Ok(loader) => loader,
Err(err) => {
set_asset_failed();
return Err(err);
}
Ok(MaybeAssetLoader::Pending { .. }) => unreachable!(),
};

// load the asset bytes
Expand Down Expand Up @@ -571,7 +492,7 @@ impl AssetServer {
if self.asset_io().is_dir(&child_path) {
handles.extend(self.load_folder(&child_path)?);
} else {
if self.get_path_asset_loader(&child_path, true).is_err() {
if self.get_path_asset_loader(&child_path).is_err() {
continue;
}
let handle =
Expand Down Expand Up @@ -790,28 +711,23 @@ mod test {
let asset_server = setup(".");
asset_server.add_loader(FakePngLoader);

let Ok(MaybeAssetLoader::Ready(t)) = asset_server.get_path_asset_loader("test.png", true) else {
panic!();
};

assert_eq!(t.extensions()[0], "png");
let t = asset_server.get_path_asset_loader("test.png");
assert_eq!(t.unwrap().extensions()[0], "png");
}

#[test]
fn case_insensitive_extensions() {
let asset_server = setup(".");
asset_server.add_loader(FakePngLoader);

let Ok(MaybeAssetLoader::Ready(t)) = asset_server.get_path_asset_loader("test.PNG", true) else {
panic!();
};
assert_eq!(t.extensions()[0], "png");
let t = asset_server.get_path_asset_loader("test.PNG");
assert_eq!(t.unwrap().extensions()[0], "png");
}

#[test]
fn no_loader() {
let asset_server = setup(".");
let t = asset_server.get_path_asset_loader("test.pong", true);
let t = asset_server.get_path_asset_loader("test.pong");
assert!(t.is_err());
}

Expand All @@ -820,7 +736,7 @@ mod test {
let asset_server = setup(".");

assert!(
match asset_server.get_path_asset_loader("test.v1.2.3.pong", true) {
match asset_server.get_path_asset_loader("test.v1.2.3.pong") {
Err(AssetServerError::MissingAssetLoader { extensions }) =>
extensions == vec!["v1.2.3.pong", "2.3.pong", "3.pong", "pong"],
_ => false,
Expand Down Expand Up @@ -855,21 +771,17 @@ mod test {
let asset_server = setup(".");
asset_server.add_loader(FakePngLoader);

let Ok(MaybeAssetLoader::Ready(t)) = asset_server.get_path_asset_loader("test-v1.2.3.png", true) else {
panic!();
};
assert_eq!(t.extensions()[0], "png");
let t = asset_server.get_path_asset_loader("test-v1.2.3.png");
assert_eq!(t.unwrap().extensions()[0], "png");
}

#[test]
fn multiple_extensions() {
let asset_server = setup(".");
asset_server.add_loader(FakeMultipleDotLoader);

let Ok(MaybeAssetLoader::Ready(t)) = asset_server.get_path_asset_loader("test.test.png", true) else {
panic!();
};
assert_eq!(t.extensions()[0], "test.png");
let t = asset_server.get_path_asset_loader("test.test.png");
assert_eq!(t.unwrap().extensions()[0], "test.png");
}

fn create_dir_and_file(file: impl AsRef<Path>) -> tempfile::TempDir {
Expand Down
11 changes: 0 additions & 11 deletions crates/bevy_asset/src/assets.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,10 +317,6 @@ pub trait AddAsset {
fn add_asset_loader<T>(&mut self, loader: T) -> &mut Self
where
T: AssetLoader;

/// Preregisters a loader for the given extensions, that will block asset loads until a real loader
/// is registered.
fn preregister_asset_loader(&mut self, extensions: &[&str]) -> &mut Self;
}

impl AddAsset for App {
Expand Down Expand Up @@ -408,13 +404,6 @@ impl AddAsset for App {
self.world.resource_mut::<AssetServer>().add_loader(loader);
self
}

fn preregister_asset_loader(&mut self, extensions: &[&str]) -> &mut Self {
self.world
.resource_mut::<AssetServer>()
.preregister_loader(extensions);
self
}
}

/// Loads an internal asset from a project source file.
Expand Down
3 changes: 1 addition & 2 deletions crates/bevy_gltf/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ impl Plugin for GltfPlugin {
.add_asset::<Gltf>()
.add_asset::<GltfNode>()
.add_asset::<GltfPrimitive>()
.add_asset::<GltfMesh>()
.preregister_asset_loader(&["gltf", "glb"]);
.add_asset::<GltfMesh>();
}

fn finish(&self, app: &mut App) {
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_render/src/texture/image_texture_loader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub struct ImageTextureLoader {
supported_compressed_formats: CompressedImageFormats,
}

pub(crate) const IMG_FILE_EXTENSIONS: &[&str] = &[
const FILE_EXTENSIONS: &[&str] = &[
#[cfg(feature = "basis-universal")]
"basis",
#[cfg(feature = "bmp")]
Expand Down Expand Up @@ -73,7 +73,7 @@ impl AssetLoader for ImageTextureLoader {
}

fn extensions(&self) -> &[&str] {
IMG_FILE_EXTENSIONS
FILE_EXTENSIONS
}
}

Expand Down
11 changes: 0 additions & 11 deletions crates/bevy_render/src/texture/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,6 @@ impl Plugin for ImagePlugin {
update_texture_cache_system.in_set(RenderSet::Cleanup),
);
}

#[cfg(any(
feature = "png",
feature = "dds",
feature = "tga",
feature = "jpeg",
feature = "bmp",
feature = "basis-universal",
feature = "ktx2",
))]
app.preregister_asset_loader(IMG_FILE_EXTENSIONS);
}

fn finish(&self, app: &mut App) {
Expand Down

0 comments on commit 131c2ae

Please sign in to comment.