Bump wgpu to 0.19 and winit to 0.29

[mkrasnitski]

Also bumps raw-window-handle to 0.6. Blocked on the following:

• A new imgui release
  • Adding support to imgui-wgpu-rs: Update wgpu to 0.19 Yatekii/imgui-wgpu-rs#107
• A new egui release
• A new fltk release: Support raw-window-handle 0.6 fltk-rs/fltk-rs#1512
• Adding support to game-loop: fix(deps)!: update winit to 0.29 tuzz/game-loop#21

Right now this PR uses these libraries are used as git dependencies. I point at my own fork of imgui-wgpu-rs for now.

[mkrasnitski]

There is one major bug that manifests in the imgui-winit example and seems to come from imgui-wgpu-rs. Resizing the window crashes with the following error:

[2024-01-31T01:42:15Z ERROR wgpu::backend::wgpu_core] Handling wgpu errors as fatal by default
thread 'main' panicked at /home/michael/.local/share/cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-0.19.1/src/backend/wgpu_core.rs:3009:5:
wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `pixels_command_encoder`
    In a set_scissor_rect command
    Scissor Rect { x: 0, y: 0, w: 641, h: 480 } is not contained in the render target (640, 480, 1)

Making the window smaller doesn't crash - only larger. Removing the gui.handle_event line from the example prevents the bug, so this is likely from imgui-wgpu-rs; also there's a call to set_scissor_rect internally in the lib.

Another annoyance that only happens with my fork is that key events are logged to the terminal like so:

KEY EVENT: KeyEvent { physical_key: Code(AltLeft), logical_key: Named(Alt), text: None, location: Left, state: Pressed, repeat: false, platform_specific: KeyEventExtra { key_without_modifiers: Named(Alt), text_with_all_modifiers: None } }

I don't know why this happens. I'd appreciate help diagnosing the cause of both of these problems, and would consider them a current blocker.

My fork: https://github.com/mkrasnitski/imgui-wgpu-rs. The only changes I made to the lib itself are to Cargo.toml, so I'm extra confused why these bugs arise.

[parasyte]

Hi, thanks for starting this! I'll kick off the CI pipeline shortly.

The difference between the scissor rect and surface has been a very common battle. Specifically I believe it's #338 (which still needs more info). The same issue was fixed in the custom-shader example in #286.

[parasyte]

Another annoyance that only happens with my fork is that key events are logged to the terminal like so:

KEY EVENT: KeyEvent { physical_key: Code(AltLeft), logical_key: Named(Alt), text: None, location: Left, state: Pressed, repeat: false, platform_specific: KeyEventExtra { key_without_modifiers: Named(Alt), text_with_all_modifiers: None } }

This is printed by the imgui-winit-support crate: https://github.com/imgui-rs/imgui-rs/blob/ca05418cb449dadaabf014487c5c965908dfcbdd/imgui-winit-support/src/lib.rs#L486

[parasyte]

Just some comments. I think the one todo! is worth addressing. But also, now that I review the value for other compressed texture formats, I'm not even sure if those are correct...

[parasyte]

This is ... incredible! Does that mean #238 is fixed, and we can add #![forbid(unsafe_code)] back to the lib?

[mkrasnitski]

I'm surprised my approach worked. Since the handle traits have auto-impls for &W, I was able to switch from &'win W to W bounded on + 'win, which means you can also pass owned objects which satisfy a + 'static bound. This way I was able to overcome the borrow checker for some of the examples by using Arc.

[mkrasnitski]

Looks like egui 0.24 bumped MSRV to Rust 1.72. Since we're bounding types using the new versions of the rwh traits, we're not compatible with older versions of egui except the newest one (when it releases). So, maybe that means we should bump MSRV as well. Thoughts?

[mkrasnitski]

This is printed by the imgui-winit-support crate: https://github.com/imgui-rs/imgui-rs/blob/ca05418cb449dadaabf014487c5c965908dfcbdd/imgui-winit-support/src/lib.rs#L486

That definitely seems like it was accidentally left in. Probably worth a PR to imgui.

EDIT: For reference: imgui-rs/imgui-rs#763

[mkrasnitski]

Seems like CI is failing because of impl_trait_projections, which were stabilized in Rust 1.74. I could change Pixels::new_async to explicitly name Pixels<'win> instead of Self, or we could bump MSRV again to 1.74.

[parasyte]

or we could bump MSRV again to 1.74

Let's just do that. We'll eventually be increasing the MSRV to 1.74 anyway. Might as well do it early for the next release with breaking changes.

[mkrasnitski]

Oh, didn't see the other "MSRV"-style CI checks. I guess I should bump the 1.70 test version to 1.74, but the Rust-version check seems redundant. Especially because the external crate takes a git dependency on the git head of this project, which means it doesn't even use the current PR for testing.

[parasyte]

That check is there to handle this clause from our MSRV policy:

The MSRV will be chosen as the minimum version of rustc that can successfully pass CI, including documentation, lints, and all examples. For this reason, the minimum version supported may be higher than the minimum version required to compile the pixels crate itself.

Because dependencies in the examples tend to require a newer version of the compiler than the library needs, testing the two versions independently allows the library to require a compiler version that diverges from the examples.

Using the git HEAD is an unfortunate case of laziness where I just never added a way to select the correct branch or PR.

There is more information on this setup here:

• Add rust-version #295
• https://github.com/parasyte/pixels-ci-rust-version

So, no. It is definitely not redundant.

[mkrasnitski]

Because dependencies in the examples tend to require a newer version of the compiler than the library needs, testing the two versions independently allows the library to require a compiler version that diverges from the examples.

Thanks for the explanation, that makes sense.

Using the git HEAD is an unfortunate case of laziness where I just never added a way to select the correct branch or PR.

This is unfortunate because this PR adds a dependency on the impl_trait_projections feature for constructing Pixels via the new_async method. So, I think the rust-version test will end up failing as soon as this PR gets merged (since the crate points to the git HEAD). It would be great if somehow CI could point at the current branch HEAD instead. Otherwise, PRs that raise the minimum required version would only fail that test after they're merged (even though MSRV is ahead of the new required version).

[parasyte]

It is valid criticism and thank you for the reminder. I am currently OK with doing the maintainer thing and keeping CI happy post-merge with additional PRs. (That responsibility falls on me, not other contributors.)

[lukexor]

Excitingly looking forward to this merging as I'm working on a project to add egui to my existing winit project using pixels. However, I'm concerned about the new lifetime requirement on Pixels as that has a very wide ranging impact on any application with a lot of impl blocks for any struct containing the pixels instance.

I specifically switched to winit/pixels to get away from some of the performance issues of rust-sdl2 - which also requires a lifetime bound for textures (unless you use the unsafe_textures feature flag) and the only reasonable way to get around that is to Box::leak it.

None of the examples for either library are complex enough to reveal how painful this can be when you don't want to stuff all of your initialization logic into main.

Edit: Though after further contemplation - it seems this isn't so bad when you wrap the window in an Arc. If only I had understood the magic of heap-allocated lifetimes earlier...Might be good to call that out in the docs/example setup for new Rust devs who might get tripped up since the signature of SurfaceTexture::new doesn't lead one to thing of using an Arc, Rc, or Box

[mkrasnitski]

Yes, I agree that the docs should make it more obvious that to avoid lifetime headaches you should just wrap your window in an Arc and use Pixels<'static>. Suggestions welcome.

[tuzz]

Hey @mkrasnitski, game-loop version 1.1.0 is now updated to use version 0.29.x of winit. I hope that helps, thanks.

[JacobMillward]

This seems to supersede my PR at #386 . I'm not quite sure what we're waiting on for this PR now, can someone explain?

[mkrasnitski]

This PR is currently blocked on imgui-wgpu-rs supporting wgpu 0.19 and winit 0.29, so that the imgui-winit example compiles. Which should be easy, however it requires that lib changing to take a git dependency on imgui-rs, which got support for the new vesions of wgpu and winit a few months ago but hasn't seen a release since April 2023. I could open a PR to see if they're OK with that.

[parasyte]

I would be in support of any effort to upstream some of the unblocking work. Speaking of, I should be able to make some more progress on imgui-rs/imgui-rs#716 now! (Just looked briefly, and the PR has been obsoleted by others)

[vincenthz]

Would it make sense to drop imgui example (temporarily) and not block this PR ?

Once imgui catch up, the example can be added back, in the meantime imgui user can use the existing old version

[parasyte]

I believe that is probably the right compromise. Both wgpu and winit have released new versions while this has been in limbo.

My proposal would be updating the imgui example to use the previous release version, with a comment somewhere about the transitive dependency issue.

[mkrasnitski]

As if on cue, imgui had its 0.12 release 2 days ago. All that remains now is Yatekii/imgui-wgpu-rs#107. Unfortunately, the imgui example still crashes the same way it did previously when resizing the window. Ideally this should be resolved before the PR is merged, but I have no idea what might be causing it.

[tuzz]

I've updated game-loop again (version 1.2.0) to support the newly released winit 0.30.x.

[mkrasnitski]

From what I can tell, supporting winit 0.30 in pixels would require similar efforts for adding support to other dependencies. The ecosystem hasn't yet moved over. So, probably this PR should remain at 0.29 rather than entering another waiting period.

[parasyte]

Yeah, upgrading winit is going to be kind of rough.

They plan to remove the existing event hander callback and replace it with a bucket of callbacks defined in a trait: rust-windowing/winit#3626. Somehow digging deeper into Inversion of Control territory is appealing to the maintainers.

I'm keen to stick with 0.29 for now, but I'll be planning on a general replacement since they insist on making app and game development more difficult than it should be.

[jeapostrophe]

Is there a way to use this with only winit and game-loop? I don't care about imgui, egui, or fltk. It is frustrating to wait so long to update when the release is held up by features that don't affect me.

[mkrasnitski]

Is there a way to use this with only winit and game-loop? I don't care about imgui, egui, or fltk. It is frustrating to wait so long to update when the release is held up by features that don't affect me.

Certainly - just point at my fork. Compatibility with dependencies you aren't using won't matter for you so you'll be able to use the lib fine.

[jeapostrophe]

@mkrasnitski Thank you!

[JacobMillward]

Is there a way to use this with only winit and game-loop? I don't care about imgui, egui, or fltk. It is frustrating to wait so long to update when the release is held up by features that don't affect me.

I tend to agree that there is an underlying issue here. The core library can be upgraded, it is merely the surrounding example integrations that consistently keep this project out of date.

@parasyte it might be worth considering moving complex examples to pinned versions of the library, or moving them into a separate repository?

[mkrasnitski]

Ok I'm fed up waiting for imgui-wgpu-rs to update support for wgpu, so the solution is to just pin the example to use a specific pixels commit instead of using path, and keep the dependencies on the old versions. This PR has been sitting blocked on transitive dependencies for far too long.

Btw @parasyte it might be worth it to cut release 0.13.1 off the current project HEAD and publish it so that the imgui example can point to 0.13.1 instead of a specific git commit.

[mkrasnitski]

Sorry for the ping @parasyte, do you have any additional concerns, or is this PR free to merge?

[parasyte]

My apologies for dropping the ball on this! I've had my attention focused elsewhere.

Just about to publish a new version from main. It required some version bumps and will lead to some conflicts.

[parasyte]

I recognize how much effort this was, and how important the updates are. All of the checks are green, so I'm comfortable merging without hesitation. Any other needs can be addressed in smaller PRs.

Thank you for everything you did here, @mkrasnitski!