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

Added Support for Extension-less Assets #10153

Merged

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Oct 17, 2023

Objective

Solution

AssetServer::load::<A> and AssetServer::load_with_settings::<A> can now use the Asset type parameter A to select a registered AssetLoader without inspecting the provided AssetPath. This change cascades onto LoadContext::load and LoadContext::load_with_settings. This allows the loading of assets which have incorrect or ambiguous file extensions.

// Allow the type to be inferred by context
let handle = asset_server.load("data/asset_no_extension");

// Hint the type through the handle
let handle: Handle<CustomAsset> = asset_server.load("data/asset_no_extension");

// Explicit through turbofish
let handle = asset_server.load::<CustomAsset>("data/asset_no_extension");

Since a single AssetPath no longer maps 1:1 with an Asset, I've also modified how assets are loaded to permit multiple asset types to be loaded from a single path. This allows for two different AssetLoaders (which return different types of assets) to both load a single path (if requested).

// Uses GltfLoader
let model = asset_server.load::<Gltf>("cube.gltf");

// Hypothetical Blob loader for data transmission (for example)
let blob = asset_server.load::<Blob>("cube.gltf");

As these changes are reflected in the LoadContext as well as the AssetServer, custom AssetLoaders can also take advantage of this behaviour to create more complex assets.


Change Log

  • Updated custom_asset example to demonstrate extension-less assets.
  • Added AssetServer::get_handles_untyped and Added AssetServer::get_path_ids

Notes

As a part of that refactor, I chose to store AssetLoaders (within AssetLoaders) using a HashMap<TypeId, ...> instead of a Vec<...>. My reasoning for this was I needed to add a relationship between Asset TypeIds and the AssetLoader, so instead of having a Vec and a HashMap, I combined the two, removing the usize index from the adjacent maps.

* Refactored `AssetLoaders` into its own module to decouple it from `AssetServer`.
* Added using `TypeId` from `Handle` as a hint for selecting an `AssetLoader`. This is tried before relying on file extensions.
* Updated `custom_asset` example to demonstrate extension-less assets.
@bushrat011899 bushrat011899 marked this pull request as draft October 17, 2023 01:33
@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Assets Load files from disk to use for things like images, models, and sounds labels Oct 17, 2023
@alice-i-cecile
Copy link
Member

I chose to refactor AssetLoaders into its own module because I was having trouble adding support for Asset TypeId information to it due to how tightly coupled it was to AssetServer. This wasn't strictly required, but I believe this is a better state for these types to be in, and should make it easier to update in the future.

I like this refactor! Decoupling that is a good step, and it's small enough that I don't feel terrible doing it in the same PR.

Because I believe type information is of a higher priority than file extension, I have chosen to ignore the extension unless required (loading without supplying a TypeId). This allows for loading different types of assets from files with the same extension:

Oh this is extremely useful: I've run into this before and had to do bizarre .foo.json naming schemes. In the future, would we be able to get hints from metadata in the file itself? Definitely agree that type info > file extension though.

As a part of that refactor, I chose to store AssetLoaders (within AssetLoaders) using a HashMap<TypeId, ...> instead of a Vec<...>. My reasoning for this was I needed to add a relationship between Asset TypeIds and the AssetLoader, so instead of having a Vec and a HashMap, I combined the two, removing the usize index from the adjacent maps.

Thanks for calling this out: it's a solid choice.

bushrat011899 and others added 3 commits October 17, 2023 12:42
If a provided `TypeId` is not found in the `AssetLoaders`, fall back to file extensions.
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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.

The "disambiguate between assets of the same file extension via type annotations" bit of this PR is wildly useful: can you add / extend an example for it?

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Oct 17, 2023
Also added support for multiple assets using the same path as long as they are distinct asset types.
@bushrat011899
Copy link
Contributor Author

The "disambiguate between assets of the same file extension via type annotations" bit of this PR is wildly useful: can you add / extend an example for it?

I've updated the custom_asset example to demonstrate the creation of a Blob asset loader, which just simply reads the bytes of a file and stores them in a Vec<u8>:

#[derive(Asset, TypePath, Debug)]
pub struct Blob {
    pub bytes: Vec<u8>,
}

This allows loading the same path multiple times, provided they are for distinct asset types:

#[derive(Resource, Default)]
struct State {
    handle: Handle<CustomAsset>,
    other_handle: Handle<CustomAsset>,
    blob: Handle<Blob>,
    printed: bool,
}

fn setup(mut state: ResMut<State>, asset_server: Res<AssetServer>) {
    // Recommended way to load an asset
    state.handle = asset_server.load("data/asset.custom");

    // File extensions are optional, but are recommended
    state.other_handle = asset_server.load("data/asset_no_extension");

    // Will use BlobAssetLoader instead of CustomAssetLoader thanks to type inference
    state.blob = asset_server.load("data/asset.custom");
}

In the example, I also show that you can create a loader with no defined extensions:

impl AssetLoader for BlobAssetLoader {
    type Asset = Blob;
    type Settings = ();
    type Error = BlobAssetLoaderError;

    fn load<'a>(/* snip */) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
        /* snip */
    }

    fn extensions(&self) -> &[&str] {
        // This loader won't be used for any files by default
        &[]
    }
}

This blob loader I think is a good example of where you might want that behaviour; you have an asset which you want to load, but it's not the default way you would interact with that particular file. It would be possible to now provide a default implementation for AssetLoader::extensions, defaulting to this behaviour. However, I haven't done that, as I believe for most users most of the time, they do want to declare a file type (and extension) for their asset.

@LennysLounge
Copy link

@alice-i-cecile bump for review.

@alice-i-cecile
Copy link
Member

So @bushrat011899, extending on my proposed use case a bit.

  1. I have multiple types which should be stored as a .json file.
  2. I want to parse the json file to determine what Rust type to load this in as, and then add it to a central collection.

Is that feasible with this PR?

That said, this example seems great to me, and I'm happy to approve this PR as is.

@bushrat011899
Copy link
Contributor Author

bushrat011899 commented Nov 16, 2023

So @bushrat011899, extending on my proposed use case a bit.

1. I have multiple types which should be stored as a .json file.

2. I want to parse the json file to determine what Rust type to load this in as, and then add it to a central collection.

Is that feasible with this PR?

@alice-i-cecile Yes that should be entirely possible:

  1. First you'd create a plain JSON AssetLoader to give you the parsed contents to inspect.
// Handle<JsonAsset> implied by .json file extension if
// `JsonAssetLoader` is configured to be default handler of .json
let my_data_json_handle = asset_server.load("data/my_data.json");
  1. Once loaded, you can then inspect the JSON data for whatever information you'd need to determine the type it should be:
let my_data_json = json_assets.get(&my_data_json_handle)?;

let serde_json::Value::String(magic_token) = my_data_json.get("magic_token")? else {
    return None;
};

// Can re-use the path value for DRY reasons
let path = my_data_json_handle.path().unwrap();

// All match arms need to have the same return type, using UntypedHandle to avoid an enum
let my_data_handle = match magic_token {
    "key_bindings" => asset_server.load::<KeyBindingsAsset>(&path).untyped(),
    "settings" => asset_server.load::<SettingsAsset>(&path).untyped(),
    "scoreboard" => asset_server.load::<ScoreboardAsset>(&path).untyped(),
    _ => my_data_json_handle.untyped(),
};

Some(my_data_handle)

I imagine you'd want an enum for the central collection, and that would probably be the type of my_data_handle in the above match statement instead of using UntypedHandle's. Additionally, this behaviour is extended to the LoadContext::load method, so you could put all of this functionality into a custom AssetLoader if the load_with_reader method @nicopap suggested in #10565 was added (would need another PR to add).

@alice-i-cecile
Copy link
Member

Awesome, I'll spin out a follow-up issue for that.

@nicopap nicopap self-requested a review November 17, 2023 10:36
@alice-i-cecile alice-i-cecile changed the title Added Support for Extenstion-less Assets Added Support for Extension-less Assets Jan 24, 2024
@bushrat011899
Copy link
Contributor Author

Sorry about the radio silence everyone, I had an unusually lovely holiday break over the new year, but I'm back now and wanting to sort out some of these outstanding PRs from last year.

I doubt this will be approved before the 0.13 merge window closes, but I've upstreamed this branch to the latest version of main and resolved all the merge conflicts that produced (of which there were enough that this was basically a full re-write! You guys were busy!)

I've also narrowed the scope of this PR to try and help spur its adoption:

  1. There are no breaking changes in this PR now. All existing public APIs are unchanged in both signature and behaviour.
  2. More specialised functions I added (like load_direct_from_reader_with_type_id) have been removed. I do think they are useful, but they aren't required to deliver on the key features of this PR.
  3. I've removed the refactor I did (loaders.rs), since it nearly doubled the size of this PR on its own, and probably didn't help with the review process.

To focus on the core functionality of this PR:

  1. Assets can now be loaded even if their path is missing an extension using type inference at the call site.
  2. The same path can now be loaded as different asset types, again, via type inference at the loading call site.
  3. AssetLoader's no longer need to define extensions if they are not to be used as a default loader. Instead, they can omit the extensions method in the trait definition entirely.

Thanks again to everyone who reviewed this PR over the last couple months while I was away! ❤️

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.

Welcome back! I'm happy with this, and pleased to see that there's now no breaking changes.

Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Looks great, this is really high quality stuff! I didn't see any issues, but I am not submitting an approval because I still feel a little hazy about the asset server internals.

crates/bevy_asset/src/server/info.rs Show resolved Hide resolved
@torsteingrindvik
Copy link
Contributor

Not sure if okay as-is or not, worth mentioning:

I tried having a CustomAsset and a CustomAssetLoader like your example, which works nicely.

However, if I try loading a CustomAsset from a file named e.g. foo.ron, I get an error that no .ron loader is added.
Loading it from foo (no extension) works.

So I suppose the question is why require listing the supported extension(s) in the loader trait impl at all, when type inference can help out?

@torsteingrindvik
Copy link
Contributor

torsteingrindvik commented Jan 31, 2024

I made an example which breaks and I think it shouldn't.

EDIT: The example tries using two custom extension-less loaders.

Example
// my-asset
RonAsset (
    name: "Cube 1",
    translation: (0.1, 0.9, 2.4)
)
// my-asset-2
RonAsset2 (
    name: "Cube 2",
    translation: (0.8, 0.3, 0.9)
)
use bevy::{
  asset::{io::Reader, ron, AssetLoader, AsyncReadExt, LoadContext},
  prelude::*,
  reflect::TypePath,
  utils::{thiserror, thiserror::Error, BoxedFuture},
};
use serde::Deserialize;

#[derive(Asset, TypePath, Debug, Deserialize, Component)]
pub struct RonAsset {
  pub name: String,
  pub translation: Vec3,
}

#[derive(Asset, TypePath, Debug, Deserialize, Component)]
pub struct RonAsset2 {
  pub name: String,
  pub translation: Vec3,
}

#[derive(Default)]
pub struct RonLoader;

#[derive(Default)]
pub struct RonLoader2;

#[non_exhaustive]
#[derive(Debug, Error)]
pub enum RonLoaderError {
  /// An [IO](std::io) Error
  #[error("Could not load asset: {0}")]
  Io(#[from] std::io::Error),
  /// A [RON](ron) Error
  #[error("Could not parse RON: {0}")]
  RonSpannedError(#[from] ron::error::SpannedError),
}

impl AssetLoader for RonLoader {
  type Asset = RonAsset;
  type Settings = ();
  type Error = RonLoaderError;
  fn load<'a>(
      &'a self,
      reader: &'a mut Reader,
      _settings: &'a (),
      _load_context: &'a mut LoadContext,
  ) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
      Box::pin(async move {
          let mut bytes = Vec::new();
          reader.read_to_end(&mut bytes).await?;
          let custom_asset = ron::de::from_bytes::<RonAsset>(&bytes)?;
          Ok(custom_asset)
      })
  }
}

