vulkano-rs / vulkano

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

Bindless Descriptor fixes and optimizations #2515

Closed Firestar99 closed 1 month ago

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

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

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

  4. [x] Run cargo +nightly fmt on the changes.

  5. [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.

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

Changelog:

### Additions
* added `DescriptorSet::invalidate()` to make vulkano forget about resources that bound to a descriptor_set, so they can be freed

### Bugs fixed
* fixed descriptor sets with `UPDATE_AFTER_BIND` or `PARTIALLY_BOUND` being wrongly validated on bind
Firestar99 commented 4 months ago

I've turned this PR into a draft, as I've added DescriptorSet::invalidate() to it and haven't gotten around to extensively test it tested it and it works :D

marc0246 commented 2 months ago

I think you already know this, but to anyone reading this, I would like to murder DescriptorSet before the next release. There's no point having all these collections in so many places that just keep references to Vulkan objects, cloning and dropping Arcs and allocating and deallocating the Vecs / HashMaps / whatever for them each and every frame multiple times, when there is one central resources collection that tracks everything in one place. I'm willing to merge this regardless if for nothing else but to not inconvenience you, but I wouldn't start designing something new around this invalidation API.

marc0246 commented 2 months ago

Regarding PARTIALLY_BOUND, I think we decided that all shader executions are unsafe, and it is documented that the shader must not access descriptors that haven't been bound. However, the logic here is not correct. It's only PARTIALLY_BOUND that permits this, and not UPDATE_AFTER_BIND.

Rua commented 1 month ago

If UPDATE_AFTER_BIND is enabled, then it's possible that the resources in the descriptor set, at the time the command is recorded, are not the same as those that will be bound when the command buffer is actually executed. So any record-time checks on the bound resources are probably superfluous?

Firestar99 commented 1 month ago

So any record-time checks on the bound resources are probably superfluous?

Yes they are, so for now I've just disabled them. Which means they are indeed not validated at all, and honestly I'd keep it that way as I don't think modding that into the old sync has any benefit. Rather invest that time into the new sync. We may want to make these flags unsafe, but wouldn't know how.