-
Notifications
You must be signed in to change notification settings - Fork 995
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
Mesh shaders - initial wgpu hal changes #7089
base: trunk
Are you sure you want to change the base?
Mesh shaders - initial wgpu hal changes #7089
Conversation
Please enter a commit message to explain why this merge is necessary,
Going to look at your proposal in a minute, but there's nothing really controversial in here, seems pretty straight forward. The spicy part is validation :) |
It is saying it's failing some benchmarks in the CI, is that a concern? Otherwise, should I mark this as a non-draft PR? Thanks! |
Yeah the error message is about a violation of a wgpu-hal precondition about features being violated, so there's something going on with wgpu-hal's features. After that is fixed, feel free to mark as ready-to-go |
.multiview_mesh_shader( | ||
needed | ||
&& multiview | ||
&& phd_features.mesh_shader.unwrap().multiview_mesh_shader == 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the best way to do this. I had to change stuff around so that whether this was supported was visible in this context. Worst comes to worst, we can forget multiview in mesh shaders for now. The main problem is that apparently llvmpipe supports mesh shaders and multiview but not multiview in mesh shaders.
@@ -143,6 +147,9 @@ impl PhysicalDeviceFeatures { | |||
if let Some(ref mut feature) = self.robustness2 { | |||
info = info.push_next(feature); | |||
} | |||
if let Some(ref mut feature) = self.multiview { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was missing for some reason, I spent quite a lot of time very confused.
The CI pooped itself, hopefully this fixes that. Will probably be undone either way.
@cwfitzgerald Sorry to bug you again. CI says there are 30 checks which haven’t completed, but are expected and waiting to be reported. But then it also says there are no checks to be done. Do I have to do something to kickstart the checking process? For some of the previous commits, it hasn’t needed any prompting. Otherwise, I think this PR is ready to be merged :) |
CI runs on the result of merging with trunk, so if there are conflicts with the merge, it can't run CI - so if you fix those conflicts, the CI will run. |
Alright, will add this to my list to review properly before we land! |
Connections
#3018
Description
Introduces basic mesh shading functionality to wgpu-hal
Testing
This change doesn't itself contain any tests. However, the functionality is tested and an example is provided on a separate branch of my fork.
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.Comments
This is one of my first PRs, so I probably did something wrong with regard to git or some other convention. Also, this is a very early draft PR. I just want some initial feedback.