Skip to content

Conversation

@cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Dec 16, 2024

Connections

Closes #3334
Closes #3640
Depends on #6732

Commits cannot be merged separately, but can be reviewed separately

@cwfitzgerald cwfitzgerald force-pushed the cw/argument-buffer-metal branch from 1f67c9f to 05236bc Compare December 16, 2024 05:37
@cwfitzgerald cwfitzgerald changed the title Move Partial Binding into Own File Switch Binding Arrays on Metal to Argument Buffers Dec 16, 2024
@cwfitzgerald cwfitzgerald force-pushed the cw/argument-buffer-metal branch 3 times, most recently from 7be0e51 to 8986532 Compare December 16, 2024 06:04
@cwfitzgerald cwfitzgerald force-pushed the cw/argument-buffer-metal branch 3 times, most recently from cb8d1d7 to 793f047 Compare December 23, 2024 01:19
CARGO_TERM_COLOR: always
WGPU_DX12_COMPILER: dxc
RUST_LOG: info
RUST_LOG: debug
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a better setting anyway

Comment on lines +70 to +72
{
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Missed a feature check on this benchmark

use crate::auxil::map_naga_stage;
use crate::TlasInstance;

use metal::foreign_types::ForeignType;
Copy link
Member Author

Choose a reason for hiding this comment

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

Trait import

(entry, layout)
});
for (entry, layout) in layout_and_entry_iter {
// Bindless path
Copy link
Member Author

Choose a reason for hiding this comment

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

The bindless path is entirely new, the bindful path is entirely the same.

@cwfitzgerald cwfitzgerald force-pushed the cw/argument-buffer-metal branch from 6857fc9 to 4ce012c Compare December 23, 2024 02:33
@cwfitzgerald cwfitzgerald marked this pull request as ready for review December 23, 2024 02:33
@cwfitzgerald cwfitzgerald requested a review from a team December 23, 2024 02:33
@cwfitzgerald cwfitzgerald requested a review from a team as a code owner December 23, 2024 02:33
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Looks good, left a question.

///
/// As such, we wrap all argument buffers in a struct that has a single generic `<T>` field.
/// This allows `NagaArgumentBufferWrapper<metal::texture<..>>*` to work. The astute among
/// you have noticed that this should be exactly the same to the compiler, and you're correct.
Copy link
Member

Choose a reason for hiding this comment

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

😅

&& self.supports_arrays_of_textures_write
{
features.insert(F::STORAGE_RESOURCE_BINDING_ARRAY);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand the old comment but removing this means that write only storage textures are no longer supported. Did this work before?

Copy link
Member

Choose a reason for hiding this comment

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

wgpu/wgpu-types/src/lib.rs

Lines 573 to 578 in f6f9233

/// Supported platforms:
/// - Metal (with MSL 2.2+ on macOS 10.13+)
/// - Vulkan
///
/// This is a native only feature.
const STORAGE_RESOURCE_BINDING_ARRAY = 1 << 29;

Seems to be listed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't fully understand the old comment but removing this means that write only storage textures are no longer supported. Did this work before?

No they hit a nice fat assert in naga and generated bad msl once through

@cwfitzgerald cwfitzgerald merged commit a8a9173 into gfx-rs:trunk Jan 7, 2025
27 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.

Partially bound descriptors on Metal Use Argument Buffers for binding_array on Metal

2 participants