attackgoat / screen-13

Screen 13 is an easy-to-use Vulkan rendering engine in the spirit of QBasic.
Apache License 2.0
252 stars 12 forks source link

Added RayQuery Support. #55

Closed DoeringChristian closed 1 year ago

DoeringChristian commented 1 year ago

I added the required extensions and features for RayQuery when the ray_tracing feature flag is set. Maybe there should be a separate flag that only enables RayQuery. What do you think?

Added the following features:

These are enabled when enabling ray_tracing. TODO: Evaluate if adding an option to only enable RayQuery makes sense.

attackgoat commented 1 year ago

I think this is a good idea - at least for now - but the change should probably expose the single rayQuery boolean so that users could check if support is actually present.

I ran into this with the vulkan_1_1_features field of Device. There is also a vulkan_1_2_features field and I was unsure if I should be Vulkan-like and leave them as-is or be "helpful" and put them together into one features field. I left them separate because I thought it would make it easier to understand them in relation to the Vulkan documentation. It's unlikely somebody uses Screen-13 and never references the Vulkan documentation - as easy as this crate makes things it's still really complicated.

So maybe a new ray_query_features field would be sensible, what do you think?

attackgoat commented 1 year ago

The other issue of whether or not to try to turn on the feature using the ray_tracing field of DriverConfig doesn't seem super critical to me. I have been mulling removing that config and instead taking an enum where the choices are something like:

DoeringChristian commented 1 year ago

I think this is a good idea - at least for now - but the change should probably expose the single rayQuery boolean so that users could check if support is actually present.

I ran into this with the vulkan_1_1_features field of Device. There is also a vulkan_1_2_features field and I was unsure if I should be Vulkan-like and leave them as-is or be "helpful" and put them together into one features field. I left them separate because I thought it would make it easier to understand them in relation to the Vulkan documentation. It's unlikely somebody uses Screen-13 and never references the Vulkan documentation - as easy as this crate makes things it's still really complicated.

So maybe a new ray_query_features field would be sensible, what do you think?

Sounds good. Would that be similar to the ray tracing properties field? Btw. I looked through the code trying to understand how features are enabled and I noticed that you implemented the PhysicalDeviceVulkan12Features struct? I persume there is a good reason that I can't find but wouldn't it be possible to just use the struct that ash provides?

attackgoat commented 1 year ago

Would that be similar to the ray tracing properties field?

Yes - it should probably be separate because it's possible for some vendor to implement ray query but not the pipelines.

... just use the struct that ash provides?

This was done to remove the s_type and next fields which can't be used in this context and also convert vk::Bool32 to bool for ease of use. It's a bit tedious to create new ones because I end up adding a bunch of copy-and-paste code but it also gives me an opportunity to copy the relevant documentation and links to the specifications so they're available on each member.

attackgoat commented 1 year ago

I'm merging this in to get it into v0.8.1 which I'd like to push out soon. The remaining question of how to enable the feature will likely go away in v0.9 when I hope to overhaul the device creation code and make it simpler, leaner.

Right now you get an error if you want ray tracing and it's not available; but I would rather the pattern be that we try to enable ray tracing things and if they're not available you just read the features to know you should render using a non-RTX path.