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 ability to provide custom a AssetIo implementation #1037

Merged
merged 2 commits into from
Dec 18, 2020

Conversation

blunted2night
Copy link
Contributor

It works by allowing you to add a factory function as a resource before adding the default plugins. The factory function is given access to the AppBuilder instance so that it can look for any configuration it might require.

@Moxinilian Moxinilian added A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible labels Dec 9, 2020
@mockersf
Copy link
Member

mockersf commented Dec 9, 2020

It's already possible to use a custom AssetPlugin, wrapping your own AssetIo!

For example, here is how I use a custom AssetIo that keeps everything in memory:

    builder
        .add_plugins_with(DefaultPlugins, |group| {
            return group
                .disable::<bevy::asset::AssetPlugin>()
                .add_after::<bevy::asset::AssetPlugin, _>(asset_io::InMemoryAssetPlugin);
        });

@blunted2night
Copy link
Contributor Author

@mockersf thanks, that makes sense.

I think their may still be some value in allowing the default asset plugin to run. Replacing the whole AssetPlugin requires copying other parts of the AssetServer's initialization, which if those details change, could then break a custom plugin that only cares about swapping out the AssetIo instance.

@smokku
Copy link
Member

smokku commented Dec 9, 2020

It is possible to get the existing AssetIo resource, wrap it and then insert own replacing the default one.
This is what https://github.com/emosenkis/bevy_prototype_inline_assets/blob/main/src/lib.rs#L123 does.

@blunted2night
Copy link
Contributor Author

blunted2night commented Dec 9, 2020

@smokku This still has the problem of copying potentially changing initialization code of the AssetServer just to override the AssetIo instance. In this example some of the initialization was skipped. Was that intentional?

From what I see, the AssetIo trait cleanly encapsulates the ability to load from an alternate files system abstraction. Needing to have knowledge of the implementation details of the rest of the infrastructure breaks that encapsulation somewhat.

One thing I am missing with my proposed approach is to export the ability to construct the platform default AssetIo. This would allow the InlineAssetIo in your example to work correctly. I think making platform_default_asset_io_factory public would be enough.

@cart
Copy link
Member

cart commented Dec 10, 2020

I think the factory approach in this pr works, but maybe its too much machinery for what we get?

Is there a reason to not do something like this:

struct AssetIoOverride(Box<dyn AssetIo>);

That enables any plugin to override the defaults (or override overrides that came before) using arbitrary criteria (ex: platform, configuration, etc)

@blunted2night
Copy link
Contributor Author

I started down that road at one point, but it would require taking ownership of the AssetIo instance from resource. That would mean that the AssetPlugin would have to remove the AssetIoOverride resource from the world in order to consume it. I don't see any reason why it wouldn't work, but it seems to go against the purpose of resources.

It made me think that it might be nice for there to be a separate channel for initialization data that is not needed once the world is up and running. I don't know how something like that would be structured though. I suppose it could just be a set of resources that only lives until the AppBuilder is run. On the other hand, maybe its fine to remove a resource once its purpose has been fulfilled.

@blunted2night
Copy link
Contributor Author

I looked at taking the AssetIoOverride from the world but it is not currently possible. Is there a reason not to have some API to remove a resource from the world?

@blunted2night
Copy link
Contributor Author

blunted2night commented Dec 10, 2020

Alternatively it could be:

struct AssetIoOverride(Option<Box<dyn AssetIo>>)

So that the Box could be taken, leaving None in its place, but this feels wrong to me.

@cart
Copy link
Member

cart commented Dec 11, 2020

Alternatively: why not just let plugins insert their own AssetServer instance? AssetPlugin could just check to see if it exists before trying to create the default instance.

That keeps the api small and seems like it accomplishes the desired outcomes.

@blunted2night
Copy link
Contributor Author

blunted2night commented Dec 11, 2020

how about, in bevy::asset:

pub fn create_default_asset_io (settings: &AssetServerSettings) -> Box<dyn AssetIo>;
pub fn build_asset_server_with_io (app: &mut AppBuilder, asset_io: Box<dyn AssetIo>);

and bevy::asset::AssetPlugin is just a thin wrapper over these two functions

then in user code:

app.add_plugins_with(DefaultPlugins, |group| {
    group.disable::<bevy::asset::AssetPlugin>()
});
bevy::asset::build_asset_server_with_io (app, Box::new(CustomAssetIo));

This way the engine still handles all of the initialization, keeping user code from needing to know how to do it. And there is very little machinery, just two extra methods available.

@cart
Copy link
Member

cart commented Dec 11, 2020

I still think inserting a custom AssetServer feels like the simpler solution. No new interfaces, just a single existence check in AssetPlugin. I think people understand constructors pretty well.

@blunted2night
Copy link
Contributor Author

That is certainly the most bang for our buck. I was just trying keep the lookup of the IO task pool out of user code, but it's not much of a savings.

I will update the PR to go with this approach.

@blunted2night
Copy link
Contributor Author

I rework this PR to use the suggested approach and added an example to demonstrate the correct way to use it.

@cart
Copy link
Member

cart commented Dec 18, 2020

Looks good to me. Thanks!

@cart cart merged commit 596bed8 into bevyengine:master Dec 18, 2020
@blunted2night blunted2night deleted the custom-asset-io branch December 18, 2020 23:47
@fopsdev fopsdev mentioned this pull request Jan 24, 2021
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants