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

Refactor triangle and texture examples to use better practices #356

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hgallagher1993
Copy link

Fixes #190

It's a huge commit but almost all of it is just moving around already existing code rather than newly written code. The reason it's so big is because I decided to refactor the ExampleBase so it matches https://vulkan-tutorial.com/ more closely which has the creation of everything broken down into small functions so I decided to do the same with the ExampleBase:: new(..) so that's pretty much where all the changed lines have come from.

As well as that I fixed the order of destroy_buffer, free_memory etc so when you close the window it doesn't error saying the device hasn't been destroyed.

And I created the render_triangle and render_texture functions just to keep the event_loop a bit smaller and less convoluted

@Ralith
Copy link
Collaborator

Ralith commented Jan 12, 2021

The reason it's so big is because I decided to refactor the ExampleBase so it matches https://vulkan-tutorial.com/

I haven't reviewed any changes yet, but this immediately gives me pause. vulkan-tutorial.com is a bit notorious for teaching poor practice; please don't take its patterns as gospel.

@hgallagher1993
Copy link
Author

@Ralith I've seen that the build failed on Clippy and Rustfmt but I'll wait until a full review is done before submitting fixes for them?

@ColdIce1605
Copy link

@hgallagher1993 eh push it

@hYdos
Copy link

hYdos commented Oct 19, 2021

any updates on this? Its been 6 months since the last update?

Event::LoopDestroyed => {
base.device.device_wait_idle().unwrap();

for &pipeline in graphics_pipelines.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best practice to drain the Vec here even though it doesn't actually change behavior here that much.

@@ -559,12 +595,18 @@ impl Drop for ExampleBase {
.destroy_fence(self.draw_commands_reuse_fence, None);
self.device
.destroy_fence(self.setup_commands_reuse_fence, None);
self.device.free_memory(self.depth_image_memory, None);
for &framebuffer in self.framebuffers.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

See earlier comment about draining the vector

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants