-
Notifications
You must be signed in to change notification settings - Fork 72
Rewrite the ash runner #419
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
base: main
Are you sure you want to change the base?
Conversation
19159e1
to
0e10f07
Compare
examples/runners/ash/src/main.rs
Outdated
_ => {} | ||
}, | ||
_ => event_loop_window_target.set_control_flow(ControlFlow::Wait), | ||
_ => event_loop_window_target.set_control_flow(ControlFlow::Poll), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, is this the change that makes it animate continually instead of only while activating the mouse/keyboard?
examples/runners/ash/src/main.rs
Outdated
..Default::default() | ||
}, | ||
vk::PipelineShaderStageCreateInfo { | ||
module: shader_module, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, you can use shader_module
for multiple stages; the differing entry points distinguish them. 👍
examples/runners/ash/src/main.rs
Outdated
.get_physical_device_surface_present_modes(self.pdevice, self.surface) | ||
.unwrap() | ||
pub fn create_swapchain(&self) -> anyhow::Result<(vk::SwapchainKHR, vk::Extent2D)> { | ||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep the unsafe
scope where it was originally? It's generally good for these scopes to be as small as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused at first where this function is supposed to be. Turns out, in the rewrite commit this function got split, which moved the unsafe part into MySwapchainManager::recreate_swapchain
and the property discovery part into MySwapchainManager::new
.
Generally, I agree that the unsafe parts should be as short as possible. However, I do count setting up the properties to call the unsafe vulkan function towards the unsafe part, since wrong configuration is where most of the unsafety comes from.
Thanks for cleaning this up, I don't understand everything about this code but the parts I do understand look good. I can see you've generally improved the error handling, fixed a bunch of "FIXME" comments, etc. It would be nice to have some commit messages that are more than a single line. The "ash runner: rewrite" commit in particular changes a lot of code with zero explanation. |
* use Vulkan 1.3 * use DynamicRendering feature replacing Framebuffers * extract `MyDevice` struct containing vk initialization * decouple rendering from swapchain management * redo swapchain sync, fixes validation errors, intentionally kept basic * make rendering lazily recreate its pipeline
0e10f07
to
def7735
Compare
6597b17
to
3007d25
Compare
Followup from #418
I've noticed that running the ash runner with debug layers enabled (
--debug-layers
) prints tons of validation errors, all of which should be considered programming errors and sort of equivalent to a runtime panic. Instead of trying to fix what is there, I decided to completely rewrite the ash runner instead, and update it to the modern Vulkan 1.3 way you'd render things.MyDevice
struct containing vk initializationMacOS: removes the
ash-molten
dependency, which means we now require MoltenVK (or the Vulkan SDK) to be installed on MacOS for the ash runner to work. The wgpu runner still works as normal, with the metal backend.Reviews
Please test this on your machine with validation layers turned on, if possible (requires the Vulkan SDK). Report back if it runs and on which OS and graphics card.
Note:
--shader=sky
: holding arrow keys should change brightness--shader=mouse
: should be animated--debug-layer
requires vulkan sdk