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

Full pipeline running in browser #220

Merged
merged 10 commits into from
Dec 8, 2022
Merged

Full pipeline running in browser #220

merged 10 commits into from
Dec 8, 2022

Conversation

raphlinus
Copy link
Contributor

@raphlinus raphlinus commented Nov 30, 2022

This is based on #201 and probably supersedes it. It's still hacky but does demonstrate the full pipeline running in a browser.

This is the command line I use:

RUSTFLAGS=--cfg=web_sys_unstable_apis cargo run --release --package run-wasm -- winit-demo

Then navigate Chrome Canary (with WebGPU flag enabled) to http://localhost:8000/

Fails on Windows because FXC, but runs on mac M1.

Significant progress toward #202

@dfrg dfrg marked this pull request as ready for review December 8, 2022 16:59
piet-wgsl/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I can't approve as I haven't run it, but this looks good.

run-wasm/Cargo.toml Show resolved Hide resolved
piet-wgsl/shader/coarse.wgsl Show resolved Hide resolved
piet-wgsl/shader/coarse.wgsl Outdated Show resolved Hide resolved
piet-wgsl/shader/binning.wgsl Outdated Show resolved Hide resolved
piet-wgsl/examples/winit/src/test_scene.rs Outdated Show resolved Hide resolved

// On wasm, append the canvas to the document body
let canvas = window.canvas();
canvas.set_width(1044);
Copy link
Member

Choose a reason for hiding this comment

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

I see this size is pre-existing, so I don't think we need to change it, but 1044 is not a very round number

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was specifically not very round to test an output that didn't align to a tile boundary.

run_wasm(event_loop, window).await
}

async fn run_wasm(event_loop: EventLoop<()>, window: Window) {
Copy link
Member

Choose a reason for hiding this comment

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

This function name isn't quite right. I'd be tempted here to factor out the wasm specific code from main into the equivalent place in run, and then not create the second function.

AFAIK, resizable just does nothing on web, rather than causing issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Refactored this a bit: run_wasm -> run and the native code is simply pulled into main

piet-wgsl/examples/winit/Cargo.toml Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
[package]
name = "winit"
name = "winit-demo"
Copy link
Member

Choose a reason for hiding this comment

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

Ha, I've been meaning to fix this. I presume you ran into the issue where it couldn't work out which winit you meant in cargo run -p winit?

Copy link
Collaborator

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

Since this is a joint PR, I'll add an approval. Current state works here (both web and native) on M1.

@raphlinus
Copy link
Contributor Author

This works on Canary Chrome (but is still missing some strokes). I'm comfortable with it going in based on the review it's gotten, but can also take a closer look if people feel that's needed.

@dfrg
Copy link
Collaborator

dfrg commented Dec 8, 2022

I feel pretty good about it as is. Feel free to merge

@raphlinus raphlinus merged commit a7dbeb3 into main Dec 8, 2022
@raphlinus raphlinus deleted the raph-browser branch December 8, 2022 18:28
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I forgot to submit these comments. They're not too important though

[workspace.package]
edition = "2021"
version = "0.1.0"
authors = ["piet-gpu developers"]
Copy link
Member

Choose a reason for hiding this comment

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

I think current best practise is to just not set authors

The default cargo new excludes it. I'm not too bothered either way it shakes out

// On wasm, append the canvas to the document body
let canvas = window.canvas();
canvas.set_width(1044);
canvas.set_height(800);
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly surprised this is needed; does setting the window size when creating it from winit not work?

I don't think it matters that much though

@raphlinus raphlinus mentioned this pull request Jan 13, 2023
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