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

Experimental WebGL wgpu backend support #1096

Merged
merged 18 commits into from
Jan 31, 2022
Merged

Experimental WebGL wgpu backend support #1096

merged 18 commits into from
Jan 31, 2022

Conversation

pacmancoder
Copy link
Contributor

@pacmancoder pacmancoder commented Oct 23, 2021

Made changes to allow the use of WebGL wgpu backend. The basic functionality works, but text rendering is still buggy - Somehow, if the initial value of the text control was changed, some parts of the text start to disappear/show artifacts.

UPD: Strange issue with text rendering is only reproducing in Firefox (v93). It is not directly related to the iced, I created an issue in wgpu_glyph here: hecrj/wgpu_glyph#78

The current implementation provides neither Application nor Sandbox implementations for this backend, and does not implement proper Clipboard in iced_winit, however, this should be at least some starting point.

Changes list:

  • Added new integration_wgpu_webgl example, based on integration_wgpu and wgpu's hello-triangle example (can't use integration_wgpu example directly because it fails with its spirv shaders on WebGL)
  • Lifted Sync requirement for Runtime and Tracker in iced_futures
  • Aded webgl feature to iced_wgpu which enables wgpu/webgl
  • Created stub implementation for Clipboard in iced_winit
Images

iced running inside WebGL canvas:
image

Text issues after changing text value:
image

Same example running on desktop backend:
image

@@ -5,6 +5,50 @@ use crate::{subscription, Executor, Subscription};
use futures::{channel::mpsc, Sink};
use std::marker::PhantomData;

#[cfg(not(target_arch = "wasm32"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of duplication of Runtime and Tracker with the different trait bounds, but I thought that such trait aliasing may work better WYT?

@pacmancoder pacmancoder marked this pull request as draft October 23, 2021 10:45
@pacmancoder
Copy link
Contributor Author

Opening PR for review, looks like an issue with text rendering is not directly related to iced repo, and reproduces only in Firefox. Created separate issue in wgpu_glyph: hecrj/wgpu_glyph#78

@pacmancoder pacmancoder marked this pull request as ready for review October 23, 2021 17:55
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Nice! Will take a proper look soon 🎉

Instead of a new example, could we instead change the existing one so it works everywhere?

@pacmancoder
Copy link
Contributor Author

@hecrj Actually yes I will do that, I just need to figure out alternative wgsl shader integration_wgpu example instead of existing spir-v.

@pacmancoder
Copy link
Contributor Author

@hecrj made original integration_wgpu example work WebGL. It does not want to work with spirv shaders, so I created identical shaders in wgsl.

Behold, it is alive 😄!
image

@pacmancoder
Copy link
Contributor Author

UPD: Just a thing to consider - this example works fine with wgsl shader on desktop backends (at least on Windows, I checked DX and VULKAN backends), so maybe we could remove spir-v shaders from this example and use wgsl on all targets? I was not sure if I can remove this, as it shows to the user that spirv shaders can be used here.

@hecrj
Copy link
Member

hecrj commented Oct 25, 2021

@pacmancoder Yes, I think we can remove the SPIR-V shaders altogether and use wgsl everywhere. It's not up to us to showcase the different features of wgpu. Simpler that way!

@pacmancoder
Copy link
Contributor Author

Removed spir-v shader and fixed code formatting (GH Workflow was failing in cargo fmt)

@pacmancoder
Copy link
Contributor Author

@hecrj is something still blocking this PR before merge?

@kaimast
Copy link
Contributor

kaimast commented Oct 28, 2021

This is great! I was about to implement the same thing before I found this. I'll take a look now as well and let you know if I find some issues.

#[cfg(target_arch = "wasm32")]
pub struct Clipboard;

#[cfg(target_arch = "wasm32")]
Copy link
Contributor

@kaimast kaimast Oct 28, 2021

Choose a reason for hiding this comment

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

When I tried to get this to work yesterday, I added a "NullClipboard" to window_clipboard instead. We can eventually replace it with proper clipboard support for different wasm environments. I think this is cleaner than having a feature flag here, but not sure what @hecrj prefers.

Can send a pull request if my approach is more descired

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

I took another quick glance and left some more feedback.

I still have to take a proper look and test everything! However, I'm working on a bunch of changes related to rendering and I'd like to finish that first.

Comment on lines 6 to 48
#[cfg(not(target_arch = "wasm32"))]
mod trait_aliases {
use super::*;

pub trait TrackerMessage: Send + 'static {}

impl<T> TrackerMessage for T where T: Send + 'static {}

pub trait TrackerMessageReceiver<Message: TrackerMessage>:
Sink<Message, Error = mpsc::SendError> + Unpin + Send + Clone + 'static
{
}

impl<Message: TrackerMessage, T> TrackerMessageReceiver<Message> for T where
T: Sink<Message, Error = mpsc::SendError>
+ Unpin
+ Send
+ Clone
+ 'static
{
}
}

#[cfg(target_arch = "wasm32")]
mod trait_aliases {
use super::*;

pub trait TrackerMessage: 'static {}

impl<T> TrackerMessage for T where T: 'static {}

pub trait TrackerMessageReceiver<Message: TrackerMessage>:
Sink<Message, Error = mpsc::SendError> + Unpin + Clone + 'static
{
}

impl<Message: TrackerMessage, T> TrackerMessageReceiver<Message> for T where
T: Sink<Message, Error = mpsc::SendError> + Unpin + Clone + 'static
{
}
}

pub use trait_aliases::{TrackerMessage, TrackerMessageReceiver};
Copy link
Member

Choose a reason for hiding this comment

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

Are these necessary? iced_web is able to use the Runtime and, therefore, the Tracker already.

iced/web/src/lib.rs

Lines 170 to 175 in 8a2a7f7

let mut runtime = iced_futures::Runtime::new(
Self::Executor::new().expect("Create executor"),
sender.clone(),
);
let (app, command) = runtime.enter(|| Self::new(flags));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly yes, this is necessary - we use winit here with WebGL backend and it (winit) does not implement Send on its EventLoopProxy on wsam32, so yea, because of that we should lift here requirement for Send for our types too

Comment on lines 5 to 9
#[cfg(not(target_arch = "wasm32"))]
type BoxStream<'a, T> = iced_futures::futures::stream::BoxStream<'a, T>;

#[cfg(target_arch = "wasm32")]
type BoxStream<'a, T> = iced_futures::futures::stream::LocalBoxStream<'a, T>;
Copy link
Member

Choose a reason for hiding this comment

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

These are already defined in iced_futures!

iced/futures/src/lib.rs

Lines 43 to 69 in 8a2a7f7

/// A boxed static future.
///
/// - On native platforms, it needs a `Send` requirement.
/// - On the Web platform, it does not need a `Send` requirement.
#[cfg(not(target_arch = "wasm32"))]
pub type BoxFuture<T> = futures::future::BoxFuture<'static, T>;
/// A boxed static future.
///
/// - On native platforms, it needs a `Send` requirement.
/// - On the Web platform, it does not need a `Send` requirement.
#[cfg(target_arch = "wasm32")]
pub type BoxFuture<T> = futures::future::LocalBoxFuture<'static, T>;
/// A boxed static stream.
///
/// - On native platforms, it needs a `Send` requirement.
/// - On the Web platform, it does not need a `Send` requirement.
#[cfg(not(target_arch = "wasm32"))]
pub type BoxStream<T> = futures::stream::BoxStream<'static, T>;
/// A boxed static stream.
///
/// - On native platforms, it needs a `Send` requirement.
/// - On the Web platform, it does not need a `Send` requirement.
#[cfg(target_arch = "wasm32")]
pub type BoxStream<T> = futures::stream::LocalBoxStream<'static, T>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@kaimast
Copy link
Contributor

kaimast commented Oct 29, 2021

Did you test input with this branch yet? I seem to have a bunch of issues but they are probably caused by winit. See this bug report here: rust-windowing/winit#2036

Problem is that iced still uses a custom winit branch, so it's hard to figure out what's going on is fixed upstream or not.

@pacmancoder
Copy link
Contributor Author

Did you test input with this branch yet? I seem to have a bunch of issues but they are probably caused by winit. See this bug report here: rust-windowing/winit#2036

Problem is that iced still uses a custom winit branch, so it's hard to figure out what's going on is fixed upstream or not.

@kaimast Ah... I see... Sliders in the example were working fine then I just tried to add TextInput to the view and yep, after that, it broke (GUI started to ignore any mouse events to any GUI element). Looks like winit indeed could be broken for wasm32?

cc @hecrj

@kaimast
Copy link
Contributor

kaimast commented Jan 8, 2022

Could you rebase this on current master? iced has moved to wgpu 0.12 and it would be good to see if some of the issues are resolved.

@pacmancoder
Copy link
Contributor Author

@kaimast sure, I'll try to find som spare time on this week and do the rebase

@kaimast kaimast mentioned this pull request Jan 10, 2022
env_logger::init();

// Initialize winit
let event_loop = EventLoop::new();

#[cfg(target_arch = "wasm32")]
let window = winit::window::WindowBuilder::new()
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you can to save some cfg by doing:

let window = if cfg!(target_arch = "wasm32") {
    let canvas_element = {
        console_log::init_with_level(log::Level::Debug)
            .expect("could not initialize logger");
        std::panic::set_hook(Box::new(console_error_panic_hook::hook));

        web_sys::window()
            .and_then(|win| win.document())
            .and_then(|doc| doc.get_element_by_id("iced_canvas"))
            .and_then(|element| element.dyn_into::<HtmlCanvasElement>().ok())
            .expect("Canvas with id `iced_canvas` is missing")
    };
    let window = winit::window::WindowBuilder::new()
        .with_canvas(Some(canvas_element))
        .build(&event_loop)
        .expect("Failed to build winit window")
} else {
    env_logger::init();
    winit::window::Window::new(&event_loop).unwrap()
};

unless the Window::new() method returns separate types

@aentity
Copy link
Contributor

aentity commented Jan 15, 2022

this is very interesting. does this patch allow to run in pure wgpu, without to use html elements for the widget?

@kaimast
Copy link
Contributor

kaimast commented Jan 15, 2022

this is very interesting. does this patch allow to run in pure wgpu, without to use html elements for the widget?

yes

@aentity
Copy link
Contributor

aentity commented Jan 15, 2022

Wow this is great news! To experiment with targetting webgpu, will this approach here block in future to do this? Do we need to add a webgpu feature when/if we try such a thing?

@hecrj hecrj force-pushed the master branch 2 times, most recently from e6ab610 to 8b0f2e6 Compare January 19, 2022 15:04
@pacmancoder
Copy link
Contributor Author

@kaimast Welp, I rebased the branch to the latest master (although, some parts of the code not polished yet and need refactoring), however the problems are still here - both input handling and text rendering issues on Firefox are still present.

Unfortunately, I am currently have no time to research this further, If someone still interested in this feature, you are welcome to get this branch and continue work on it

@kaimast
Copy link
Contributor

kaimast commented Jan 23, 2022

I created a tiny pull request that should fix the input issues. I think it should be close to being merge-able now.

@pacmancoder
Copy link
Contributor Author

Thanks, @kaimast, merged

@@ -179,7 +178,7 @@ where

let mut context = task::Context::from_waker(task::noop_waker_ref());

event_loop.run_return(move |event, _, control_flow| {
event_loop.run(move |event, _, control_flow| {
Copy link
Contributor

Choose a reason for hiding this comment

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

@hecrj do you think this would break anything? It simplifies the code a lot and the winit docs says about run_return "You are strongly encouraged to use run, unless the use of this is absolutely necessary."

@hecrj
Copy link
Member

hecrj commented Jan 28, 2022

I made some changes here! Will explain everything first thing tomorrow!

You can use `trunk serve` to easily compile them!
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Okay! First of all, thanks for all the efforts here. This is very cool!

As I mentioned yesterday, I made a bunch of changes, some of them quite important.

First, I have removed the iced_web subcrate completely from the repository in 825c774! iced_web has always been quite experimental and their dependencies have been outdated for a while now. Therefore, I believe it's better to move it to a different repository in the iced-rs organization. As a consequence, from now on when targetting Wasm, iced will leverage wgpu and / or glow to render into a canvas using the iced_native crate (see 26d95fd).

In my opinion, this is a better default than iced_web because it makes targetting Wasm quite easier for users. As there are fewer differences between the native stack and the web stack, all of the native widgets should work on Wasm. If you have a native iced application, chances are you can compile it to Wasm! No additional feature flags necessary! Just slap a --target wasm32-unknown-unknown to your cargo build command.

That said, the plan is to still offer iced_web as a separate crate for users that need to leverage the DOM.

The rest of the changes are relatively minor:

  • I changed the Compositor in iced_wgpu to properly use WebGL limits in 7767241.
  • I created a new time module that offers a cross-platform Instant implementation in 87b3e03 and 83c649b.
  • I introduced a MaybeSend trait in iced_futures in 5dab5a3, which allows us to get rid of all the trait_aliases modules.
  • I have split the iced_futures executor implementations into different backend modules in 167be45. Each backend should expose the same API!
  • I implemented time::every for the wasm_bindgen backend in e730d97.

Finally, I have added a couple of index.html files to the todos and tour examples. You can try running them on your browser with trunk.

There are still some issues present related to rendering text. My MacBook Pro M1 displays completely mangled text in all of the examples both in Chrome and Firefox:

image

image

This seems to happen only to dynamic text, which seems to indicate there may be an issue in the cache upload logic in wgpu_glyph. Most likely related to hecrj/wgpu_glyph#78.

That said, I am pretty happy with the changes here and I think we can merge this! 🎉 We can iron out the issues in future PRs.

@m4b
Copy link

m4b commented Jan 30, 2022

this is... awesome 😹

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

Successfully merging this pull request may close these issues.

7 participants