Support fixed-length arrays in graphics pipeline in addition to from_iter

[danielzgtg]

• [x] Added an entry to CHANGELOG_VULKANO.md or CHANGELOG_VK_SYS.md if knowledge of this change could be valuable to users
• [x] (not simple enough for users) Updated documentation to reflect any user-facing changes - in this repository
• [x] (not simple enough for users) Updated documentation to reflect any user-facing changes - PR to the guide that fixes existing documentation invalidated by this PR.
• [x] Ran cargo fmt on the changes

This adds support for passing buffers statically typed as containing fixed-length arrays to draw calls.

It allows the following snippet of my code to work:

struct State {
    vertex_positions: Arc<ImmutableBuffer<[Vertex; 4]>>,
    instance_positions: Arc<DeviceLocalBuffer<[Instance; 10]>>,
    // [...]
}
// [...]
.draw(
    self.cursor_pipeline.clone(),
    &self.dynamic_state,
    (
        // After a lot of fighting with Rust, I found that the only solution that worked on stable Rust
        // was to just wrap all arguments of Content=[T; N] in another [T; 1]
        // It might be possible to improve this after const generics and specialization are stabilized
        [self.vertex_positions.clone()],
        self.instance_positions.clone()
            .into_buffer_slice().slice(0..self.positions_length).unwrap(),
        // It is now possible for either or both, in addition to no, arguments to be Content=[T; N]
    ),
    (),
    (),
    std::iter::empty(),
).unwrap()

This allows me to avoid preventing GPU optimizations when using CpuAccessibleBuffer, avoid having to copy stuff into the buffer each frame, be able to efficiently write to the buffer in a compute shader, and be able to do all of that with the safety of Rust type checking.

[danielzgtg]

It seems like the runner isn't using the latest stable Rust. Minimum const generics have been stabilized since late last year. Can someone fix the CI?

[Eliah-Lakhin]

@danielzgtg While I agree it would be a good idea to support static arrays out of the box, we can't relay on unstable Rust version yet unfortunately, since not all users might be using unstable Rust. So we need to at least await for the feature stabilization on compiler's side. Also, it would be nice to avoid extra wrapping to [N; 1].

But as a prototype for the future improvements I like your idea :+1:

[danielzgtg]

unstable Rust

I didn't add any line of code that started with #![feature. Therefore it should already work on stable Rust.

I think the problem is that our version of stable Rust is still last year's version

[danielzgtg]

Also, it would be nice to avoid extra wrapping to [N; 1].

I had already tried but I wasn't able to avoid the wrapping. I get errors about conflicting implementations of the trait. The only thing that might work is Rust's specialization, which is no longer planned to be merged any time soon. The other option is to attempt a rewrite of our use of generics throughout the entire library, something neither the end users nor I would like

[danielzgtg]

This works fine in stable rust:

$ rustc --version
rustc 1.51.0 (2fd73fabe 2021-03-23)
$ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/home/.rustup

installed toolchains
--------------------

stable-x86_64-unknown-linux-gnu (default)
nightly-x86_64-unknown-linux-gnu

active toolchain
----------------

stable-x86_64-unknown-linux-gnu (default)
rustc 1.51.0 (2fd73fabe 2021-03-23)

[Eliah-Lakhin]

@danielzgtg Well, there is a chance some users might still be using older versions of Rust too. I think it's important to have some level of back compatibility even if there is a lack of newest features. Anyway, to proceed with merging we need to upgrade Rust in Github Actions, and I don't have much experience with this stuff.

btw, what was the motivation for the feature implementation? Is it a blocker for your work that uses Vulkano? I agree it's not bad to have static arrays, but at first glance it doesn't seem to be too critical to use runtime-sized, and in most cases users probably would prefer this option.

[danielzgtg]

btw, what was the motivation for the feature implementation

I ported something over from wgpu-rs and I set everything to fixed-length there.

I also planned to optimize my game by dynamically varying the number of instances to draw while keeping the buffer size fixed to avoid re-allocations. Another game would also have the concern of safely using compute shaders.

Is it a blocker for your work that uses Vulkano

It's not really critical. I could always just wrap all my usages with an extra copy through CpuAccessibleBuffer. A performance decrease is expected if I do that though.

runtime-sized

I never use runtime-sized slices for performance and safety reasons.

might still be using older versions of Rust too

We aren't version 1.0 yet and the changelog is full of breaking changes. A breaking MSRV change shouldn't be much more

[Eliah-Lakhin]

@danielzgtg Understood. ok, I will try to propagate this change, but it may take some time. I need at least figure out how to update Github Actions.

[Eliah-Lakhin]

@AustinJ235 Do you know how to upgrade Rust version in Github Actions? @danielzgtg suggesting a feature that requires const generics. The implementation is a bit controversial, but I think it would be good to have this feature.

[JohnPeel]

You can use the rust-toolchain action to update the toolchain in your workflow.

[AustinJ235]

https://github.com/vulkano-rs/vulkano/pull/1619 has changed VertexDefinition, so this pull request needs to be updated. Is the rustc version used by workflows by default still a blocking issue here? I kind of put this off to the side and forgot about it. I am not to experienced with workflows in general, but as mentioned:

You can use the rust-toolchain action to update the toolchain in your workflow.

Seems like a solution for if we wanted to have separate nightly tests also along with a more up to date stable.

[Eliah-Lakhin]

@danielzgtg We finally updated to the latest Rustc, so I think we can proceed with this change now. Can you update your Pull Request please, and me or Austin will proceed with merging.

Thank you for your work again, and I apologize for the too long review process.