impl AssetLoader for RonLoader2 {
  type Asset = RonAsset2;
  type Settings = ();
  type Error = RonLoaderError;
  fn load<'a>(
      &'a self,
      reader: &'a mut Reader,
      _settings: &'a (),
      _load_context: &'a mut LoadContext,
  ) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
      Box::pin(async move {
          let mut bytes = Vec::new();
          reader.read_to_end(&mut bytes).await?;
          let custom_asset = ron::de::from_bytes::<RonAsset2>(&bytes)?;
          Ok(custom_asset)
      })
  }
}

fn main() {
  App::new()
      .add_plugins(DefaultPlugins)
      .init_asset::<RonAsset>()
      .init_asset_loader::<RonLoader>()
      .init_asset::<RonAsset2>()
      .init_asset_loader::<RonLoader2>()
      .add_systems(Startup, setup)
      .add_systems(Update, (set_cube_translation, set_cube_translation2))
      .run();
}

#[derive(Debug, Component)]
struct Cube;

fn setup(
  mut commands: Commands,
  mut meshes: ResMut<Assets<Mesh>>,
  mut materials: ResMut<Assets<StandardMaterial>>,
  asset_server: Res<AssetServer>,
) {
  // circular base
  commands.spawn(PbrBundle {
      mesh: meshes.add(shape::Circle::new(4.0)),
      material: materials.add(Color::WHITE),
      transform: Transform::from_rotation(Quat::from_rotation_x(-std::f32::consts::FRAC_PI_2)),
      ..default()
  });

  // cube 1
  commands.spawn((
      PbrBundle {
          mesh: meshes.add(shape::Cube { size: 1.0 }),
          material: materials.add(Color::rgb_u8(124, 144, 255)),
          ..default()
      },
      asset_server.load::<RonAsset>("data/my-asset"),
  ));

  // cube 2
  commands.spawn((
      PbrBundle {
          mesh: meshes.add(shape::Cube { size: 1.0 }),
          material: materials.add(Color::rgb_u8(124, 244, 255)),
          ..default()
      },
      asset_server.load::<RonAsset2>("data/my-asset-2"),
  ));

  // light
  commands.spawn(PointLightBundle {
      point_light: PointLight {
          intensity: 250_000.0,
          shadows_enabled: true,
          ..default()
      },
      transform: Transform::from_xyz(4.0, 8.0, 4.0),
      ..default()
  });
  // camera
  commands.spawn(Camera3dBundle {
      transform: Transform::from_xyz(-2.5, 4.5, 9.0).looking_at(Vec3::ZERO, Vec3::Y),
      ..default()
  });
}

fn set_cube_translation(
  mut transform_via_asset: Query<(&mut Transform, &Handle<RonAsset>)>,
  ron_assets: Res<Assets<RonAsset>>,
) {
  for (mut transform, asset_handle) in &mut transform_via_asset {
      let Some(asset) = ron_assets.get(asset_handle) else {
          continue;
      };
      if transform.translation != asset.translation {
          info!("Updating {}", asset.name);
          transform.translation = asset.translation;
      }
  }
}

fn set_cube_translation2(
  mut transform_via_asset: Query<(&mut Transform, &Handle<RonAsset2>)>,
  ron_assets: Res<Assets<RonAsset2>>,
) {
  for (mut transform, asset_handle) in &mut transform_via_asset {
      let Some(asset) = ron_assets.get(asset_handle) else {
          continue;
      };
      if transform.translation != asset.translation {
          info!("Updating {}", asset.name);
          transform.translation = asset.translation;
      }
  }
}

Run via cargo run --example hot_asset_reloading --features="file_watcher" (I modified the hot_asset_reloading.rs file).

Initially all seems ok:

2024-01-31T09:39:49.332644Z  INFO hot_asset_reloading: Updating Cube 1
2024-01-31T09:39:49.332663Z  INFO hot_asset_reloading: Updating Cube 2

Updating one of the files via hot-reloading gives an error:

2024-01-31T09:40:19.259971Z  INFO bevy_asset::server: Reloading data/my-asset because it has changed
2024-01-31T09:40:19.260257Z ERROR bevy_asset::server: no `AssetLoader` found for file with no extension

Updating the other file does not give an error, but the system updating the translation doesn't run like it should either

2024-01-31T09:40:29.994741Z  INFO bevy_asset::server: Reloading data/my-asset-2.ron because it has changed

@bushrat011899
Copy link
Contributor Author

I made an example which breaks and I think it shouldn't:
Example

Run via cargo run --example hot_asset_reloading --features="file_watcher" (I modified the hot_asset_reloading.rs file).

Ah that's why I'm having trouble replicating this issue myself, I wasn't testing with the file_watcher feature. Thank you for posting this! I'll look at this and see if I can get this PR compatible with that feature as well.

When a reload is requested for a given path, now `load_internal` will be called with the existing `UntypedHandle` for that asset (if still valid), for each handle.

If that process is unsuccessful, but the asset still should be reloaded (as decided by `should_reload`), then a `load_internal` is called without a handle as well.
@bushrat011899
Copy link
Contributor Author

Ok @torsteingrindvik this should be resolved now. Again, thanks for testing this, I had completely forgotten to test against the file_watcher feature. As an aside, the fix involves now passing the original input_handle into the load_internal method called by reload, which should skip a fistful of redundant operations involved in getting/recreating the handle. Probably no performance impact in the real world, but still nice.

Running your example now works as expected, where editing the extensionless assets is immediately reflected in the running application with no runtime errors.

@torsteingrindvik
Copy link
Contributor

Ok @torsteingrindvik this should be resolved now. Again, thanks for testing this, I had completely forgotten to test against the file_watcher feature. As an aside, the fix involves now passing the original input_handle into the load_internal method called by reload, which should skip a fistful of redundant operations involved in getting/recreating the handle. Probably no performance impact in the real world, but still nice.

Running your example now works as expected, where editing the extensionless assets is immediately reflected in the running application with no runtime errors.

Great! Approved. Thanks yourself for the nice PR.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 31, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 31, 2024
Merged via the queue into bevyengine:main with commit afa7b5c Jan 31, 2024
23 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

- Addresses **Support processing and loading files without extensions**
from bevyengine#9714
- Addresses **More runtime loading configuration** from bevyengine#9714
- Fixes bevyengine#367
- Fixes bevyengine#10703

## Solution

`AssetServer::load::<A>` and `AssetServer::load_with_settings::<A>` can
now use the `Asset` type parameter `A` to select a registered
`AssetLoader` without inspecting the provided `AssetPath`. This change
cascades onto `LoadContext::load` and `LoadContext::load_with_settings`.
This allows the loading of assets which have incorrect or ambiguous file
extensions.

```rust
// Allow the type to be inferred by context
let handle = asset_server.load("data/asset_no_extension");

// Hint the type through the handle
let handle: Handle<CustomAsset> = asset_server.load("data/asset_no_extension");

// Explicit through turbofish
let handle = asset_server.load::<CustomAsset>("data/asset_no_extension");
```

Since a single `AssetPath` no longer maps 1:1 with an `Asset`, I've also
modified how assets are loaded to permit multiple asset types to be
loaded from a single path. This allows for two different `AssetLoaders`
(which return different types of assets) to both load a single path (if
requested).

```rust
// Uses GltfLoader
let model = asset_server.load::<Gltf>("cube.gltf");

// Hypothetical Blob loader for data transmission (for example)
let blob = asset_server.load::<Blob>("cube.gltf");
```

As these changes are reflected in the `LoadContext` as well as the
`AssetServer`, custom `AssetLoaders` can also take advantage of this
behaviour to create more complex assets.

---

## Change Log

- Updated `custom_asset` example to demonstrate extension-less assets.
- Added `AssetServer::get_handles_untyped` and Added
`AssetServer::get_path_ids`

## Notes

As a part of that refactor, I chose to store `AssetLoader`s (within
`AssetLoaders`) using a `HashMap<TypeId, ...>` instead of a `Vec<...>`.
My reasoning for this was I needed to add a relationship between `Asset`
`TypeId`s and the `AssetLoader`, so instead of having a `Vec` and a
`HashMap`, I combined the two, removing the `usize` index from the
adjacent maps.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
7 participants