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

Support dual source blending #4022

Merged
merged 6 commits into from
Sep 19, 2023
Merged

Support dual source blending #4022

merged 6 commits into from
Sep 19, 2023

Conversation

freqmod
Copy link
Contributor

@freqmod freqmod commented Aug 9, 2023

Support writing shaders with alt blender mode.
See https://github.com/freqmod/helicoid/tree/master/helicoid-wgpu for usage example.

Checklist

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file. TBD: This is a PR to get initial feedback.

Connections
Resolves #1804.
Depends on gfx-rs/naga#2427.

Description
See issue 1804 (linked above)

Testing
Tested on linux with OpenGL, and Vulkan, and on Mac using metal. I have made some changes for Direct X but since I don't have a windows build environment I haven't tested that.
Tested in the helicoid-wgpu crate in external repo, I guess a test or example could be made if we deem it useful.

@freqmod freqmod mentioned this pull request Aug 9, 2023
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Show resolved Hide resolved
wgpu/src/backend/web.rs Outdated Show resolved Hide resolved
wgpu-hal/src/metal/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/conv.rs Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-core/src/validation.rs Show resolved Hide resolved
@freqmod
Copy link
Contributor Author

freqmod commented Aug 12, 2023

Thanks for the feedback, they make sense. I think i would like to finish the naga review first, when that is done i will look into these comments.

@freqmod
Copy link
Contributor Author

freqmod commented Sep 6, 2023

I hope I have addressed the issues you pointed out.

I tested briefly with my simple application. It works when everything is set up correctly. It stops when the feature is not defined, and it complains when blend is enabled and the shader does not have an output with it.

I also ran the unittests and they seem to pass except one which also fails in trunk.

Cargo.toml conflicts as i have updated the hash to contain the naga changes.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes!
Besides a few naming/phrasing suggestions, I think the validation can be improved a bit more.

wgpu-hal/src/metal/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/metal/adapter.rs Outdated Show resolved Hide resolved
wgpu-core/src/validation.rs Outdated Show resolved Hide resolved
wgpu-core/src/pipeline.rs Outdated Show resolved Hide resolved
wgpu-core/src/pipeline.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

This should be the last round of review.
Note that the PR needs to be rebased.

wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-hal/src/metal/mod.rs Outdated Show resolved Hide resolved
@freqmod
Copy link
Contributor Author

freqmod commented Sep 18, 2023

I have addressed the issues in this round and performed a rebase. If the assumptions in the comment to resource.rs lines 2924 to 2931 l are correct i hope indeed it is the last chages, if not i guess we may need some more rounds. All unittests except wgpu-test::wgpu-tests shader::struct_layout::uniform_input pass on linux, but that test also fails on trunk.

@freqmod
Copy link
Contributor Author

freqmod commented Sep 18, 2023

seems like i have forgot clippy. Will fix, Fixed

… fragment stage. Also check if the entry point for the fragment stage uses dual source blending (up for discussion in the review)
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
@teoxoy teoxoy changed the title Alt blend Support dual source blending Sep 19, 2023
@teoxoy
Copy link
Member

teoxoy commented Sep 19, 2023

I pushed a few more tweaks. Let me know if they look good and we can merge this.

Thank you for implementing this new functionality in naga & wgpu!

@freqmod
Copy link
Contributor Author

freqmod commented Sep 19, 2023

The changes looks good to me, thanks for you patience with the multiple review rounds.

@teoxoy teoxoy merged commit dc5beac into gfx-rs:trunk Sep 19, 2023
bradwerth pushed a commit to bradwerth/wgpu that referenced this pull request Sep 19, 2023
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.

Dual Source Blending
3 participants