shame icon indicating copy to clipboard operation
shame copied to clipboard

Storage/Uniform Buffer Any API rework

Open chronicl opened this issue 5 months ago • 0 comments
trafficstars

Storage/Uniform Buffer Any API rework

The goal of this PR is twofold

  • Improve the user facing Any api for creating storage and uniform buffer bindings, mainly by making the surface of errors smaller.
  • Fully integrate #19 and remove any layout calculations that aren't part of layoutable/align_size.rs.

They go hand in hand: The TypeLayout rework makes creating TypeLayouts less error prone and moves potential layout errors (mainly for uniform layout rules) outside of the Any api, so that a user of the Any api may respond to layout errors before encoding time.

The PR is based on #19, which should be merged first. Decoupling it from #19 is not possible.

Changes

The main change is splitting Any::binding into three methods Any::storage_buffer_binding, Any::uniform_buffer_binding and Any::handle_binding:

// The current `Any::binding` function
pub fn binding(
     path: BindPath,
     visibility: StageMask,
     ty: ir::StoreType,
     binding_ty: BindingType,
     buffer_binding_as_ref: bool,
) -> Any {
    // This function has huge error surface
    // - `StoreType` and `BindingType` need to be matching variants
    // - `StoreType` contains layout information (custom_min_align/size) and may
    //    not follow the required layout rules of `BindingType::Buffer` (storage or uniform).
    // - `buffer_binding_as_ref == true` is only allowed for storage buffer bindings
}

// 1/3 new binding functions
pub fn storage_buffer_binding(
    bind_path: BindPath,
    visibility: StageMask,
    layout: GpuTypeLayout<repr::Storage>,
    access: AccessModeReadable,
    buffer_binding_as_ref: bool,    
    has_dynamic_offset: bool,
) -> Any {
    // Error surface is massively reduced:
    // - `BindingType` is now created inside of the function.
    // - `GpuTypeLayout<repr::Storage>` replaces `StoreType` and guarantees that it's
    //    useable for a storage buffer, so no layout errors can occur within the function.
}

// 2/3 new binding functions
 pub fn uniform_buffer_binding(
     bind_path: BindPath,
     visibility: StageMask,
     layout: GpuTypeLayout<repr::Uniform>,
     has_dynamic_offset: bool,
) -> Any {
    // Error surface similarly reduced. Note that we don't have a 
    // `buffer_binding_as_ref` parameter.
}

// 3/3 new binding functions
pub fn handle_binding(bind_path: BindPath, visibility: StageMask, handle_type: HandleType) -> Any {
    // Speaks for itself.
}

Open questions

  1. I stumbled upon this function. I was assuming that custom_min_align and custom_min_size can't be too small, because of min. Can this function be removed or is it supposed to exist?
  2. Trait bounds are quite the mess now, especially for the Array implementations. I believe the entire Buffer(Ref) code could be massively cleaned up by removing the indirection through GpuStore. I have ideas on how to do this, but I'd prefer to do it as a separate PR. Removing the GpuStore dependency of Buffer(Ref) will allow for a lot of trait bound cleanup.

This PR is functional, but needs cleanup. Not yet ready for review!

chronicl avatar May 30 '25 05:05 chronicl