BGE: New API to get 2D filter shader program
Needs ReviewPublic

Authored by Thomas Szepe (hg1) on Oct 21 2015, 7:36 PM.

Details

Summary

The new attribute shaderProgramNumber returns the actual shader program number.
Using the shader program number, allows to bind custom uniforms with the bgl wrapper, which were actually not provided by Blender itself.

Diff Detail

Thomas Szepe (hg1) retitled this revision from to BGE: New API to get 2D filter shader program.Oct 21 2015, 7:36 PM
Thomas Szepe (hg1) updated this object.

I already have found a way to add custom other custom uniforms to the 2D Filter.
http://blenderartists.org/forum/showthread.php?374488-Bind-custom-uniform-values-to-a-2D-filter-%28using-bgl-wrapper%29

But this method (loop over all shader numbers) to get the shader number is only working correct
If the custom shader actuator is the last one. If not or a second shader actuator is on the object, you will get a wrong program number.
That is the reason why this new API is needed.

Example file:

Porteries Tristan (panzergame) requested changes to this revision.Oct 21 2015, 9:38 PM

The patch not compile natively. Also and the test file create a bug for me, backtrace : http://www.pasteall.org/61824

source/gameengine/GameLogic/SCA_2DFilterActuator.cpp
36–37

It's needed for what ? if you have to access to the scene you can only include "SCA_IScene.h", and it's already included in the SCA_I2DFilterActuator.h. (all works without these headers)

[EDIT]
This not compile with cmake because /Ketsji is not added in CMakeList.txt

159

it will be not better to keep a pointer to the filter manager in SCA_2DFilterActuator ? (maybe less modifications)

source/gameengine/GameLogic/SCA_2DFilterActuator.h
75

Please don't mix camel case and _, here it should be pyattr_get_shader_program_number since we use this type for all python attribute declaration.

source/gameengine/Ketsji/KX_Scene.cpp
2163

This function should be const.

2165

I don't like that kind of conversion variable -> pointer for classes/structs, maybe m_filtermanager should be already a pointer in KX_Scene.

source/gameengine/Rasterizer/RAS_2DFilterManager.cpp
306

This function can be set under const.

source/gameengine/Rasterizer/RAS_2DFilterManager.h
108

It will be nice to add a little comment to describe what number returns this function.

This revision now requires changes to proceed.Oct 21 2015, 9:38 PM

Files in the GameLogic folder are not supposed to depend on Ketsji code. This is why you'll find some logic bricks in the Ketsji folder. Whether this design decision still makes sense is another issue.

Thomas Szepe (hg1) marked 4 inline comments as done.Oct 21 2015, 11:31 PM
Thomas Szepe (hg1) added inline comments.
source/gameengine/GameLogic/SCA_2DFilterActuator.cpp
36–37

This includes are not necessary. I only forgotten to delete it.

159

I am not sure how you mean this. There is only one filter manager class instance for each scene.

source/gameengine/Ketsji/KX_Scene.cpp
2163

I don't see much benefits to make everything const. A programmer should always know what he does.
But OK I will change this.

2165

M_filtermanager is single class instance, not generated with new/delete.
If you want really to change this, then we need to use new/delete or use a separate variable to store a pointer reference.
This need some more changes in the source.

source/gameengine/Rasterizer/RAS_2DFilterManager.h
108

Like this? Returns the shader program number.

I don't know if it is really useful to make a description, when the method name already describe it properly.

source/gameengine/GameLogic/SCA_2DFilterActuator.cpp
159

I mean store the filter manager as a member attribute, and in this case pass it as constructor argument.

source/gameengine/Ketsji/KX_Scene.cpp
2165

If everyone use this getter or do a reference it's OK, but if in a code we do a temporary normal variable, it will create issues.

Currently it's OK, leave it like that.

source/gameengine/Rasterizer/RAS_2DFilterManager.h
108

I don't know if the name without description is enough explicit to understand that's an OpenGL shader number and not a BGE self code number.

Something like :
Returns the filter OpenGL program shader id.

Thomas Szepe (hg1) marked 3 inline comments as done and an inline comment as not done.

Done most of Tristan's requested changes.

One thing that makes me hesitant about this patch is that we are leaking OpenGL details through our interface. In other words, the Rasterizer is supposed to be generic, but this patch adds OpenGL specific details to it. I'm sure this is not the first case of this happening, but we are trying to move toward properly encapsulating all OpenGL behind the Rasterizer. This patch could be quite useful, so this may be okay in this case.

Currently it's ok, but i will prefer to create a class for shader program reused for filter, vertex and fragment shaders. This class will manage program id, uniform binding ect....

ex :

KX_ProgramShader:
-> setUniform()
-> setUniformDef()
-> setSampler()
-> validate()
-> source
-> id

Thomas Szepe (hg1) marked an inline comment as done.Oct 25 2015, 8:53 AM

I already started three month ago to do this. Also in terms of compute and tessellation shader. But I run into some issues.
Actually this methods are implemented in BL_Shader. So we need to move this into a own class. Also some of the methods are not needed for 2D filter like setAttrib, setUniformDef(). So we need to put this methods in an own class or extend it in BL_Shader.
Then the RAS_2DFilterManager for the 2D filter is written in C style. So I need to change it to allow create instances of it and store the instances in a vector to keep the pass number working.
To do this, I need to rewrite most of the shader stuff. Also I don't have a good concept now, to get everything backward compatibility, consistent and nice looking.
I need much more time to think over this.

Anyway. I still think to get the shader program is a good thing, even we get the set functions API implemented. Because we newer know what OpenGL will be implement in the future and what the user need in addition to get a shader working.

For me actually it is only important to know if the naming of the new API "shaderProgramNumber" is good. Or should I rename it to "shaderProgram", "ProgramNumber" or just "program".