[Merged by Bors] - Improve examples loading feedback #355
Conversation
- The idea to use `TransformStream` and original code to get the download progress is by @ickk Co-Authored-By: ickk <17050131+ickk@users.noreply.github.com>
|
This is a proof of concept. Right now it doesn't work in Firefox as it needs a polyfill. Once the polyfill is added and if everything works fine I'll change the PR status for review. |
|
I just noticed that it doesn't work on Android Firefox. Desktop works though with the polyfill. -_- |
|
Looks like it's this issue: bevyengine/bevy#4431 I've tried on a locally built Error Message |
|
works as in the video on my Safari and Firefox 👍 |
|
I've tried it with ngrok, and over |
|
This is great, thanks for working on this! To reiterate my comments left on #338 I don't think monkey-patching is an ideal solution.
|
|
Hi! I think there is a misunderstanding with my goal here. I thought #338 was for the examples on the Bevy Website, as it supersedes #236. This PR just pretends to improve the website examples loading feedback, it's not a proposal for the general Bevy engine "assets loading feedback" story itself. There is an UX issue on the website right now (#236) and this just tries to solve that UX issue with the tools at hand. It's super hacky but it (kind-of) does the job. That being said… it's true that the monkey-patch approach is fragile. For example, if someone who is not aware of the hack adds some search functionality on the header using I think I have an idea, I'll use
Then, in This is still a hack, even if it doesn't pollute the original |
…` implementation to the WASM js file
shorten comments
…llow reuse shorten name of trackLoadingProgressFetch to loadingBarFetch
factor progressiveFetch into tools.js for reuse by other pages
|
@ickk much better! What do you think on using |
|
I don't personally mind using unpkg, but @cart might have a strong opinion one way or the other. Another idea I had: we could actually factor the whole loading bar thing into a function that takes a canvas-id and returns a custom-fetch anonymous fn. Then the API would be import init from './{{ page.title }}.js';
import { loadingBarFetch } from '/tools.js';
init(loadingBarFetch('bevy'));and would let us have as many wasm thoughts? |
…its loading feedback elements
|
It could be nice, but I don't know if it's practical right now. The examples have the canvas ID hardcoded & then we have the JS Where would you use this though? News? I think that my computer would explode if more than one example load/run at the same time… 😅 |
|
You are right that it is a fragile system :/ I think longer term, something like bevy_framepace can probably help prevent blowing up too many computers, as well as not automatically downloading and running bevy instances; this is already pretty egregious with e.g. That said, even if something like a news post would only include a single canvas, being a self contained module would still be tidy (and there's no reason not to allow specifying the canvas-id). But maybe you were right. Perhaps some kind of holistic web-container stuff should belong in a different PR, and this PR should be just a quick and dirty UX improvement for the examples. 💁♀️ |
|
Would it be possible to get the name of the resource that is being fetched to add somme text like "Loading flight_helmet.gltf" and then only show one loading bar at a time. It just looks like a bug when there's multiple loading bar showing up and showing only one loading bar at a time without text would probably not be great either. |
We could pretty easily print just the URL of the resource, if that suffices? |
|
Yup, much better. You'll notice that in the end the progress bar is at half, there is nothing I can do about that. The main thread blocks at that point and the browser doesn't even repaint… Or not, I suppose I could delay the bevy-example-progress-bar-03.mp4 |
|
Actually there was a bug, it didn't call the @cart what do you think about loading the poly-fills from an external service? 👉 #355 (comment) I might be introducing malicious code via those huge polyfill files… 😈 🙄 |
|
Oh! good catch regarding the bug. Looks good enough to me! |
There was a problem hiding this comment.
The filename addition is really nice. I'm not a massive fan of the background effect before loading the wasm, but it's not a deal breaker for me.
Also, using third parties to load polyfill is pretty standard practice on the web so I'm personally fine with it, especially since at worst it will mean there won't be any loading bar, but nothing else would break if for some reason unpkg somehow went down.
|
Thanks for the reviews! As all three of us agree, I've removed the poly-fill files and used |
|
Ah, btw, I added the bg animation as intermediate feedback until the wasm file starts loading… but it's easy to remove if more people don't like it. |
|
I'd prefer to self host the polyfills (for the same reasons we don't use google fonts directly):
|
|
I also think I'm in favor of removing the bg animation. Its a bit "loud" atm. I suspect theres an ideal animation out there (maybe a subtle pulse of the background color), but I think I'd prefer to just use a solid color in this PR so we can merge it asap and then follow up later to iterate (if we are so inclined). |
|
With those changes, I'm ready to merge this! |
…improve-example-loading-feedback
|
@cart both issues now are fixed. Now that I see it, that background loading animation it's probably not necessary. The idea was to show something before the wasm file starts loading… but in reality I think this will happen quickly enough so that the user won't even notice. |
IceSentry
added
the
S-Ready-For-Final-Review
|
bors r+ |
This is a proposal for #338. - Builds on top of the code by @ickk using `TransformStream`. - Adds loading feedback on examples. - The solution is hacky, it monkey-patches `fetch` in the example template. https://user-images.githubusercontent.com/188612/163875627-b11bf330-0fb8-4991-a710-e12e8657fe07.mp4 Co-Authored-By: ickk <17050131+ickk@users.noreply.github.com> Co-authored-by: ickk <git@ickk.io>
|
Pull request successfully merged into master. Build succeeded: |
bors
bot
changed the title
I think we've ironed out enough of the kinks to justify making the examples page more prominent. We've been sitting on a lot of value here for awhile 😄 For posterity: big thanks to @mockersf for putting together this feature in #225 and @doup for adding loading feedback in #355 and a bunch of other visual improvements.
This is a proposal for #338.
TransformStream.fetchin the example template.bevy-example-progress-bar-02.mp4
Co-Authored-By: ickk 17050131+ickk@users.noreply.github.com