ash-rs / ash

Vulkan bindings for Rust
Apache License 2.0
1.82k stars 186 forks source link

unify slice interfaces #883

Closed ChaseLewis closed 6 months ago

ChaseLewis commented 6 months ago

One thing I noticed messing with Ash is some of the handcrafted method's return a Vec, but some methods don't. This seems weird, unnecessary and kinda against the spirit of matching the C++ code with no major compromises in release build.

Ex: Forces Vec

    /// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/vkAllocateDescriptorSets.html>
    #[inline]
    pub unsafe fn allocate_descriptor_sets(
        &self,
        allocate_info: &vk::DescriptorSetAllocateInfo<'_>,
    ) -> VkResult<Vec<vk::DescriptorSet>> {
        let mut desc_set = Vec::with_capacity(allocate_info.descriptor_set_count as usize);
        (self.device_fn_1_0.allocate_descriptor_sets)(
            self.handle(),
            allocate_info,
            desc_set.as_mut_ptr(),
        )
        .set_vec_len_on_success(desc_set, allocate_info.descriptor_set_count as usize)
    }

Ex: Does not return Vec

    /// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/vkGetImageSparseMemoryRequirements2.html>
    ///
    /// Call [`get_image_sparse_memory_requirements2_len()`][Self::get_image_sparse_memory_requirements2_len()] to query the number of elements to pass to `out`.
    /// Be sure to [`Default::default()`]-initialize these elements and optionally set their `p_next` pointer.
    #[inline]
    pub unsafe fn get_image_sparse_memory_requirements2(
        &self,
        info: &vk::ImageSparseMemoryRequirementsInfo2<'_>,
        out: &mut [vk::SparseImageMemoryRequirements2<'_>],
    ) {
        let mut count = out.len() as u32;
        (self.device_fn_1_1.get_image_sparse_memory_requirements2)(
            self.handle(),
            info,
            &mut count,
            out.as_mut_ptr(),
        );
        assert_eq!(count as usize, out.len());
    }

Imo, the library should move all methods to match the C++ version so the end user can determine how they want to allocate the memory or potentially reuse existing memory.

Ex: Update descriptor sets to not use Vec

    /// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/vkAllocateDescriptorSets.html>
    #[inline]
    pub unsafe fn allocate_descriptor_sets(
        &self,
        allocate_info: &vk::DescriptorSetAllocateInfo<'_>,
        descriptor_sets: &mut[vk::DescriptorSet]
    ) -> VkResult<()> {
        assert_eq!(allocate_info.descriptor_set_count,descriptor_sets.len() as u32);
        (self.device_fn_1_0.allocate_descriptor_sets)(
            self.handle(),
            allocate_info,
            descriptor_sets.as_mut_ptr()
        ).result()
    }
MarijnS95 commented 6 months ago

Mind if I temporarily close & lock this PR so that we can discuss the (much larger than what is shown here) scope of this topic in the duplicate issue https://github.com/ash-rs/ash/issues/884? I'd hate to waste maintainers' time on discussing and cross-referencing the same points in two disconnected places.