Skip to content

Conversation

@DGriffin91
Copy link
Contributor

@DGriffin91 DGriffin91 commented May 12, 2025

Sometimes a bit more flexibility is needed here than just vsync on/off for testing, and occasionally in video games players may also want more control.

Inspired by https://github.com/gfx-rs/wgpu/blob/v25/wgpu-types/src/lib.rs#L5125 but didn't include AutoVsync or AutoNoVsync, and rather opted for making all of the modes non-fallible with appropriate fallback options.

Didn't update WindowData or WindowBuilder in this PR since I wanted to get your opinion first on the general direction first. (I also don't personally use WindowData or WindowBuilder so I wouldn't need a change with those to accomplish what I'm trying to do)

@attackgoat
Copy link
Owner

I don't use the window crate either; it's great for examples but any serious user is going to want much more control over their window(s). So it's kind of a starting point, and I like how this keeps that API simple but clearly shows where you can do neat stuff.

I really like the documentation calling out the tearing behavior because those modes are confusing.

Having a copy of vk::PresentModeKHR is otherwise a code smell which could be avoided using a new trait (PresentModeFallback or some such) implemented by default for vk::PresentModeKHR (and slices of the same) with this logic but allowing the user to pass their own fallback order.

... but essentially all users would use this logic because the choice is either smooth animation (fifo or relaxed) or immediate display (immediate or mailbox).

The other present modes are interesting and I wonder if the extensions should be checked and added to the physical device capabilities so they can be used.

These are my thoughts, but overall this is great and better than current. So your call if we make further changes or just merge.

@DGriffin91
Copy link
Contributor Author

@attackgoat Good point about having the copy. Hadn't thought about how screen-13 only has one backend so it makes sense to use the vk stuff directly.

Would this PresentModeFallback mean using a trait object? I'm not 100% following what this method would ideally look like yet. I do like the idea of the user being able to pass their own fallback order though!

@attackgoat
Copy link
Owner

Here's an example of what I'm describing, it might be one of those APIs that is more cumbersome than required:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c5cec6bfeda7879ba3eb270fd395e211

@DGriffin91
Copy link
Contributor Author

Hmm I wonder if this is maybe getting a bit fancy for just the vsync modes, not sure. I think this would also require SwapchainInfo to be generic over PresentModeFallback? I wonder if the really simple but still flexible solution would be to just have present_mode be a Vec<vk::PresentModeKHR> from the start. The allocation already happens with the current impl, it's just delayed. Then it can be fully defined by the user and also a function like present_mode_smooth() could just return the appropriate Vec<vk::PresentModeKHR>

@attackgoat
Copy link
Owner

A Vec<vk::PresentModeKHR> is probably the best for this code because it gets called basically once and easy-to-understand is the key.

DeviceInfo also uses functions like that to do default logic on color spaces.

@attackgoat
Copy link
Owner

attackgoat commented May 16, 2025

If you're good with this design than so am I and let's fix the test and merge it.

Another option is to create new functions on Surface which wrap get_physical_device_surface_capabilities and get_physical_device_surface_present_modes and let the user have control. That way the info would be the actual info that describes the swapchain instead of desired_image_count and desired_present_modes.

I am mixed because in general I can't decide whether this crate should make helpful choices or help make choices.

@DGriffin91
Copy link
Contributor Author

Yeah, it's a tricky balance. I think I'm good with this for now, but something worth considering, for sure.

@attackgoat attackgoat merged commit 11be0a1 into attackgoat:master May 16, 2025
3 checks passed
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.

2 participants