Skip to content

feat: Added compute shader support for vulkan #2685

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: Added compute shader support for vulkan #2685

wants to merge 7 commits into from

Conversation

johang88
Copy link
Contributor

@johang88 johang88 commented Mar 22, 2025 •
edited
Loading

PR Details

Vulkan graphics backend has been modified to support compute shaders, the shader compiler has also been modified to support computer shader generation for GLSL.

Potential breaking changes

Shadercompiler: Enabled GL_EXT_samplerless_texture_functions for all GLSL shaders and removed the old "NoSampler" code, this greatly simplies the code gen and should work on most devices.

CommandList: Removed if (mapMode == MapMode.WriteDiscard) exception, I had some trouble getting my streaming terrain to work when the check was present. It seems to mostly work as it should but there are probably some scenarios where this will cause an issue, ofc it would have crashed in the previous version so I consider this to be an improvement.

Notable changes

  • Added StorageImage, StorageTexelBuffer, StorageBuffer descriptor types and support for these when missing. Descriptor types still have a static max count but it is now capped if greater than the physical device limit.
  • Added additional pixel formats.

ShaderCompiler

  • Added compute shader support for GLSL generation if compiling for vulkan, same path is most likely usable in the future for OpenGL.
  • Added compute specific intrinsics like gl_GlobalInvocationID and memoryBarrier functions.
  • Support for RWTexture / RWBuffer store and load.
  • Using std430 for non constant buffers as its neater.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@johang88
Copy link
Contributor Author

I have only tested with a simple compute shader so far but seems to work fine.

Sample code, note that the sprite rendering can cause some validation errors if done after the forward renderer, there are no validation errors if the sprite rendering is removed, but then I have to validate the output in renderdoc :D

public class ComputeShaderRenderer : SceneRendererBase
{
    private ComputeEffectShader _shader;
    private Texture _texture;

    private SpriteBatch _sprite;

    protected override void DrawCore(RenderContext context, RenderDrawContext drawContext)
    {
        _shader ??= new(context) { ShaderSourceName = "TestShader" };
        _texture ??= Texture.New2D(context.GraphicsDevice, 64, 64, PixelFormat.R8G8B8A8_UNorm, TextureFlags.UnorderedAccess | TextureFlags.ShaderResource);
        _sprite ??= new(context.GraphicsDevice);

        drawContext.CommandList.ResourceBarrierTransition(_texture, GraphicsResourceState.UnorderedAccess);

        _shader.Parameters.Set(TestShaderKeys.TestTexture, _texture);
        _shader.ThreadGroupCounts = new Int3(64 / 8, 64 / 8, 1);
        _shader.ThreadNumbers = new Int3(8, 8, 1);

        _shader.Draw(drawContext, "TestShader");

        drawContext.CommandList.ResourceBarrierTransition(_texture, GraphicsResourceState.PixelShaderResource);

        _sprite.Begin(drawContext.GraphicsContext, SpriteSortMode.Immediate);
        _sprite.Draw(_texture, new(100, 100));
        _sprite.End();
    }
}

Shader:

shader TestShader : ComputeShaderBase
{
	rgroup Test
	{
		stage RWTexture2D<float4> TestTexture;
	}

	override void Compute()
	{
		TestTexture[streams.DispatchThreadId.xy] = float4((float)streams.DispatchThreadId.x / 64, (float)streams.DispatchThreadId.y / 64, 0, 1);
	}
};

Mandatory screenshot of the very fancy rectangle:
bild

@johang88 johang88 marked this pull request as draft March 22, 2025 10:59
@johang88
Copy link
Contributor Author

johang88 commented Mar 22, 2025 •
edited
Loading

Got my FFT shaders to work, it seems to work pretty well now. Note the commits from #2684 are currently included.

I got rid of the NoSampler sampler hack and am using GL_EXT_samplerless_texture_functions, not sure if it's a good idea or not but it looks like it should be supported pretty well and it's easier to manage.

I also noticed that the resource barrier api is a bit lacking as it wouldn't insert a barrier as my texture was already in UnorderedAccess, I had to insert an extra transition just to get the barrier in place. Not sure if we want to remove the check that skips the transition or maybe add an extra method argument that can force the barrier to be inserted? My hack works but it's not neat ...

context.CommandList.ResourceBarrierTransition(buffer, GraphicsResourceState.PixelShaderResource);
context.CommandList.ResourceBarrierTransition(buffer, GraphicsResourceState.UnorderedAccess);

johang88 added 2 commits June 12, 2025 10:57
@johang88
feat: Added compute shader support for vulkan
8819d65 
Vulkan graphics backend has been modified to support compute shaders,
additional modifications were also made to the shader compiler so that correct
glsl compute shaders can be generated.
@johang88
Using GL_EXT_samplerless_texture_functions instead of NoSampler
19ecc1d
@johang88 johang88 marked this pull request as ready for review June 12, 2025 12:20
Kryptos-FR
Kryptos-FR reviewed Jun 12, 2025
View reviewed changes
Copy link
Member

@Kryptos-FR Kryptos-FR left a comment

Choose a reason for hiding this comment

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

Can't really review the graphics code as that is not my area of expertise. With that said, I have a few minor remarks.

sources/engine/Stride.Graphics/Vulkan/CommandList.Vulkan.cs Outdated
Comment on lines 1330 to 1334
// Maybe it just works if removed?
//if (mapMode == MapMode.WriteDiscard)
//{
// throw new InvalidOperationException("Can't use WriteDiscard on Graphics API that doesn't support renaming");
//}
Copy link
Member

Choose a reason for hiding this comment

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

So, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to work, but there are probably scenarios that still fail, but I think it should be fine for now. To be fair I was certain I had removed this and not just commented it out but I guess it came back during all my rebases and cherry-picks ...

@johang88
Copy link
Contributor Author

Can't really review the graphics code as that is not my area of expertise. With that said, I have a few minor remarks.

Thanks for taking a look, a bit sloppy of me to leave those behind ... 😁

@Doprez
Copy link
Contributor

Doprez commented Jul 22, 2025

Is this good to merge? This should fix some cross platform issues as well which would be a nice win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kryptos-FR Kryptos-FR Kryptos-FR left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johang88 @Doprez @Kryptos-FR