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

Handling of empty layouts #14

Closed
kvark opened this issue May 17, 2021 · 15 comments
Closed

Handling of empty layouts #14

kvark opened this issue May 17, 2021 · 15 comments

Comments

@kvark
Copy link
Contributor

kvark commented May 17, 2021

See gfx-rs/wgpu#1390 (comment)
Is this supposed to be handled by this library?

@zakarumych
Copy link
Owner

Descriptor set layouts with zero bindings are valid.

@kvark
Copy link
Contributor Author

kvark commented May 17, 2021

So this is a bug in gpu-descriptor then?

@almarklein
Copy link

To be precise, not sure if it matters, but the observed crash happens when a bind group with zero entries is created (in wgpuDeviceCreateBindGroup).

@zakarumych
Copy link
Owner

I see where problem comes from. While it is valid to create descriptor set layout with no descriptors, it is invalid to create descriptor pool with no descriptors. Which is weird. Why not allow or forbid both.
As a workaround, gpu-descriptor may add one dummy descriptor to the pool which won't be used.

@zakarumych
Copy link
Owner

Probably DescriptorDevice implementation may do so, as it is a Vulkan quirk.

@zakarumych
Copy link
Owner

Conceptually DescriptorDevice may return dummy pool which will be used to allocate dummy descriptor sets with no actual resources inside. But empty layout is kinda edge case, so supporting it somehow without affecting common use case is preferable.

@kvark
Copy link
Contributor Author

kvark commented May 17, 2021

Well that was quick. Thank you!

@zakarumych
Copy link
Owner

IIRC wgpu rolls its own DescriptorDevice for some reason. Same change should be applied there then

@kvark
Copy link
Contributor Author

kvark commented May 17, 2021

It's supposed to be just a glue between gpu-descriptor and gfx-hal. If gpu-descriptor doesn't consider this in scope, then we'd move it into the glue. But I think it's better to have gpu-descriptor handling this.

@zakarumych
Copy link
Owner

I think it's better to have Vulkan specific problem be handled in Vulkan API glue. gpu-descriptor will treat allocated pool as empty after all.
Maybe right in gfx-backend-vulkan?

@kvark
Copy link
Contributor Author

kvark commented May 17, 2021

This crate - gpu-descriptor - is supposed to be compatible with all the Vulkan users, right? So Ash, Erupt, gfx, and others potentially. So the problem affects everybody. If we put a fix in the glue, or into gfx-backend-vulkan, it's going to be fine for us. But it means that all the other clients will have to put in similar fixes. That doesn't sound good, comparing to a single place that address everything - right here.

@zakarumych
Copy link
Owner

You are correct. But how about other APIs? I suppose there's no such problem in gfx-backend-dx12 and metal.

@zakarumych
Copy link
Owner

I guess it is not big deal, giving that empty layout is degenerate case anyway.

@kvark
Copy link
Contributor Author

kvark commented May 17, 2021

On gfx-rs side, other APIs don't care about this. So we could indeed put it into gfx-backend-vulkan, but then all the other users of gpu-descriptor would have to do the same.

@zakarumych
Copy link
Owner

Ok. This sounds convincing enough. I'll put workaround into gpu-descriptor.

zakarumych added a commit that referenced this issue May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants