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

[3.x] Add BLEND_MODE_DISABLED for spatial shaders. #56279

Closed

Conversation

GlitchedCode
Copy link

@GlitchedCode GlitchedCode commented Dec 27, 2021

I made this pull request by manually reviewing the changes in 35278 for use in the 3.x branch, and added relevant documentation for the new BlendMode enum value.

As is stated in the original PR, this is a really useful feature for multi-pass compute involving tightly packed data in color channels. For my use case, I need a solution (implemented via render mode in this case) that i can pair with the unshaded render mode to be able to modify the alpha channel to a value other than 1 without the graphics driver subsequently modifying the values in the albedo channels, so that i can use the alpha channel for data transfer as well as the albedo ones.

@GlitchedCode GlitchedCode requested review from a team as code owners December 27, 2021 11:52
@Chaosus
Copy link
Member

Chaosus commented Dec 27, 2021

Commits need to be squashed and renamed to match a topic name. Besides that, you need to create a master version of this PR. Changes should always be made for the master/main branch first unless it's something very specific to the previous version.

@GlitchedCode
Copy link
Author

GlitchedCode commented Dec 27, 2021

Sorry for wasting your time, I'm new to contributing on Github, and I find this feature to be very useful for a project i'm working on, so I patched it onto the 3.x branch, altough I do intend to look into implementing this in the master branch as well. I would then have to open a separate PR, correct?
I squashed the commits and changed the commit message as stated, but I can see a lot of automated tests failing in the Github workflow, and will try to fix them on my end.

@Calinou
Copy link
Member

Calinou commented Dec 27, 2021

I believe I've already opened a pull request for this feature in both master and 3.x: #51711, #51844

@GlitchedCode
Copy link
Author

GlitchedCode commented Dec 27, 2021

I believe I've already opened a pull request for this feature in both master and 3.x: #51711, #51844

I tested the PR you linked on a custom build that also integrates the changes from this PR and #51708, and it doesn't completely fit my use case, which i will include in more detail the PR's description.

example project adapted from #51708: 32bpcViewportTest.zip
For some reason disabling transparent bg on the viewport fixes the issue with both render modes.

@Chaosus
Copy link
Member

Chaosus commented Dec 27, 2021

I can see a lot of automated tests failing in the Github workflow, and will try to fix them on my end.

You can click on Details to see why this happens and fix it. In that case initialization of desired_blend_mode is missing:

image

@Chaosus
Copy link
Member

Chaosus commented Dec 27, 2021

There is a violation of the static check - use clang-format tool on the files you've changed to fix it (https://docs.godotengine.org/en/latest/community/contributing/code_style_guidelines.html?highlight=clang-format#using-clang-format-locally)

@GlitchedCode
Copy link
Author

Ok, i think it should pass all checks now, thank you for the instructions.

@GlitchedCode
Copy link
Author

Over the last days i've been looking into implementing this PR for the master branch as well. Not reading the contributing guidelines carefully led me to skip submitting an issue to the godot-proposals repository. Should I submit an proposal going more in depth about the use case before submitting a new PR that implements these changes in master?

@reduz
Copy link
Member

reduz commented Feb 5, 2022

I think for Godot 4.x we might need a better solution for these types of use cases. For 3.x not up to me to decide waht can be done.

@GlitchedCode
Copy link
Author

GlitchedCode commented Feb 7, 2022

I think for Godot 4.x we might need a better solution for these types of use cases. For 3.x not up to me to decide waht can be done.

The thing is this functionality is already present for canvas_item shaders in 3.x, and not having it for spatial shaders is limiting when tackling problems that can be solved with multi-pass materials that use discard. Users may be forced to rely, at the very least, on multiple CanvasItem nodes. I agree this particolar solution may be outphased by compute shader support in 4.x though.

@akien-mga
Copy link
Member

This should likely also be implemented in the GLES2 renderer.

@clayjohn
Copy link
Member

This should likely also be implemented in the GLES2 renderer.

Yes, this will need to be added to both GLES2 and GLES3 before being merged.

My outstanding concern is that this same mode will be very difficult in 4.0 and will naturally behave differently (as 3D and 2D use different texture color formats). Further, even in 3.x, depending on how this is being used for calculations it will behave different on mobile and desktop (mobile can't render to RGBA16 buffers, it uses RGB10_A2 for non-transparent and RGBA8 for transparent. In GLES2 we always use RGB8 or RGBA8).

Some more details about your use-case would be helpful because, ultimately, we want to ensure that we find a solution to your particular use-case and create a feature that is intuitive and robust for other users as well.

@GlitchedCode
Copy link
Author

GlitchedCode commented Feb 21, 2022

Some more details about your use-case would be helpful [...]

This feature essentially lets me easily use all four color channels when writing compute shaders in Godot 3.x. For this to happen in spatial shaders, there are two main requirements (at least in my workflow) that must be satisfied:

  • Writing a value other than 1 in the alpha channel does not cause the other channel's values to be altered before being written to the framebuffer. (This is what [3.x] Add disable_alpha render mode to shaders #51844 accomplishes)
  • When reading from a viewport that includes objects using shaders with this render mode, the alpha channel's value in fragments that are output by such shaders must be the same as it was originally output by the fragment shader, meaning that the alpha channel can be used for data transfer and for compute purposes. (This is what the blend_disabled render mode in this PR and the one available for canvas_item shaders also accomplishes)

Due to my inexperience with Vulkan i've been struggling to find a solution for this in 4.0 as well, even though this feature, as far as i know, only benefits compute shaders, which limits its importance for 4.0 which will have a more robust compute shader support.

@clayjohn
Copy link
Member

My main concern is over the changing behaviour between platforms. In Godot 3.x, the scene shader was never designed to be used as a GPGPU (general-purpose computing on graphics processing units) shader. Further the framework around it assumes that it is doing traditional forward rendering of meshes. Accordingly, the engine will break your shader if you use it from other platforms (notably, GLES2, or any mobile device).

If you are doing general-purpose compute, is there any reason not to use CanvasItem shaders? CanvasItem shaders don't have any post-processing or tonemapping applied. Importantly, the texture you render to is the one you read from, so they suffer less from precision loss than Spatial shaders and would generally be more suitable for this kind of thing.

@GlitchedCode
Copy link
Author

In most cases, CanvasItem shaders are fine. This is for those instances where it's not enough: for example because implementing a multi-pass compute shader that operates on a significant viewport size requires you instance an additional viewport for each pass and consume several times more VRAM.

@clayjohn
Copy link
Member

clayjohn commented Feb 23, 2022

In most cases, CanvasItem shaders are fine. This is for those instances where it's not enough: for example because implementing a multi-pass compute shader that operates on a significant viewport size requires you instance an additional viewport for each pass and consume several times more VRAM.

How would this PR solve that issue? You would still need multiple viewports wouldn't you? Also, with CanvasItems, you can use BackBufferCopies to stack shaders without having to reallocate an entire Viewport.

To be clear, I hope I am not coming off as if I am needlessly criticizing your PR. I just think there may be a better solution for your problem and if there is, I would like to find that before introducing something new to the API that we won't be able to keep in the next upcoming version.

@GlitchedCode
Copy link
Author

GlitchedCode commented Feb 23, 2022

No worries, i recognize this is an edge case, I just revived the PR because it solved this problem on my particular situation.

How would this PR solve that issue?

CanvasItem shader materials do not support multi-pass shading, and Spatial shaders do not support
CanvasItem's blend_disabled mode. This PR is a solution to when you need both.

You would still need multiple viewports wouldn't you? Also, with CanvasItems, you can use BackBufferCopies to stack shaders without having to reallocate an entire Viewport.

You don't need to stack viewports if passes in the middle don't need to communicate with the next pass: discard and render priority are enough to achieve this if each shader pass reads from the same source. And the thing about BackBufferCopy is that still needs to allocate additional memory, while this solution does not.
In my use case, which is a simulation heavy on GPU computation that allows for a variable amount of passes, this solution is much more scalable as far as pass count and viewport size goes, since i went from using multiple viewports to a finite amount regardless of how complex it gets.

@clayjohn
Copy link
Member

situation

Ahh, I think I am beginning to understand what you are doing. Are you using a full screen quad with multiple chained next_pass materials set in the material? next_pass duplicates your geometry and renders it again after rendering the object. The equivalent in CanvasItems is to use multiple CanvasItems with the same geometry but different materials (because in 2D draw order is specified by the tree order). For example, if you are using a full screen quad, you could instead have a few ColorRects in order in the sceen tree. Do you use a complex 3-dimensional mesh? If, not CanvasItems may be the way to go as it allows you to specify draw order without having to worry about next_pass or render_priority it would put you in full control over the order of draw calls.

I suggested BackBufferCopy as I assumed subsequent passes were reading data from earlier passes.

@GlitchedCode
Copy link
Author

Thanks for the explanation, i will also try doing it this way.

@clayjohn
Copy link
Member

Please let me know how it goes! If this PR really does make the process way easier for you, then I am in favour of merging (we will have to sort out what to do about 4.0 though).

@akien-mga
Copy link
Member

This seems abandoned and there isn't much demand for users to add this feature in 3.x, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants