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

add wasm-backend using HTML canvas element #741

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

Conversation

genieCS
Copy link
Contributor

@genieCS genieCS commented Jul 13, 2023

Add WebAssembly (WASM) backend to Cursive library

This PR adds a new backend to the Cursive library that allows it to be used in WebAssembly (WASM) environments. The new backend is implemented using the web-sys, js-sys crates and provides a way to run Cursive-based applications in the browser.

To use the new backend, simply enable the wasm-backend feature in your Cargo.toml file:

[dependencies]
cursive = { version = "0.16", features = ["wasm-backend"] }

I made sample project using this backend, so you can see how to use this new backend.
repo link: https://github.com/genieCS/wretris
project link: https://geniecs.github.io

genieCS added 2 commits July 13, 2023 15:35
* add wasm-backend

* add wasm configuration and replace sleep

* add wasm-backend
* add wasm-backend

* add wasm configuration and replace sleep

* add wasm-backend

* fix busy_waiting

* fix busy waiting

* add buffer to print

* add js module

* change passing parameter as pointer
@genieCS genieCS marked this pull request as ready for review July 26, 2023 09:06
@genieCS genieCS changed the title (WIP) add wasm-backend using HTML canvas element add wasm-backend using HTML canvas element Jul 26, 2023
Copy link
Contributor

@christoph-heiss christoph-heiss left a comment

Choose a reason for hiding this comment

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

Just a shallow, quick review; currently breaks builds due to changing default features.

cursive/Cargo.toml Outdated Show resolved Hide resolved
cursive-core/Cargo.toml Outdated Show resolved Hide resolved
cursive/Cargo.toml Outdated Show resolved Hide resolved
cursive-core/Cargo.toml Outdated Show resolved Hide resolved
@genieCS
Copy link
Contributor Author

genieCS commented Jul 27, 2023

Just a shallow, quick review; currently breaks builds due to changing default features.

thank you for taking a look:) I addressed what you mentioned

Copy link
Contributor

@christoph-heiss christoph-heiss left a comment

Choose a reason for hiding this comment

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

Apart from two more comments, looks pretty good overall!

Can also confirm that builds and tests now run through again without a problem.

cursive/src/backends/wasm.rs Outdated Show resolved Hide resolved
cursive/src/backends/wasm.rs Outdated Show resolved Hide resolved
@genieCS genieCS requested a review from christoph-heiss July 28, 2023 05:37
Copy link
Contributor

@christoph-heiss christoph-heiss left a comment

Choose a reason for hiding this comment

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

Really just some nits this time, but generally LGTM. 👍

cursive/src/backends/canvas.js Outdated Show resolved Hide resolved
cursive/src/backends/canvas.js Outdated Show resolved Hide resolved
cursive/src/backends/wasm.rs Outdated Show resolved Hide resolved
cursive/src/backends/wasm.rs Outdated Show resolved Hide resolved
cursive/src/backends/wasm.rs Outdated Show resolved Hide resolved
@gyscos
Copy link
Owner

gyscos commented Aug 17, 2023

Thanks for the work!

My main concern is that the async features:

  • is non-additive since it changes the API
  • seems tied to wasm (we could imagine an async loop without wasm).

To avoid these two points, I'd be open to a major bump for API changes if needed, maybe even making async the default way to run the loop (or an option regardless of backend).

@genieCS
Copy link
Contributor Author

genieCS commented Sep 2, 2023

@gyscos I made run as async. Could you have another look?

@gyscos
Copy link
Owner

gyscos commented Sep 10, 2023

(I'm finally back from a family trip and have at last some time to look at this PR in more depth.)

Thanks for the updates and reactivity!

Looking at the code more, I think we can actually avoid breaking changes by adding a parallel code-path for async (rather than replacing the existing one). The current "sync" API could be defined as simply calling block_on the async API, preserving compatibility with existing code, without duplicating (too much) logic (I wish this didn't require the futures crate though).

The "bare minimum" would be to have async fn post_events_async(), so users could re-build their own event loop (the various step and run methods are just convenient methods to remove some boilerplace, but could be re-implemented user-side).

EDIT: I tried to do that in a async-restricted branch, but I have no idea how to test it and see if it works.

We could then, later, add the convenient equivalent.

It also looks like the sleep behaviour should maybe be part of the backend instead - and this opens up the possibility of having other async methods from the backend (input? drawing? refresh?). At this point, post_events will no longer be the only method with an async equivalent.
The main issue is having async in traits currently requires the async_trait crate. The heap allocation per method from async-trait doesn't matter much when it's just to call run (or even step), but it might not be ideal for the Backend trait, especially for the many print methods (potentially thousands per second).
On the other hand we're using dynamic dispatch for the backend (which makes it incompatible with the current async_fn_in_trait proposal) so maybe we should just use async-trait here, only making sleep async at first, and maybe refresh/input later (but not the print methods).

As a side-question, I'm wondering if this wasm backend would work in a non-browser wasm environment (like wasmtime?). I suspect not, so I'd favor calling it wasm_browser or wasm_js or something else more indicative of the browser aspect.

Also, would it be possible to add a very simple example using this backend? I haven't found how to use wasm-pack with an example, so maybe a dedicated "example" crate would be simpler.

(I'm really sorry for the back-and-forth.)

@genieCS
Copy link
Contributor Author

genieCS commented Sep 18, 2023

@gyscos thank you for your time and effort to review this PR.

  • "wasm_js" or "wasm_browser" idea sounds good to me. I'll think more to rename it.

  • Promise can be executed only if it runs forefront.
    In your async-restricted branch, it is wrapped by futures::executor::block_on so it would not work.
    How about having duplicate sets of sleep, post_events, run, step, ...etc with configuration option as my first draft?

regarding test, I'll make a simple project and let you know in a week

@genieCS
Copy link
Contributor Author

genieCS commented Sep 23, 2023

@gyscos I made a simple project(https://github.com/genieCS/hello-wasm) for tests.

  1. in / path, run wasm-pack build
  2. in www/path, run npm run start
    if you see "Hello from Rust!" in web console, it works.

@genieCS
Copy link
Contributor Author

genieCS commented Sep 23, 2023

@gyscos canvas is for browser but it would not be hard to make other wasm backends work.
I'd like to work on implementing other backend as a separate PR.

regarding async_trait, I agree that it would be more proper to put sleep in backend.
but for now, it's only sleep, so how about keep current status and when we have more to change Backend trait related to async, change together?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants