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

asset: WasmAssetIo #703

Merged
merged 5 commits into from
Oct 20, 2020
Merged

asset: WasmAssetIo #703

merged 5 commits into from
Oct 20, 2020

Conversation

cart
Copy link
Member

@cart cart commented Oct 18, 2020

This adds a partial AssetIo wasm implementation. Loading folders doesn't work (just returns an empty list for now) and loading files within AssetLoaders will probably break (because we use futures::block_on). Some GLTF files won't load as a result of this. But the asset example works!

The latest cursor grabbing api broke wasm winit setup, so I fixed that too.

@mrk-its @chemicstry

Copy link

@chemicstry chemicstry left a comment

Choose a reason for hiding this comment

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

Nice work!

Regarding block_on, I think it would be best to make loaders async as it is the only way to make it work on single threaded wasm. GLTF loading with sub assets would still be broken, but that can be fixed by making schedule async as in the wasm threads PR.

impl AssetIo for WasmAssetIo {
async fn load_path(&self, path: &Path) -> Result<Vec<u8>, AssetIoError> {
let path = self.root_path.join(path);
let window = web_sys::window().unwrap();

Choose a reason for hiding this comment

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

This will not work in web workers and WorkerGlobalScope should be used instead. However, web-sys doesn't seem to provide any abstraction to access this cross-environments as WindowOrWorkerGlobalScope mixin is not supported.

I think the best option would be to access js_sys::global() and try casting it to either Window or WorkerGlobalScope, whichever succeeds. Similar fix was done in rustwasm/gloo#106, but it's kind of ugly. Maybe it could land in bevy_utils?

Copy link
Member Author

Choose a reason for hiding this comment

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

haha anything specific about web-sys / webworkers is going to be out of my depth at the moment. but that sounds reasonable to me. given that we know this works outside of webworkers, im inclined to merge as-is for now. Then someone who knows what they're doing (like you) can follow up with webworker compat.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding block_on, I think it would be best to make loaders async as it is the only way to make it work on single threaded wasm. GLTF loading with sub assets would still be broken, but that can be fixed by making schedule async as in the wasm threads PR.

+1 for making loaders async - I did it on my branch and it was very simple change

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hesitant to make asset loaders async because I didn't want consumers to need the #[async_trait] proc-macro. But:

  1. atelier-assets uses async loaders
  2. they do it without requiring #[async_trait] in consumers by using BoxFuture
  3. even if async_trait was required, thats not a huge loss relative to what we get

I'll make the change

Copy link
Member

@mrk-its mrk-its Oct 19, 2020

Choose a reason for hiding this comment

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

here is my implementation, for example working gltf loader: mrk-its@05bf8ae

BTW I didn't know how to conditionally enable '?Send' without copying whole struct definition, that's why I've ended with wrapping web_sys types with unsafe impl Send :) good to know about #[cfg_attr] :)

@cart
Copy link
Member Author

cart commented Oct 19, 2020

Not a huge fan of the reduced clarity of:

impl AssetLoader for HdrTextureLoader {
    fn load<'a>(&'a self, bytes: &'a [u8], load_context: &'a mut LoadContext) -> BoxedFuture<'a, Result<()>> {
        Box::pin(async move {
            Ok(())
        })
    }

    fn extensions(&self) -> &[&str] {
        static EXTENSIONS: &[&str] = &["hdr"];
        EXTENSIONS
    }
}

instead of

#[async_trait]
impl AssetLoader for HdrTextureLoader {
    fn load(&self, bytes: &[u8], load_context: &mut LoadContext) -> Result<()> {
        Ok(())
    }

    fn extensions(&self) -> &[&str] {
        static EXTENSIONS: &[&str] = &["hdr"];
        EXTENSIONS
    }
}

But I think I still prefer it over requiring consumers to pull in async_trait

@cart
Copy link
Member Author

cart commented Oct 19, 2020

I totally get that first class async traits are hard. But dang thats a huge missing piece. It sounds like the "real" fix is a long way off (GATs and a few other features are required). Imo it makes sense to bake in #[async_trait] to std and then deprecate it when the "full" impl lands.

@cart cart mentioned this pull request Oct 20, 2020
@cart cart merged commit f88cfab into bevyengine:master Oct 20, 2020
@cart cart mentioned this pull request Oct 20, 2020
@karroffel karroffel added A-Assets Load files from disk to use for things like images, models, and sounds O-Web Specific to web (WASM) builds labels Oct 20, 2020
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 O-Web Specific to web (WASM) builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants