Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danielzgtg
Copy link
Contributor

  • Added an entry to CHANGELOG_VULKANO.md or CHANGELOG_VK_SYS.md if knowledge of this change could be valuable to users
  • (not simple enough for users) Updated documentation to reflect any user-facing changes - in this repository
  • (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.
  • 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
Copy link
Contributor Author

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
Copy link
Contributor

@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 👍

@danielzgtg
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor

@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.

@Eliah-Lakhin Eliah-Lakhin added the PR: Approved. Blocked. Maintainer approved the PR in general, but the merging is temporary blocked by some reasons. label Apr 10, 2021
@JohnPeel
Copy link

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

@AustinJ235
Copy link
Member

#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.

@AustinJ235 AustinJ235 added PR: Approved. Additional fixes required. Maintainer approved the PR in general. Additional Actions required from the Author to continue. and removed PR: Approved. Blocked. Maintainer approved the PR in general, but the merging is temporary blocked by some reasons. labels Jul 26, 2021
@Eliah-Lakhin
Copy link
Contributor

@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.

@AustinJ235 AustinJ235 added the PR: Long Standing There were no updates from the Author for a long time. label Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Approved. Additional fixes required. Maintainer approved the PR in general. Additional Actions required from the Author to continue. PR: Long Standing There were no updates from the Author for a long time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants