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

Implement compute shaders support, make fragment shader non-mandatory #1154

Merged
merged 2 commits into from
Jun 22, 2024

Conversation

VReaperV
Copy link
Contributor

Requires #1124.

Cherry-picked from #1137 to split a large pr up a bit.

Adds some code to allow using compute shaders. Shaders can now have either of vertex, fragment, or compute shaders enabled/disabled.

Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

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

It seems like compute shaders are a totally separate workflow from the existing draw shaders, which would ideally be a separate class. Like instead of just GLShader you have one class with fragment shader, vertex shader, and vertex attribs; and another with just compute shader.

src/engine/renderer/gl_shader.cpp Outdated Show resolved Hide resolved
@VReaperV
Copy link
Contributor Author

VReaperV commented May 24, 2024

It seems like compute shaders are a totally separate workflow from the existing draw shaders, which would ideally be a separate class. Like instead of just GLShader you have one class with fragment shader, vertex shader, and vertex attribs; and another with just compute shader.

I considered that, the reason I made it part of GLShader is that they can still make use of the way uniforms are set, vertex/fragment includes, same paths for loading/compiling shaders including shader binaries, as well as GLHeaders. Maybe both classes should be derived from GLShader which would just provide the aforementioned functionality.

@slipher
Copy link
Member

slipher commented May 24, 2024

A really easy way to make things a bit clearer could be to have two constructors for GLShader, one for each use case. Instead of having a constructor that allows all 3 shader types at once.

@VReaperV
Copy link
Contributor Author

VReaperV commented May 24, 2024

So something like

GLShader( const std::string &name, const std::string &mainShaderName, uint32_t vertexAttribsRequired, GLShaderManager *manager,
const bool hasVertexShader, const bool hasFragmentShader)

and

GLShader( const std::string &name, const std::string &mainShaderName, uint32_t vertexAttribsRequired, GLShaderManager *manager,
const bool hasComputeShader

?

@slipher
Copy link
Member

slipher commented May 24, 2024

I guess there's nothing that distinguishes the constructors very well so maybe it would be easier to go ahead and define the subclasses even if all the functionality is in the base class. Like

class GLShaderBase {
    // everything that's in GLShader right now
};
class GLShader : public GLShaderBase { //draw shader
   // constructor does _hasFragment = true, _haveVertex = true?
};
class ComputeShader : public GLShaderBase {
    // constructor does _hasCompute = true
};

@VReaperV
Copy link
Contributor Author

Yeah, I ran into some issues with GLShader constructors earlier too, since the wrong one was being selected with some types being converted.

@VReaperV
Copy link
Contributor Author

I guess there's nothing that distinguishes the constructors very well so maybe it would be easier to go ahead and define the subclasses even if all the functionality is in the base class. Like

class GLShaderBase {
    // everything that's in GLShader right now
};
class GLShader : public GLShaderBase { //draw shader
   // constructor does _hasFragment = true, _haveVertex = true?
};
class ComputeShader : public GLShaderBase {
    // constructor does _hasCompute = true
};

This should be evaluated later because #1105 and some further PRs add more GLShader constructors.

Copy link
Member

@illwieckz illwieckz left a comment

Choose a reason for hiding this comment

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

LGTM.

@illwieckz
Copy link
Member

Don't forget to squash the fix commit. ;-)

Adds some code to allow using compute shaders. Shaders can now have either of vertex, fragment, or compute shaders enabled/disabled.
@VReaperV
Copy link
Contributor Author

Yep, done.

@VReaperV VReaperV merged commit 1b2cece into DaemonEngine:master Jun 22, 2024
9 checks passed
@VReaperV VReaperV deleted the compute-shaders branch June 22, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants