asset hooks #12077

MalekiRe · 2024-02-24T02:25:35Z

Objective

Need to perform transformations on gLTFs and other assets in the future, in order to parse it properly and spawn entities, components based on the data before it's loaded.

Solution

This pr allows any third party crate to register multiple transformations to apply to assets before they actually get loaded.

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

Added

fn register_asset_hook<A: Asset, Out, Marker>(
&mut self,
hook: impl IntoSystem<AssetWrapper, Out, Marker> + Sync + Send + 'static + Clone,
) ;
to register asset hooks.

crates/bevy_asset/src/server/mod.rs

crates/bevy_asset/src/lib.rs

crates/bevy_asset/src/loader.rs

alice-i-cecile · 2024-02-24T02:30:49Z

Generally a cool idea! I think this is a nice architecture. Needs more docs, tests and an example however: Bevy's asset code is already really tricky to learn and change. We should steadily make that better.

…ergonomics and total functionality.

CooCooCaCha · 2024-02-24T06:14:43Z

Would it align more with overall design of the asset system if sub-asset processing was specified in meta files?

My understanding is each piece of data in a gltf file has a path, so you should be able to specify processing steps and settings per-mesh, per-texture, etc.

Thats how I imagined the asset system would work. A loader loads a file, which spawns other loaders/processors.

UkoeHB

This poor bevy_asset/server/mod.rs file is getting kinda big...

UkoeHB · 2024-02-24T07:00:07Z

crates/bevy_asset/src/lib.rs

+    /// Registers a hook that gives a &mut Asset to the user, along with any arbitrary system
+    /// parameters. This runs before the asset is loaded into the world. You can register multiple
+    /// hooks.


Suggested change

/// Registers a hook that gives a &mut Asset to the user, along with any arbitrary system

/// parameters. This runs before the asset is loaded into the world. You can register multiple

/// hooks.

/// Registers a system hook that runs after any asset of type `A` is loaded, but before it is added to the world.

///

/// The system hook's `In` parameter contains an [`AssetWrapper`], which holds a reference

/// to the asset. A panic will occur if the asset wrapper is saved anywhere (the reference is only valid

/// in the system invocation).

///

/// You can register multiple hooks.

UkoeHB · 2024-02-24T07:04:50Z

crates/bevy_asset/src/loader.rs

+    /// Exists such that we can determine the type statically for the loading hook from an asset that has an erased type.
+    fn load_hook(&mut self, asset_hooks: &mut AssetHooks, world: &mut World);


Suggested change

/// Exists such that we can determine the type statically for the loading hook from an asset that has an erased type.

fn load_hook(&mut self, asset_hooks: &mut AssetHooks, world: &mut World);

/// Runs [`AssetHooks`](AssetHook) for the asset in this container, if any exist.

fn run_hooks(&mut self, asset_hooks: &mut AssetHooks, world: &mut World);

UkoeHB · 2024-02-24T07:05:24Z

crates/bevy_asset/src/loader.rs

+    fn load_hook(&mut self, asset_hooks: &mut AssetHooks, world: &mut World) {
+        asset_hooks.trigger::<A>(self, world);
+    }


Suggested change

fn load_hook(&mut self, asset_hooks: &mut AssetHooks, world: &mut World) {

asset_hooks.trigger::<A>(self, world);

}

fn run_hooks(&mut self, asset_hooks: &mut AssetHooks, world: &mut World) {

asset_hooks.run::<A>(self, world);

}

UkoeHB · 2024-02-24T07:06:54Z

crates/bevy_asset/src/server/mod.rs

+}
+#[allow(clippy::unused_unit)]


Suggested change

}

#[allow(clippy::unused_unit)]

}

#[allow(clippy::unused_unit)]

UkoeHB · 2024-02-24T07:07:41Z

crates/bevy_asset/src/server/mod.rs

+}
+/// Safe wrapper around asset pointers to allow for passing into systems.


Suggested change

}

/// Safe wrapper around asset pointers to allow for passing into systems.

}

/// Safe wrapper around asset pointers to allow for passing into systems.

UkoeHB · 2024-02-24T07:23:31Z

crates/bevy_asset/src/server/mod.rs

+    fn add_asset_hook<A, Out, Marker, F>(&mut self, f: F)
+    where
+        A: Asset,
+        F: IntoSystem<AssetWrapper<A>, Out, Marker> + Sync + Send + 'static + Clone,


Suggested change

fn add_asset_hook<A, Out, Marker, F>(&mut self, f: F)

where

A: Asset,

F: IntoSystem<AssetWrapper<A>, Out, Marker> + Sync + Send + 'static + Clone,

fn add_asset_hook<A, (), Marker, F>(&mut self, f: F)

where

A: Asset,

F: IntoSystem<AssetWrapper<A>, (), Marker> + Sync + Send + 'static,

There should not be an output. Also Clone shouldn't be necessary (see fix below).

UkoeHB · 2024-02-24T07:26:12Z

crates/bevy_asset/src/server/mod.rs

+    pub fn register_asset_hook<A: Asset, Out, Marker>(
+        &self,
+        hook: impl IntoSystem<AssetWrapper<A>, Out, Marker> + Sync + Send + 'static + Clone,


Suggested change

pub fn register_asset_hook<A: Asset, Out, Marker>(

&self,

hook: impl IntoSystem<AssetWrapper<A>, Out, Marker> + Sync + Send + 'static + Clone,

pub fn register_asset_hook<A: Asset, (), Marker>(

&self,

hook: impl IntoSystem<AssetWrapper<A>, (), Marker> + Sync + Send + 'static,

UkoeHB · 2024-02-24T07:29:31Z

crates/bevy_asset/src/server/mod.rs

+        let hooks = self.0.get_mut::<AssetHookVec<A>>().unwrap();
+        hooks.push(Box::new(move |asset, world| {
+            let mut system: F::System = IntoSystem::into_system(f.clone());
+            system.initialize(world);
+            let asset_wrapper = AssetWrapper::new(asset);


Suggested change

let hooks = self.0.get_mut::<AssetHookVec<A>>().unwrap();

hooks.push(Box::new(move |asset, world| {

let mut system: F::System = IntoSystem::into_system(f.clone());

system.initialize(world);

let asset_wrapper = AssetWrapper::new(asset);

let hooks = self.0.get_mut::<AssetHookVec<A>>().unwrap();

let mut system: F::System = IntoSystem::into_system(f);

let mut initialized = false;

hooks.push(Box::new(move |asset, world| {

if !initialized {

system.initialize(world);

initialized = true;

}

let asset_wrapper = AssetWrapper::new(asset);

Avoid cloning.

UkoeHB · 2024-02-24T07:30:46Z

crates/bevy_asset/src/server/mod.rs

        let loaded_asset: LoadedAsset<A> = asset.into();
+
        let erased_loaded_asset: ErasedLoadedAsset = loaded_asset.into();


Suggested change

let loaded_asset: LoadedAsset<A> = asset.into();

let erased_loaded_asset: ErasedLoadedAsset = loaded_asset.into();

let loaded_asset: LoadedAsset<A> = asset.into();

let erased_loaded_asset: ErasedLoadedAsset = loaded_asset.into();

UkoeHB · 2024-02-24T07:31:17Z

crates/bevy_asset/src/server/mod.rs

+                    loaded_asset
+                        .value
+                        .load_hook(&mut server.data.hooks.write(), world);


Suggested change

loaded_asset

.value

.load_hook(&mut server.data.hooks.write(), world);

loaded_asset

.value

.run_hooks(&mut server.data.hooks.write(), world);

mockersf · 2024-02-24T07:44:10Z

This is a new way to do things that are already possible, without a clear advantage and adding complexity.

And as it modify the gltf assets, this would mean a plugin registering hooks for an asset type could break all other use of that asset type

nicopap · 2024-02-24T08:04:52Z

Previous work:

nicopap

I don't think this is the right way of implementing it. We should first look into improving composability of AssetTransformer before adding this.

In any case, I gave some feedback to help improve this code if it is to be merged.

nicopap · 2024-02-24T08:41:03Z

crates/bevy_asset/src/server/mod.rs

+    pub(crate) hooks: RwLock<AssetHooks>,
+}
+
+struct TypeMap(hashbrown::HashMap<TypeId, Box<dyn std::any::Any + Send + Sync>>);


As mentioned, the additions to this file should really be moved in a separate module.

I notice that the key type for TypeMap here is exclusively AssetHookVec<A>. I would suggest replacing the Box<dyn Any> by some erased version of AssetHookVec<A>. Maybe by replacing AssetHookVec<A> with a Vec<SystemId>.

nicopap · 2024-02-24T08:42:17Z

crates/bevy_asset/src/server/mod.rs

+        if !self.0.has::<AssetHookVec<A>>() {
+            self.0.set::<AssetHookVec<A>>(Vec::new());
+        }


This, followed by the get_mut().unwrap() should be replaced by .entry(value).get_or_default().

nicopap · 2024-02-24T09:42:11Z

crates/bevy_asset/src/server/mod.rs

+            let mut system: F::System = IntoSystem::into_system(f.clone());
+            system.initialize(world);
+            let asset_wrapper = AssetWrapper::new(asset);


I feel like the initialization logic shouldn't be ran in the closure, but rather in the trigger method. This would reduce code bloat.

asset hooks

cb92852

alice-i-cecile added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds labels Feb 24, 2024

formatting

0879383

alice-i-cecile reviewed Feb 24, 2024

View reviewed changes

crates/bevy_asset/src/server/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Feb 24, 2024

View reviewed changes

crates/bevy_asset/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Feb 24, 2024

View reviewed changes

crates/bevy_asset/src/loader.rs Outdated Show resolved Hide resolved

alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Feb 24, 2024

MalekiRe added 3 commits February 23, 2024 20:11

Changed it to use the &mut world and take in a system for far better …

c4d3510

…ergonomics and total functionality.

Formatting

535ec11

Fixed nasal demons

55be783

MalekiRe added 2 commits February 23, 2024 22:18

Clippy fixes and general fixes

91eaaa0

Added doc comments

462f1fb

MalekiRe requested a review from alice-i-cecile February 24, 2024 06:53

MalekiRe mentioned this pull request Feb 24, 2024

Improved glTF extension support #12039

Open

UkoeHB suggested changes Feb 24, 2024

View reviewed changes

mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Feb 24, 2024

nicopap reviewed Feb 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asset hooks #12077

asset hooks #12077

MalekiRe commented Feb 24, 2024 •

edited

Loading

alice-i-cecile commented Feb 24, 2024

CooCooCaCha commented Feb 24, 2024 •

edited

Loading

UkoeHB left a comment

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024 •

edited

Loading

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024

UkoeHB Feb 24, 2024

mockersf commented Feb 24, 2024

nicopap commented Feb 24, 2024

nicopap left a comment

nicopap Feb 24, 2024

nicopap Feb 24, 2024

nicopap Feb 24, 2024

-    /// Registers a hook that gives a &mut Asset to the user, along with any arbitrary system
-    /// parameters. This runs before the asset is loaded into the world. You can register multiple
-    /// hooks.
+    /// Registers a system hook that runs after any asset of type `A` is loaded, but before it is added to the world.
+    ///
+    /// The system hook's `In` parameter contains an [`AssetWrapper`], which holds a reference
+    /// to the asset. A panic will occur if the asset wrapper is saved anywhere (the reference is only valid
+    /// in the system invocation).
+    ///
+    /// You can register multiple hooks.

		/// Exists such that we can determine the type statically for the loading hook from an asset that has an erased type.
		fn load_hook(&mut self, asset_hooks: &mut AssetHooks, world: &mut World);

		}
		/// Safe wrapper around asset pointers to allow for passing into systems.

		let loaded_asset: LoadedAsset<A> = asset.into();

		let erased_loaded_asset: ErasedLoadedAsset = loaded_asset.into();

asset hooks #12077

Are you sure you want to change the base?

asset hooks #12077

Conversation

MalekiRe commented Feb 24, 2024 • edited Loading

Objective

Solution

Changelog

Added

alice-i-cecile commented Feb 24, 2024

CooCooCaCha commented Feb 24, 2024 • edited Loading

UkoeHB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UkoeHB Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf commented Feb 24, 2024

nicopap commented Feb 24, 2024

nicopap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalekiRe commented Feb 24, 2024 •

edited

Loading

CooCooCaCha commented Feb 24, 2024 •

edited

Loading

UkoeHB Feb 24, 2024 •

edited

Loading