Hey, I am working on something similar so maybe we should combine our efforts.

Sorry, but I have some issues with your current system:

Every time you set a uniform value, it needs to query the uniform location first. This could potentially lead to a performance loss since most shaders will require you to update multiple uniforms per frame.

getGLVersion shouldn't be a public method of ShaderManager -- (a) because it doesn't return the expected verison (but instead just one digit) and (b) because, from an object-oriented point of view, it doesn't belong in a class about shaders

You should check for versions via the GLContext.getCapabilties().OpenGLXX booleans, and shader support with the GL_ARB_shader_objects (and vertex/frag objects) boolean also in ContextCapabilities

Right now there is a lot of duplicate code since you use GL20 and ARBShaderObjects. Ideally you should stick to one; if the version is >= 2.0 then the shader extension (ARBShaderObjects) should work regardless. If we want the slight benefit that GL20 code might provide then we should be implementing that in our SGL/Renderer classes.

You should use Slick's Log instead of System.out.print

Your programs don't have any error checking on the compiling/source code, so there's no way for the user to know whether there was a problem

All of your code could easily fit into a single class; and IMO it should be renamed to ShaderProgram. Multiple shaders (vertex, fragment, geometry) might link to a single OpenGL program

Every time you set a uniform value, it needs to query the uniform location first. This could potentially lead to a performance loss since most shaders will require you to update multiple uniforms per frame.

Thanks, I'd never thought of this issue. Obviously a biggie. How would this be solved? Would it be more/less efficient to have a map resolving variables to IDs?

Quote:

getGLVersion shouldn't be a public method of ShaderManager -- (a) because it doesn't return the expected verison (but instead just one digit) and (b) because, from an object-oriented point of view, it doesn't belong in a class about shaders

Understood.

Quote:

You should check for versions via the GLContext.getCapabilties().OpenGLXX booleans, and shader support with the GL_ARB_shader_objects (and vertex/frag objects) boolean also in ContextCapabilities

Done

I'm currently refining this class again. Currently I've got an abstract class called Program, which has (along with other stuff) a load method which returns a Shader. 2 classes extend this, ARBProgram and GLProgram, which both have their own implementations of their methods. This way no class is checking if it is ARB or OpenGL2+. I'm also investigating into supporting Geometry Shaders, however this would require a whole new level checking since it's only supported on OpenGL3.2+.

Quote:

All of your code could easily fit into a single class; and IMO it should be renamed to ShaderProgram. Multiple shaders (vertex, fragment, geometry) might link to a single OpenGL program

Also done. I'm thinking I could have a Program class, and if you wanted to say use a geometry shader, you could call a method attachShader(Type.geometry, source). How does this sound?

LibGDX maps the uniform locations to a hash map, which makes it pretty easy to call things like this:

Code:

shaderProgram.setUniform3f("blah", 2f, 2f, 2f);

I'd imagine it would be faster than asking GL to lookup the integer each time. The best way to do it in terms of sheer performance would be to let the user decide how to map the IDs -- i.e. give them a getUniformID(String) method that returns an int, then let them pass an ID using setUniform2f(int, float, float). This is something with already do with Sound and the play method, which returns an integer ID.

In terms of the ARB thing, just do it like so:

Add methods glCreateProgram/glGetUniformLocation/etc to SGL (this will break compatibility, but hopefully any user trying to extend SGL should realize that is is unstable and will be frequently changed to include new Slick features!)

In ImmediateModeOGL, implement the methods via ARBShaderObjects and constants via GL20 (as they map to the same value)

Call these abstracted methods from the ShaderProgram class

At a later point, we may decide to further abstract our rendering so that the SGL implementation decides whether to use ARB/EXT or GLXX.

Like I said, if GL20 is supported then ARBShaderObjects should also work. There are some slight benefits to using the GLXX classes instead if it's supported, which is why we should eventually abstract it (though it's not a huge priority). This also goes for FBO, PBuffer, and anything else in Slick that uses EXT/ARB.

Here's some changes to my old class. It uses a hash map for convenience, like LibGDX, and adds a Shader inner class (which maybe will be moved to outer leve) for shader/program independency.http://pastebin.com/kSXpYnAb

The releasing stuff is a bit messy. We'll have to add things like detachShader/attachShader if we really want it to be independent (and make shaders reusable over multiple programs).

Take a look at a more efficient version of my shader class here. I've implemented alot of the things mentioned in this thread (except for error checking, todo still). It uses a HashMap for efficient variable lookup. The variables are loaded into the HashMap when a shader is attached. The attachShader function allows for easy attaching of different types of shaders, and doesn't restrict the program to using a fragment and vertex shader. Currently I still have to implement geometry shader support.

Another efficient part of this code in comparison to my last implementation, is that it isn't using if statements everywhere to check if it is a GL20 or ARB shader. All of this works with ARB and GL20.

An example usage -

Code:

Program program = Program.create(); // Depending on the platform, will create either an ARB or GL20 programprogram.attachShader(ShaderType.VERTEX, "vertexshadersourcehere");program.finaliseProgram();

So can we take this into the dev branch? I would like to experiment a bit with shaders (especially providing some simple shaders)

Some Questions:

Shoulda specifiv Shader class implement methods like update to update the uniforms and so on?

What is the benifit in naming it "Program" instead of ShaderProgram?

Error locking would be cool, I suggest throwing the developer out of the game not spaming error messages?

You might want to use the ResourceLoader class for the source String

Edit:Also, on line 145. You try to get the length of an array, can you be sure the objuect is not null?Because I get a null pointer with no information at all :/ I guess because you forgot to load the source but expect to user to load it I would implement functions for that like davedes did.

@R.D. Sorry man, should've removed the code earlier, I forgot. It's not stable right now, plus it's also not efficient. My newer implementation is better (however still doesn't work ). I'll push to my fork later this week when I have some time.

Maybe you really want to look and davdes implementation. I got it working without even reading the source or anything Want you want for sure is loading the source into a string using the ResourceLoader! That's the proper way for Slick2D loading.

If we want to allow users to attach the same Shader to multiple Programs (which is not a very common requirement for simple games -- LibGDX doesn't support this, for example), then we need to tweak things (hence why I started writing an object-oriented Shader class along with ShaderProgram). We also need to take things like releasing() and possibly allowing for geometry shaders (which IMO is overkill for a 2D library).

If we don't require the above feature then the class will be much more minimal (more like my first draft).

We need to include all setUniform1f/setUniform2f/etc. as well as the same for setAttribute. This might be more "clutter" than simply including a method that allows for a float, but it will be more efficient for the end user.

In order to make it more Slick-friendly, we can introduce getUniform1f/getUniform2f/etc. -- noting in JavaDoc that it needs to create a new byte buffer per call (so should only really be used for convenience).

Using multiple textures (glActiveTexture) is not very slick-friendly at the moment, we'll have to figure something out with that. But many shaders only require one texture, which will be active by default AFAIK

Otherwise the class I posted works fine and is ready to at least be placed into the dev branch. I've been using it with my own game now with no problems.

I'm already working with you version although I can't seem to get a uniform to fit in. I just added a unfiform float to my fragment shader and for some reason it does not pop up in the uniform HashMap.

Uniforms are only "active" in GLSL if the compiler can determine that they are actually used; so "time" (since you don't use it in the shader) isn't considered an active uniform and thus its uniform location (i.e. ID) returns -1.http://ogltotd.blogspot.ca/2007/12/acti ... ertex.html

I'll have to implement something to ShaderProgram like hasUniform or isUniformActive, as well as some error checking within setUniformXX to ensure that no null pointers are thrown.

"Strict mode" is by default enabled; this means that trying to use setUniform1f on a non-active uniform will throw an illegal argument exception. You can disable strict mode to have these exceptions silently suppressed (i.e. so that altering a non-active uniform does nothing).

Who is online

Users browsing this forum: No registered users and 1 guest

You cannot post new topics in this forumYou cannot reply to topics in this forumYou cannot edit your posts in this forumYou cannot delete your posts in this forumYou cannot post attachments in this forum