open-rmf / rmf_site

Experimental visualizer for dense buildings in RMF
32 stars 13 forks source link

Move workspace loading to workflows #233

Closed luca-della-vedova closed 1 month ago

luca-della-vedova commented 1 month ago

New feature implementation

Implemented feature

Refactor workspace loading to use bevy_impulse workflows.

Implementation description

As noted in the title, first try at using the library so happy to hear ways I can do it better / more efficiently. Previously we created channels and used a system that spawns a detached async task that sends messages to a channel (for file dialogs) and one that continuously polls that channel to see if any event has been received. By contrast, now this is done in a workflow where the output of the file selecting is piped into the function that will use it.

I went for an API that removes events altogether and uses workflows instead. This has, to my understanding, a few benefits:

I was on the fence on how to save workflows in the app, I went for a single resource for "all workspace loading workflows" but we can play with modularity and, for example, have one resource per workflow, use marker components, split the services, etc. but that is a whole different discussion.

If this looks good I'll tackle workspace saving next, then go for the really complex one of model handling

luca-della-vedova commented 1 month ago

Web CI failure is real, it seems bevy impulse cannot compile on web because of different types uses to represent tasks in wasm (where multithreading is not implemented).

mxgrey commented 1 month ago

I was hoping the wasm situation would have had a fix upstream by now, but I'm not surprised that it isn't working.

One solution that comes to mind is to implement a simple single-threaded async executor that polls all the ready tasks during flush_impulses. We would use feature flags to choose between the async task pool vs single threaded flusher.

I think the main issue is this would make async providers unsuitable for long-running tasks that don't await often when they're run in web assembly, even though they'd work fine in a native build. That kind of inconsistency may cause confusion and frustration for users.

In any case I'll take a stab at this and see how it goes.

mxgrey commented 1 month ago

I've added a single_threaded_async feature for bevy_impulse which seems to help the wasm issue.

However there are some new compilation errors that show up in site editor when compiling for web assembly, all related to this map_async call:

error: future cannot be sent between threads safely
   --> rmf_site_editor/src/workspace.rs:386:18
    |
386 |                 .map_async(|_| async {
    |                  ^^^^^^^^^ future created by async block is not `Send`

I'm pretty baffled about why that would be Send when not targeting wasm but not be Send when targeting wasm. Maybe AsyncFileDialog doesn't support Send when it targets wasm..?

Theoretically when single_threaded_async is active we might not need the Send bound on tasks since they'll always be run on one thread..? I don't know if it's realistic to relax that bound, but it might be worth trying.

mxgrey commented 1 month ago

I've figured out a way to relax the Send bound when the single_threaded_async feature is on. Testing the web build of rmf_site_editor locally, it seems to be working very well. The dialog selection workflow is working exactly as expected.

The only issue I noticed is that zooming in/out seems to have some probability of making the whole screen go blank for no apparent reason. I doubt that has anything to do with this PR, in fact it could easily be a weird issue with the version of Firefox on my laptop rather than any problem is the site editor itself, but it may be something to keep an eye on.