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.

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

Add directory related functions to AndroidAssetReader #11495

Merged
merged 13 commits into from
Oct 1, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 51 additions & 14 deletions crates/bevy_asset/src/io/android.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
use crate::io::{
get_meta_path, AssetReader, AssetReaderError, EmptyPathStream, PathStream, Reader, VecReader,
};
use alloc::ffi::CString;
use crate::io::{get_meta_path, AssetReader, AssetReaderError, PathStream, Reader, VecReader};
use bevy_utils::tracing::error;
use std::path::Path;
use futures_lite::stream;
use std::{ffi::CString, path::Path};

/// [`AssetReader`] implementation for Android devices, built on top of Android's [`AssetManager`].
///
/// Implementation details:
///
/// - [`load_path`](AssetIo::load_path) uses the [`AssetManager`] to load files.
/// - [`read_directory`](AssetIo::read_directory) always returns an empty iterator.
/// - All functions use the [`AssetManager`] to load files.
/// - [`is_directory`](AssetReader::is_directory) tries to open the path
/// as a normal file and treats an error as if the path is a directory.
/// - Watching for changes is not supported. The watcher method will do nothing.
///
/// [AssetManager]: https://developer.android.com/reference/android/content/res/AssetManager
Expand Down Expand Up @@ -46,15 +45,53 @@ impl AssetReader for AndroidAssetReader {

async fn read_directory<'a>(
&'a self,
_path: &'a Path,
path: &'a Path,
) -> Result<Box<PathStream>, AssetReaderError> {
let stream: Box<PathStream> = Box::new(EmptyPathStream);
error!("Reading directories is not supported with the AndroidAssetReader");
Ok(stream)
let asset_manager = bevy_winit::ANDROID_APP
.get()
.expect("Bevy must be setup with the #[bevy_main] macro on Android")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this expectation to the documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a message that is also used in bevy_winit, so it is just reused here. I agree it would be good to add to the docs. I can add it separately to the android page in the book (which is currently hidden, but I guess it is good to have it there so it is not forgotten once it gets published).

.asset_manager();
let opened_assets_dir = asset_manager
.open_dir(&CString::new(path.to_str().unwrap()).unwrap())
.ok_or(AssetReaderError::NotFound(path.to_path_buf()))?;

let mapped_stream = opened_assets_dir
.filter_map(move |f| {
let file_path = path.join(Path::new(f.to_str().unwrap()));
// filter out meta files as they are not considered assets
if let Some(ext) = file_path.extension().and_then(|e| e.to_str()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

.as_asset_file() or something could probably be its own function since that seems useful to have generally speaking.

Copy link
Contributor Author

@esensar esensar Sep 30, 2024

Choose a reason for hiding this comment

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

I agree, but I think it might be better to just extract out the "meta" file detection and filtering. I am worried that something like .as_asset_file() might be confusing (since it would take a file path and then either return Some or None, depending on this condition - and that Some value would be the original value unchanged).

Do you think that would make sense?

if ext.eq_ignore_ascii_case("meta") {
return None;
}
}
Some(file_path.to_owned())
})
.collect::<Vec<_>>();

let read_dir: Box<PathStream> = Box::new(stream::iter(mapped_stream));
Ok(read_dir)
}

async fn is_directory<'a>(&'a self, _path: &'a Path) -> Result<bool, AssetReaderError> {
error!("Reading directories is not supported with the AndroidAssetReader");
Ok(false)
async fn is_directory<'a>(
&'a self,
path: &'a Path,
) -> std::result::Result<bool, AssetReaderError> {
let asset_manager = bevy_winit::ANDROID_APP
.get()
.expect("Bevy must be setup with the #[bevy_main] macro on Android")
.asset_manager();
// HACK: `AssetManager` does not provide a way to check if path
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

// points to a directory or a file
// `open_dir` succeeds for both files and directories and will only
// fail if the path does not exist at all
// `open` will fail for directories, but it will work for files
// The solution here was to first use `open_dir` to eliminate the case
// when the path does not exist at all, and then to use `open` to
// see if that path is a file or a directory
let cpath = CString::new(path.to_str().unwrap()).unwrap();
let _ = asset_manager
.open_dir(&cpath)
.ok_or(AssetReaderError::NotFound(path.to_path_buf()))?;
Ok(asset_manager.open(&cpath).is_none())
}
}