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

[wgpu-hal] Initial Vulkan ray query and acceleration structure support #3020

Closed
wants to merge 14 commits into from

Conversation

expenses
Copy link
Contributor

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections

#1040

Description

Still a work in progress, but I can ray-trace instanced triangles now:

20220912_23h26m53s_grim

Needs a cleanup and support for acceleration structure updates.

Testing
Explain how this change is tested.

@expenses
Copy link
Contributor Author

Besides documentation and dummy implementations for the other backends, I think this is ready for review.

@expenses expenses marked this pull request as ready for review September 15, 2022 10:10
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great work - have no massive comments. tbh I don't fully understand RT apis as I haven't used them, but from what I know about them, this design seems sane and low overhead.


let geometry_info = geometry_info.build();

let range = &[range][..];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What on earth?

Comment on lines +791 to +798
align_mask: if desc
.usage
.contains(crate::BufferUses::TOP_LEVEL_ACCELERATION_STRUCTURE_INPUT)
{
16
} else {
req.alignment
} - 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pull the if statement into a variable.

vk::WriteDescriptorSetAccelerationStructureKHR::builder()
.acceleration_structures(&raw_acceleration_structures[raw_start..]);

// todo: Dereference the struct to get around lifetime issues. Safe as long as we never resize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to do this, let's add an assertion that we always have room on push.

instance_custom_index_and_mask: pack_24_8(0, 0xff),
instance_shader_binding_table_record_offset_and_flags: pack_24_8(0, 0),
acceleration_structure_reference: unsafe {
device.get_acceleration_structure_device_address(&blas)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the kind of thing we'd hide at the wgpu-core level?

Comment on lines +687 to +689
const ACCELERATION_STRUCTURE_SCRATCH = 1 << 10;
const BOTTOM_LEVEL_ACCELERATION_STRUCTURE_INPUT = 1 << 11;
const TOP_LEVEL_ACCELERATION_STRUCTURE_INPUT = 1 << 12;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These probably need to be added to either INCLUSIVE or EXCLUSIVE in some way?

@cwfitzgerald
Copy link
Member

Poke from triage

@expenses
Copy link
Contributor Author

expenses commented Dec 8, 2022

This PR is currently on-pause because I don't have the time to work on it along with everything else I'm doing. I don't think it'll take much to get it to a mergeable state and if someone else wants to work on that then I'm happy to review their changes.

I wish there was a way to indicate this on github, lots of people (understandably) seem to be afraid to touch someone else's PR if it's still open

@cwfitzgerald cwfitzgerald added the help required We need community help to make this happen. label Dec 8, 2022
@seabassjh
Copy link
Contributor

@cwfitzgerald is there a list of TODOs left on this before being mergeable? I'd like to pick this up

@cwfitzgerald
Copy link
Member

@seabassjh love to have this be picked up - the main ones are the comments on the PR

@expenses
Copy link
Contributor Author

@cwfitzgerald is there a list of TODOs left on this before being mergeable? I'd like to pick this up

I imagine there's some work to be done to resolve #3044 etc as well

@daniel-keitel
Copy link
Contributor

I would like to help out.
@seabassjh have you continued work on this PR?
I have already made most of the requested changes, but I don't know how to contribute since it is @expenses PR.

@expenses
Copy link
Contributor Author

expenses commented Feb 2, 2023

@daniel-keitel if you link me your changes I can review them and merge them into this branch

@daniel-keitel
Copy link
Contributor

daniel-keitel commented Feb 20, 2023

@expenses I created a pull request on your fork.
expenses#1

@expenses
Copy link
Contributor Author

Hi @daniel-keitel, sorry I missed this. It might be better if you created a new PR on wgpu and then the discussion moved over to there.

@cwfitzgerald
Copy link
Member

Closing on favor of 3507

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help required We need community help to make this happen.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants