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

Draft: add http & https asset sources #16366

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mrchantey
Copy link
Contributor

@mrchantey mrchantey commented Nov 13, 2024

Objective

Solution

  • Add http & https asset sources

Testing

  • Copied tests from bevy_web_asset
  • We might want to use our own sources for this, i just havent found a url for an asset .meta file yet.
  • Verify visually by running cargo run --example http_asset

Showcase

Bevy now supports assets loaded via url!

  // Simply use a url where you would normally use an asset folder relative path
  let handle = asset_server.load("https://raw.githubusercontent.com/bevyengine/bevy/refs/heads/main/assets/branding/bevy_bird_dark.png");
  commands.spawn(Sprite::from_image(handle));

TODO

  • Document caching behavior and native/wasm differences
  • Formal wasm tests?
  • Ability to swap out fetch strategy, allowing for more formal caching mechanisms

@mrchantey
Copy link
Contributor Author

It looks like surf and http-cache-surf have failed licence, vunerability & unmaintaned checks. Does that mean they're not allowed as first party dependencies?

@BenjaminBrienen
Copy link
Contributor

It's possible to add an exception for the licenses, but unmaintained and vulnerable is pretty bad.

@IQuick143 IQuick143 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 C-Dependencies A change to the crates that Bevy depends on X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 13, 2024
@mnmaita
Copy link
Member

mnmaita commented Nov 13, 2024

This could be done with something like reqwest maybe to avoid using the unmaintained crates. I successfully ran the example with it but my code is probably bad 😅. I can still push a PR as a showcase in case it serves as inspiration.

Copy link
Contributor

@MalekiRe MalekiRe left a comment

Choose a reason for hiding this comment

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

Yep this looks awesome to me! Also I wasn't aware of the surf crate; I'll have to keep it in mind for any bevy dashboards I make in the future

@mrchantey
Copy link
Contributor Author

I can still push a PR as a showcase in case it serves as inspiration.

@mnmaita actually that would be great, I've replaced surf and http-cache-surf with reqwest line for line but am a bit stuck at the moment with reqwest demanding to be run inside a tokio runtime.

// crates/bevy_asset/src/http_source.rs#134

    let client = reqwest_middleware::ClientBuilder::new(Client::new())
        .with(Cache(HttpCache {
            mode: CacheMode::Default,
            manager: CACacheManager::default(),
            options: HttpCacheOptions::default(),
        }))
        .build();

    let response = ContinuousPoll(client.get(str_path).send())
        .await

@jf908
Copy link

jf908 commented Nov 15, 2024

FWIW surf has a bug where any erroring http response >8kb stops every subsequent request from succeeding and is unlikely to ever be fixed which made me unable to use bevy_web_asset so I would personally appreciate a switch 😄.

If you can't get reqwest to work perhaps hyper would suffice since it's already in the dep tree for BRP? Perhaps too low level for this.

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-Dependencies A change to the crates that Bevy depends on C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Upstream bevy_web_asset, allowing assets loaded via http
6 participants