Upgrade ash to 0.38 and egui to 0.27.2 #10

YouSafe · 2023-11-12T19:12:18Z

Fix #9

MarijnS95

A lot of these calls aren't contributing to safety: refer to the Ash README on the latest published version.

MarijnS95 · 2023-11-13T08:28:07Z

examples/example/main.rs

+                let wait_dst_stage_mask = [vk::PipelineStageFlags::COLOR_ATTACHMENT_OUTPUT];
+                self.device.queue_submit(
+                    self.graphics_queue,
+                    &[vk::SubmitInfo::builder()


MarijnS95 · 2023-11-13T08:29:14Z

examples/example/main.rs

-                    .image_indices(&[image_index as u32])
-                    .wait_semaphores(&[self.render_finished_semaphores[self.current_frame]]),
+                    .swapchains(std::slice::from_ref(&self.swapchain))
+                    .image_indices(&image_indices)


Fwiw the temporary allocation here is fine, as long as you don't call .build.

MarijnS95 · 2023-11-13T08:30:02Z

examples/user_texture/main.rs

+                let wait_dst_stage_mask = [vk::PipelineStageFlags::COLOR_ATTACHMENT_OUTPUT];
+                self.device.queue_submit(
+                    self.graphics_queue,
+                    &[vk::SubmitInfo::builder()


Again from_ref might help to remove .build(). Constructing slices inside the same expression is fine.

MarijnS95 · 2023-11-13T08:30:30Z

src/integration.rs

-        }
-        .expect("Failed to create descriptor pool.");
+        let descriptor_pool = {
+            let pool_sizes = [vk::DescriptorPoolSize::builder()


No wins in factoring this out either.

MarijnS95 · 2023-11-13T08:31:21Z

src/integration.rs

+            let subpasses = [vk::SubpassDescription::builder()
+                .pipeline_bind_point(vk::PipelineBindPoint::GRAPHICS)
+                .color_attachments(&color_attachment_refs)
+                .build()];


This is where the lifetime of &color_attachments_refs gets lost.

MatchaChoco010 · 2023-11-18T04:37:35Z

First of all, thanks for the PR!

Sorry, but the PR seems to contain things that don't necessarily contribute to safety, as @MarijnS95 points out.
I know it's hard because the issue of lifetime and raw pointer is quite a difficult one, but @YouSafe could you please fix this PR?

MarijnS95 · 2023-11-18T09:02:56Z

For what it is worth some older Ash docs recommended (iirc) the &[...build()] pattern, as long as it resides inside the function-call statement because individual expression results are only dropped at the end of an expression (at the ;).

As such there are a few ways to go at this PR and these changes; but for the time being I'd recommend either tackling everything or only fix the actual lifetime issues (and not introduce any new ones!).

When .build() is removed in Ash 0.38 these temporary &[] borrows of local arrays are once again fine and safe (and the borrow checker will tell you to refactor when they're not) so it isn't worth replacing them all with slice::from_ref(). Removing .build() now however requires all those to be replaced with slice::from_ref(&) "temporarily" (and that doesn't work for >1 items).

MarijnS95 · 2024-04-02T13:40:21Z

The ash 0.38 release is out, which now no longer has these safety issues when using the builder pattern. build() can no longer be called.

YouSafe · 2024-04-05T16:42:25Z

Thank you for your work on ash 0.38, @MarijnS95!

Here are some notes on this PR:

I was unable to upgrade ash alone without also upgrading egui and, consequently, winit because of version mismatch issues with raw-window-handle.
Since the PR for the ash upgrade for gpu-allocator has not yet been merged, I temporarily specified the branch of the PR as a dependency here.
There are still sync validation errors which I'm not sure if there were present before the upgrade.
I think the wrong color space is being used for the examples, but that is not a new issue introduced by this PR.

I have some questions if that is okay with you, @MarijnS95:

I've used std::slice::from_ref in two places in integration.rs where it would otherwise give the following error: creates a temporary value which is freed while still in use. Is that okay?
Do you think it is okay that I used this kind of import: use ash::ext::debug_utils::Instance as DebugUtils; or would you strongly advise against that?

YouSafe · 2024-04-05T17:12:58Z

I ran the current version on the main branch and can confirm that the sync issues were present before this PR.

MarijnS95 · 2024-04-05T18:23:46Z

I think the wrong color space is being used for the examples, but that is not a new issue introduced by this PR.

Without diving in this codebase, this might be a common problem where the first format from swapchain' capabilities is used. If this is BGR instead of RGB, the render target bind point might not be updated and hence swizzle the wrong colors on write (though the validation layer should complain here).

I've used std::slice::from_ref in two places in integration.rs where it would otherwise give the following error: creates a temporary value which is freed while still in use. Is that okay?

slice::from_ref is still something that we recommend for more idiomatic code.

Do you think it is okay that I used this kind of import: use ash::ext::debug_utils::Instance as DebugUtils; or would you strongly advise against that?

I'd recommend to use ash::{vk, ext, ...} and reference this as ext::debug_utils::Instance. That makes it less confusing especially when you also want to use ext::debug_utils::Device to e.g. place markers on a command buffer.

MarijnS95 · 2024-04-05T18:26:57Z

src/integration.rs

+                    .dependencies(&[vk::SubpassDependency::default()
                        .src_subpass(vk::SUBPASS_EXTERNAL)
                        .dst_subpass(0)
                        .src_access_mask(vk::AccessFlags::COLOR_ATTACHMENT_WRITE)
                        .dst_access_mask(vk::AccessFlags::COLOR_ATTACHMENT_WRITE)
                        .src_stage_mask(vk::PipelineStageFlags::COLOR_ATTACHMENT_OUTPUT)
-                        .dst_stage_mask(vk::PipelineStageFlags::COLOR_ATTACHMENT_OUTPUT)
-                        .build()]),
+                        .dst_stage_mask(vk::PipelineStageFlags::COLOR_ATTACHMENT_OUTPUT)]),


Fyi for POD structs without references (pointers to objects or arrays), especially when you fill all fields like this, are cleaner to write with plain Rust field syntax (than the builder pattern). Same for vk::AttachmentDescription above, vk::ImageSubresourceRange and probably a few others.

MarijnS95 suggested changes Nov 13, 2023

View reviewed changes

MatchaChoco010 mentioned this pull request Nov 18, 2023

Undefined behaviour in library #9

Open

Upgrade ash to 0.38 & egui to 0.27.2

8c1af54

YouSafe force-pushed the main branch from 7e6d85f to 8c1af54 Compare April 5, 2024 15:53

YouSafe changed the title ~~Fix undefined behaviour~~ Upgrade ash to 0.38 and egui to 0.27.2 Apr 5, 2024

YouSafe added 3 commits April 5, 2024 18:01

Merge branch 'main' into main

646fea2

Make raw-window-handle a dependency

0709104

Fix unused import warning

662865e

MarijnS95 reviewed Apr 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade ash to 0.38 and egui to 0.27.2 #10

Upgrade ash to 0.38 and egui to 0.27.2 #10

YouSafe commented Nov 12, 2023

MarijnS95 left a comment

MarijnS95 Nov 13, 2023

MarijnS95 Nov 13, 2023

MarijnS95 Nov 13, 2023

MarijnS95 Nov 13, 2023

MarijnS95 Nov 13, 2023

MatchaChoco010 commented Nov 18, 2023

MarijnS95 commented Nov 18, 2023 •

edited

Loading

MarijnS95 commented Apr 2, 2024

YouSafe commented Apr 5, 2024

YouSafe commented Apr 5, 2024

MarijnS95 commented Apr 5, 2024

MarijnS95 Apr 5, 2024

Upgrade ash to 0.38 and egui to 0.27.2 #10

Are you sure you want to change the base?

Upgrade ash to 0.38 and egui to 0.27.2 #10

Conversation

YouSafe commented Nov 12, 2023

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 Nov 13, 2023

Choose a reason for hiding this comment

MarijnS95 Nov 13, 2023

Choose a reason for hiding this comment

MarijnS95 Nov 13, 2023

Choose a reason for hiding this comment

MarijnS95 Nov 13, 2023

Choose a reason for hiding this comment

MarijnS95 Nov 13, 2023

Choose a reason for hiding this comment

MatchaChoco010 commented Nov 18, 2023

MarijnS95 commented Nov 18, 2023 • edited Loading

MarijnS95 commented Apr 2, 2024

YouSafe commented Apr 5, 2024

YouSafe commented Apr 5, 2024

MarijnS95 commented Apr 5, 2024

MarijnS95 Apr 5, 2024

Choose a reason for hiding this comment

MarijnS95 commented Nov 18, 2023 •

edited

Loading