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

basic winit port #176

Merged
merged 4 commits into from
Feb 23, 2024
Merged

basic winit port #176

merged 4 commits into from
Feb 23, 2024

Conversation

xorgy
Copy link
Member

@xorgy xorgy commented Feb 17, 2024

This removes (for now) accessibility, as well as the system menu.

@xorgy xorgy marked this pull request as draft February 17, 2024 23:17
@xorgy xorgy mentioned this pull request Feb 17, 2024
src/widget/raw_event.rs Outdated Show resolved Hide resolved
@dfrg
Copy link
Contributor

dfrg commented Feb 20, 2024

This is much cleaner than I would have expected. Overall LGTM and I’m comfortable merging now and making further changes incrementally.

@xorgy
Copy link
Member Author

xorgy commented Feb 20, 2024

This is much cleaner than I would have expected. Overall LGTM and I’m comfortable merging now and making further changes incrementally.

It's not really clean, because there are a bunch of hanging references in App which rely on the (Glazier) WindowHandle, which can't really access it because of the ownership boundary between them. This PR breaks accessibility in its current form, and there's not really any clean way to restore it without changing the App/AppLauncher API.

@Philipp-M
Copy link
Contributor

not really any clean way to restore it without changing the App/AppLauncher API

What do you mean exactly?

@xorgy
Copy link
Member Author

xorgy commented Feb 20, 2024

@Philipp-M

What do you mean exactly?

I mean that the AppLauncher has to own the window, but the App accesses it directly to implement accessibility integration, and there is no clean way to let both of them access it. Right now there is no mechanism for opening subwindows anyway, which was the purpose of separating App and AppLauncher, so collapsing these interfaces together seems to be the quickest way to get it working.

@raphlinus
Copy link
Contributor

I skimmed the PR and tested it. It works for me (tested on mac), and further work blocks on it. My preference is to merge soon, and document all the regressions and rough edges to those can be separate PRs. Thanks for the work!

@xorgy
Copy link
Member Author

xorgy commented Feb 21, 2024

Yeah. I'll add the removals for the the features that are broken by this PR and pull it out of draft; we can go-no-go at the Office Hours tomorrow.

@xorgy xorgy marked this pull request as ready for review February 21, 2024 20:06
@Philipp-M
Copy link
Contributor

I'm running into a weird issue at this line (so far I could pinpoint it at least):

let surface_texture = surface

This seems to deadlock for me with this PR, after a few frames painted (slightly under 10 frames).
I'm not sure it's directly related to this PR though, as I have sometimes a crash when resizing on current main (at the same line with an failed to acquire next swapchain texture: Outdated error) but it doesn't crash or deadlock at this line otherwise.

I'm not sure where the exact issue could be. I'm testing on Hyprland (wayland + nvidia beta driver) currently. I can try on X11 if it works there for me.

I just tried the wgpu raytracing triangle example which works for me (maybe relevant).

@Philipp-M
Copy link
Contributor

I just tested it on X11, there it works. I can still provoke the Outdated swapchain texture issue though when resizing...

@Philipp-M
Copy link
Contributor

Philipp-M commented Feb 22, 2024

Ok, I'm pretty sure it's a wgpu issue, I could reproduce the issue with the examples on wgpu 0.18 (which is what xilem still uses). But not on wgpu 0.19

@Philipp-M
Copy link
Contributor

Ok, no unfortunately it wasn't as easy as that...
I have quickly hacked in wgpu 0.19 and updated to current vello (I guess I can upstream that on your/this branch, when it's already done anyway?).

Same issue though, deadlock at that line after some frames...

Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

I could "fix" it by commenting out these two lines: https://github.com/gfx-rs/wgpu/blob/b8a8ff6e44162a41b8d54184a88613794dd3d2e4/wgpu-hal/src/vulkan/instance.rs#L967

And since these lines don't exist on the trunk branch on wgpu, it does work indeed with

[patch.crates-io]
wgpu = { git = 'https://github.com/gfx-rs/wgpu.git' }

in Cargo.toml (after updating/adjusting wgpu and vello in xilem).

When no one else has these issues (seems to happen on wayland + vulkan + maybe nvidia), I'd be ok to merge with this regression, as it otherwise looks good to me.

Maybe it also makes sense to add the lines above to Cargo.toml (but I guess this can be its own PR on top of this, including the update to wgpu and vello).

src/widget/raw_event.rs Show resolved Hide resolved
@Philipp-M
Copy link
Contributor

Philipp-M commented Feb 22, 2024

FWIW, here's my changes to get that working for me with wayland: Philipp-M@9ab5c83

@xorgy xorgy added this pull request to the merge queue Feb 23, 2024
@xorgy
Copy link
Member Author

xorgy commented Feb 23, 2024

FWIW, here's my changes to get that working for me with wayland: Philipp-M@9ab5c83

When this is merged, please put that up as a PR. :+ )

Merged via the queue into linebender:main with commit 21771ef Feb 23, 2024
7 checks passed
@Philipp-M Philipp-M mentioned this pull request Mar 2, 2024
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.

4 participants