vulkano-rs / vulkano

Safe and rich Rust wrapper around the Vulkan API
Apache License 2.0
4.53k stars 436 forks source link

Mesh shading example #2437

Closed Firestar99 closed 10 months ago

Firestar99 commented 10 months ago
  1. [ ] Update documentation to reflect any user-facing changes - in this repository. - None

  2. [ ] Make sure that the changes are covered by unit-tests. - None

  3. [x] Run cargo fmt on the changes.

  4. [x] Please put changelog entries in the description of this Pull Request if knowledge of this change could be valuable to users. No need to put the entries to the changelog directly, they will be transferred to the changelog file by maintainers right after the Pull Request merge.

    Please remove any items from the template below that are not applicable.

  5. [ ] Describe in common words what is the purpose of this change, related Github Issues, and highlight important implementation aspects.

Changelog:

### Additions
- A `mesh-shader` example.
marc0246 commented 10 months ago

Please be so kind as to rebase so Rua's commits aren't mixed in.

It can be done without any merge conflicts by rebasing onto Rua's branch first (for a remote named Rua):

git rebase Rua/mesh-shading

Then squashing her commits (pick her first commit and squash all her other commits into it):

git rebase -i b2bbe34896b0aa922dd904a0188e4288decaf6ce

Then you can rebase onto master without conflicts (making sure it's the upstream master not some outdated fork's):

git rebase master

There may be some more ergonomic way to go about it but this I've found works 100% of the time.

Firestar99 commented 10 months ago

Sure, I've squashed Rua's PR. Otherwise it was already rebased on Rua's PR, which was already rebased on current master.

Rua commented 10 months ago

But now this PR still contains the same changes as my PR, which makes reviewing it more difficult, as it's hard to separate your changes from mine.

marc0246 commented 10 months ago

You need to rebase onto master as well.

marc0246 commented 10 months ago

As for the first step, that's needed because Rua's branch has had more commits added to it than your fork. If you didn't rebase onto Rua's updated branch, you would get a merge conflict along the way.

Firestar99 commented 10 months ago

Ohhhh.. my remote for vulkano didn't update for some reason, I didn't notice you've merged ext_mesh_shaders already

marc0246 commented 10 months ago

That's why I say to make sure to use upstream master ;) Because this happens every time. I suggest making your local master branch track upstream/master, that is, our remote, and have all the other branches track your remote.

Firestar99 commented 10 months ago

That's already how my setup works, it's just that Intellij Idea's "update project" button seems to only fetch the origin of the checked out branch, not all branches...

Firestar99 commented 10 months ago

I'm not submitting this PR because it is ready, but because there is a weird struct layout issue here.

https://github.com/vulkano-rs/vulkano/blob/032f77709dcce49edd3fd487838c1473fbc5698a/examples/mesh-shader/shader.mesh#L47

First turn on LOAD_FROM_INSTANCE_BUFFER, which it is on the current latest commit, to use instance data loaded from buffers instead of generating all the geometry dynamically in the mesh shader.

https://github.com/vulkano-rs/vulkano/blob/032f77709dcce49edd3fd487838c1473fbc5698a/examples/mesh-shader/shader.mesh#L18-L22

On the GPU side this Instance struct uses STD140 layout, which causes it's alignment to be 16 bytes, but on the CPU it's aligned to 12 bytes. In the main.rs the InstanceData struct is currently defined manually, but even if I define it as

// also move the shaders here type InstanceData = mesh::Instance;

it still is misalignend, producing garbage output. Manually using repr(C, alignn(16)) also does not help.

marc0246 commented 10 months ago

I see, that is the default behavior of git fetch I believe. I use lazygit which fetches every remote automatically, I can quite recommend it.

marc0246 commented 10 months ago

I think the shaders should be named mesh.glsl and frag.glsl, for consistency with the other examples and easier syntax highlighting.

Firestar99 commented 10 months ago

Wouldn't it be more consistent if we've used glslang's file endings?

$ glslang --help
[..]
'file' can end in .<stage> for auto-stage classification, where <stage> is:
    .conf   to provide a config file that replaces the default configuration
            (see -c option below for generating a template)
    .vert   for a vertex shader
    .tesc   for a tessellation control shader
    .tese   for a tessellation evaluation shader
    .geom   for a geometry shader
    .frag   for a fragment shader
    .comp   for a compute shader
    .mesh   for a mesh shader
    .task   for a task shader
    .rgen    for a ray generation shader
    .rint    for a ray intersection shader
    .rahit   for a ray any hit shader
    .rchit   for a ray closest hit shader
    .rmiss   for a ray miss shader
    .rcall   for a ray callable shader
    .glsl   for .vert.glsl, .tesc.glsl, ..., .comp.glsl compound suffixes
    .hlsl   for .vert.hlsl, .tesc.hlsl, ..., .comp.hlsl compound suffixes

The last two options could also be a good alternative.

marc0246 commented 10 months ago

Those file endings are for automatic stage resolution, which we don't have, and I wouldn't call those endings standard at all. They don't even tell you what language it is. And as I already mentioned, syntax highlighting becomes harder. See for example how GitHub doesn't support the mesh extension yet. Granted, I use those extensions in my own projects, but I've had to do additional configuration in at least 2 editors I used. That's not exactly a quality I would be looking for in examples.

marc0246 commented 10 months ago

Can you please name the file frag.glsl? I realize this is me being very nit-picky but it's for consistency with the other examples.

marc0246 commented 10 months ago

Awesome, thanks. The only other thing that needs to be changed IMO is the formatting of the shaders. Namely, please use 4 spaces instead of the tabs like the other examples, and remove the multiple consecutive linefeeds.

Firestar99 commented 10 months ago

I've also added an output variable to the mesh shader, simply passing the triangle color, so that pretty much everything you may want to do with mesh shaders is covered. Apart from task shaders, but those aren't super complicated to understand, and should easily be transferable from any other example or tutorial.

https://github.com/vulkano-rs/vulkano/blob/e89f419648cca71cffcd2c062325b99aaec9a7cf/examples/mesh-shader/mesh.glsl#L38-L39

marc0246 commented 10 months ago

Actually something else I forgot, the Rust formatting is not like ours either. I assume this is rust-analyzer's formatting work. If you look in the .rustfmt.toml file and uncomment the lines, then run cargo +nightly fmt it should do it for you.

marc0246 commented 10 months ago

Thank you for the very nice work! I really appreciate it :heart:

Let's keep the one comment unresolved for a possible future alteration. I think it's fine as is.