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

Mesh shaders - initial wgpu hal changes #7089

Open
wants to merge 16 commits into
base: trunk
Choose a base branch
from

Conversation

SupaMaggie70Incorporated
Copy link

@SupaMaggie70Incorporated SupaMaggie70Incorporated commented Feb 8, 2025

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

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • Add change to 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.

@SupaMaggie70Incorporated SupaMaggie70Incorporated changed the title Mesh shading/wgpu hal Mesh shaders - initial wgpu hal changes Feb 10, 2025
@cwfitzgerald
Copy link
Member

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 :)

@SupaMaggie70Incorporated
Copy link
Author

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!

@cwfitzgerald
Copy link
Member

It is saying it's failing some benchmarks in the CI, is that a concern

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

@SupaMaggie70Incorporated SupaMaggie70Incorporated marked this pull request as ready for review February 13, 2025 03:38
@SupaMaggie70Incorporated SupaMaggie70Incorporated requested a review from a team as a code owner February 13, 2025 03:38
.multiview_mesh_shader(
needed
&& multiview
&& phd_features.mesh_shader.unwrap().multiview_mesh_shader == 1,

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 {

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.
@SupaMaggie70Incorporated
Copy link
Author

@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 :)

@cwfitzgerald
Copy link
Member

Do I have to do something to kickstart the checking process?

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.

@cwfitzgerald
Copy link
Member

Alright, will add this to my list to review properly before we land!

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

Successfully merging this pull request may close these issues.

2 participants