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 #2427

Merged
merged 4 commits into from
Aug 30, 2023
Merged

Support dual source blending #2427

merged 4 commits into from
Aug 30, 2023

Conversation

freqmod
Copy link
Contributor

@freqmod freqmod commented Aug 9, 2023

Support writing shaders with alt blender mode.
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.

See https://github.com/freqmod/helicoid/tree/master/helicoid-wgpu for usage example.
See gfx-rs/wgpu#1804 (comment) for related issue

@freqmod
Copy link
Contributor Author

freqmod commented Aug 9, 2023

I had a look into the clippy errors, and it seems to be in some kind of external crate that I have not changed.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

I can't leave a full review, since I'm not familiar with the domain and don't trust myself to validate shader output properly (CC @teoxoy), but most of the changes in code don't look controversial to me, assuming the shader output is correct. In addition to spanned commentary, here are some reviewer notes I made for myself:

src/back/msl/mod.rs Outdated Show resolved Hide resolved
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
@freqmod
Copy link
Contributor Author

freqmod commented Aug 10, 2023

@freqmod mentions adding support for DirectX, which I presume to mean HLSL, but I don't see the HLSL represented here. I found KhronosGroup/SPIRV-Cross#815, which looks like a potentially helpful model for what to do here (viz., use SV_Target${index}).

Regarding HLSL it may be that i only did some work on that in the WGPU patch (and not the naga patch) and mixed up.

Regarding example code: there is a shader in https://github.com/freqmod/helicoid/blob/master/helicoid-gpurender/shaders/text.fs.wgsl but it may be useful to get one in this (or the wgpu) repo for proper regression testing.

I will look further at this and address the comments later.

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Aug 10, 2023

We should definitely have shader snapshot tests for this here, independent of wgpu. That would also make the intent of the output of these changes clearer.

@freqmod
Copy link
Contributor Author

freqmod commented Aug 10, 2023

  • I did an attempt at some HLSL code based on the as suggested (still I haven't set up a windows dev environment).
  • I have added a test for dual source blending. I got some problems with the validation of bindings that I have hacked around, although that should probably be done more properly with some input from the maintainers of that code.
  • I also fixed the small code comments I got.

src/back/spv/writer.rs Outdated Show resolved Hide resolved
src/back/glsl/mod.rs Show resolved Hide resolved
src/back/glsl/features.rs Outdated Show resolved Hide resolved
src/valid/interface.rs Outdated Show resolved Hide resolved
src/back/msl/mod.rs Outdated Show resolved Hide resolved
src/front/wgsl/parse/mod.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
src/valid/interface.rs Show resolved Hide resolved
src/valid/interface.rs Outdated Show resolved Hide resolved
src/valid/interface.rs Outdated Show resolved Hide resolved
src/valid/interface.rs Show resolved Hide resolved
src/back/glsl/features.rs Outdated Show resolved Hide resolved
src/back/glsl/features.rs Outdated Show resolved Hide resolved
src/back/glsl/features.rs Outdated Show resolved Hide resolved
src/front/wgsl/parse/mod.rs Outdated Show resolved Hide resolved
tests/snapshots.rs Outdated Show resolved Hide resolved
src/front/wgsl/parse/mod.rs Outdated Show resolved Hide resolved
@freqmod
Copy link
Contributor Author

freqmod commented Aug 19, 2023

I accepted the proposed changes that i think are relevant. I will do another upload with manual changes to address the other comments later.

@freqmod
Copy link
Contributor Author

freqmod commented Aug 23, 2023

I would still prefer to have something like secondary_blend_source etc, but since you are more likely to maintain this in the future i think it is better to just go with your name and get this in without further bikeshedding.

@freqmod
Copy link
Contributor Author

freqmod commented Aug 23, 2023

I hope i have seen and addressed all the comments. If there are any minor fixes needed feel free to just do them if you prefer to avoid another review cycle.

@freqmod
Copy link
Contributor Author

freqmod commented Aug 23, 2023

Pushed rebase. It should not be any changes (except adaptions to upstream) since last push.

@teoxoy
Copy link
Member

teoxoy commented Aug 29, 2023

I would still prefer to have something like secondary_blend_source

How about second_blend_source?

I can make the change.

Otherwise, PR looks great! Thanks for all the changes :)

@freqmod
Copy link
Contributor Author

freqmod commented Aug 30, 2023

second_blend_source is good. I can do an upload. I'll also squash everything as the commits now are quite messy..

@teoxoy
Copy link
Member

teoxoy commented Aug 30, 2023

If you are at it, can you update the WGSL attribute and also I saw a few remaining places (esp. errors) where we still use "alternate". Thanks!

@freqmod
Copy link
Contributor Author

freqmod commented Aug 30, 2023

I will review the squashed version before pushing to fix any small things left.

@freqmod
Copy link
Contributor Author

freqmod commented Aug 30, 2023

Now I have addressed everything I can see. Feel free to do any changes on top of the last upload you need and merge when you are happy. Thanks for working on the review. I'll start to rebase and address the wgpu changes soon.

@teoxoy
Copy link
Member

teoxoy commented Aug 30, 2023

Thanks! I pushed a few more commits, let me know if they look good and we can land it!

@freqmod
Copy link
Contributor Author

freqmod commented Aug 30, 2023

Thanks. They look good to me, please land it.

@teoxoy teoxoy changed the title Support alt blender mode Support dual source blending Aug 30, 2023
@teoxoy teoxoy merged commit 0491d39 into gfx-rs:master Aug 30, 2023
fornwall added a commit to fornwall/naga that referenced this pull request Sep 1, 2023
fornwall added a commit to fornwall/naga that referenced this pull request Sep 1, 2023
fornwall added a commit to fornwall/naga that referenced this pull request Sep 1, 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.

3 participants