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.

Sign up for GitHub

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

Jump to bottom

Fix fetching assets in Web Workers #12134

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Fix fetching assets in Web Workers #12134

merged 4 commits into from
Mar 25, 2024

Conversation

allsey87
Copy link
Contributor

Objective

This PR fixes #12125

Solution

The logic in this PR was borrowed from gloo-net and essentially probes the global Javascript context to see if we are in a window or a worker before calling fetch_with_str.

@allsey87
Support fetching assets in Web Workers
b7358cf 
The logic in this PR was borrowed from gloo-net and essentially probes the global Javascript context to see if we are in a window or a worker before calling `fetch_with_str`.
@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@allsey87
Copy link
Contributor Author

Not sure what the procedure/rules are here, but I would like to suggest that this be added to the 0.13.1 milestone.

@mockersf mockersf added A-Assets Load files from disk to use for things like images, models, and sounds O-Web Specific to web (WASM) builds labels Feb 26, 2024
@mockersf mockersf added this to the 0.13.1 milestone Feb 26, 2024
@mockersf
Copy link
Member

unblocks your use case, not a breaking change, and it works 👍

@allsey87
Copy link
Contributor Author

@mockersf do I need to rebase this PR onto the release-0.13.1 branch?

@mockersf
Copy link
Member

no, once this PR has been approved by someone else, we'll merge it to main then cherry pick to the release branch

@allsey87
Determine whether the global context is a web_sys::Window or a `web…
1ed55de 
…_sys::WorkerGlobalScope` using bindings instead of `js_sys::Reflect`.
@allsey87
Copy link
Contributor Author

Updated this PR to use bindings to a Global object as suggested by @cart and @daxpedda.

I noticed that the check "validation jobs / run-examples-on-wasm (pull_request)" was skipped, is there any reason for this? It seems like a relevant check for this PR.

@BD103
Copy link
Member

BD103 commented Feb 29, 2024

I noticed that the check "validation jobs / run-examples-on-wasm (pull_request)" was skipped, is there any reason for this? It seems like a relevant check for this PR.

run-examples-on-wasm does not run until this PR gets added to the merge queue. You can ignore it for now :)

@james7132 james7132 requested a review from cart March 2, 2024 03:31
@mockersf mockersf self-requested a review March 4, 2024 09:09
@allsey87 allsey87 requested a review from daxpedda March 5, 2024 14:22
daxpedda
daxpedda approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

To reduce the amount of FFI calls you could cache the current global context in a thread_local. Keep in mind that this might or might not be worth it considering we are doing a HTTP request here.

Otherwise LGTM!

bushrat011899
bushrat011899 approved these changes Mar 17, 2024
View reviewed changes
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Nice catch and a simple fix! I've just added some comments as a suggestion to help explain some of the JS weirdness, and link back to MDN documentation as well.

crates/bevy_asset/src/io/wasm.rs Show resolved Hide resolved
crates/bevy_asset/src/io/wasm.rs Show resolved Hide resolved
crates/bevy_asset/src/io/wasm.rs Show resolved Hide resolved
crates/bevy_asset/src/io/wasm.rs Show resolved Hide resolved
@mockersf mockersf modified the milestones: 0.13.1, 0.13.2 Mar 18, 2024
@allsey87 @bushrat011899
Add comments and link back to MDN documentation
11b6268 
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
@pablo-lua pablo-lua 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 Mar 25, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 25, 2024
Merged via the queue into bevyengine:main with commit 3200331 Mar 25, 2024
27 checks passed
mockersf pushed a commit that referenced this pull request Apr 1, 2024
@allsey87 @bushrat011899 @mockersf
Fix fetching assets in Web Workers (#12134)
1d8a735 
# Objective

This PR fixes #12125

## Solution

The logic in this PR was borrowed from gloo-net and essentially probes
the global Javascript context to see if we are in a window or a worker
before calling `fetch_with_str`.

---------

Co-authored-by: Zachary Harrold <zac@harrold.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bushrat011899 bushrat011899 bushrat011899 approved these changes

@daxpedda daxpedda daxpedda approved these changes

@cart cart Awaiting requested review from cart

@mockersf mockersf Awaiting requested review from mockersf

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds O-Web Specific to web (WASM) builds 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
Milestone
0.13.2
Development

Successfully merging this pull request may close these issues.

Fetching assets is broken in web workers
8 participants
@allsey87 @mockersf @BD103 @daxpedda @bushrat011899 @cart @alice-i-cecile @pablo-lua