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

Sample shader improvements #84

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dante1130
Copy link

@dante1130 dante1130 commented Apr 3, 2024

Problem

Example shader class interface is error-prone

The example code for the shader class is very error-prone to use for users. This is from experience from some of my mates using this as a template for their project. This is because the public interface of the shader class has methods that are meant to be used in private implementations.

E.g. Someone with some knowledge of setting up OpenGL shaders, will know they have to first create the shader before attaching them. As such, they will call the method in this order:

Shader shader;
shader.create("vert.glsl");
shader.attach("vert.glsl");
...

However, the correct use of this is to actually just use the attach method.

The example for the shader class in the README omitted the step to link the shaders

There is a step missing from the example code given for the sample shader class, which is that the user has to call link to link the shaders to the shader program.

Someone, knowing no better, will be confused and trust the example code instead, while trying to fix seemingly non-existing bugs.

Solution

  • I've cleaned up the public interface of the shader class by moving methods that are meant to be called internally to the private region.
  • I've updated the README for the shader example so that the step to call link is included.

Testing

  • I've compiled a simple program that uses the modified shader class and ensured that it works.

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.

1 participant