BGE: Adding geometry shader API
AbandonedPublic

Authored by Thomas Szepe (hg1) on Jun 5 2015, 3:17 PM.

Details

Summary

This patch allows the user to add a custom geometry shader to the material.

Diff Detail

Thomas Szepe (hg1) retitled this revision from to BGE: Adding geometry shader API.Jun 5 2015, 3:17 PM
Thomas Szepe (hg1) updated this object.

Instead of modifying setSampler() I have added the new setGeometryProg() API. I think this is better if we want to add later tessellation evaluation and control shader.
But this has a little downside, because you need to call first setGeometryProg() and then setSampler() to compile the shader with the geometry shader.
Otherwise if we extend setSampler(), the we need to add keyword arguments to it, if we add later other shader types to it.
Actually I don't know which one we should use. Please make comment on this.

Test file:

source/gameengine/Ketsji/BL_Shader.cpp
166

You use 0 instead of NULL for pointers ?

192

Same here.

292

Same think here. i'm not sure that other devs accepts to mix cleanup and features, even if it's a minor cleanup.

344

Space : (const char **).

357

0 instead NULL ?

435

NULL instead 0.

461

Why this test ? you can return directly geomProg, no ?

914

It's a silent user error ?

920

it's not better to do :

if (!ParseArg…)
    return NULL

geomPorg = g;
Py_RETURN_NONE;
949

Space between "( )".

Thomas Szepe (hg1) marked 7 inline comments as done.

@tristan (tristan). I only copied the fragment shader code and modified it for the geometry shader. That is the reason why there are some 0 instead of NULL.
I though it is better to do style and code cleanup (goto programError, and code deduplication) this later with an extra patch.
Anyway I have done most of your requests.

source/gameengine/Ketsji/BL_Shader.cpp
292

As Sybren told me (he has asked Campbell) we can mix cleanup and feature patches for lines and methods that are affected by the patch.

435

tmpGeom is a unsigned int not a pointer.

914

Yes there is no message, as on the other shades. Blender blocks changing the shader code here.
The documentation says you need to delete first the old shader before you can assign a new one.

I don't know why Blender blocks shader recompiling. My graphic card can recompile running shader, but maybe some other graphic card have problems with it.

I am assuming in your earlier post you meant to say setSource instead of setSampler. I would like to see the setting of sources to be consistent. So either being able to set them all with setSource with keyword arguments, or adding a setVertexProg, setFragmentProg, and a function to call once all programs are set to actually handle the compiling. If possible it would be nice to replace one shader without replacing all of them. For example, being able to change the vertex shader and continue using the fragment shader that Blender generated. I haven't looked at that part of the code in a while to know how feasible that is.

source/gameengine/Ketsji/BL_Shader.cpp
192

I would like to see this be consistent. Either using 0 for geomProg, or change the others to NULL as well.

292

I agree, this is fine. Whitespace/style cleanups in modified code are fine. The kinds of cleanups that should not be mixed with features are things that actually affect logic, since this can make things more confusing for code review.

340

This looks like it probably shares a lot of code with fragment and vertex shaders, any chance of reducing duplicate code here?

source/gameengine/Ketsji/BL_Shader.h
123

Probably best to be consistent with existing code on the placement of the *

176

Probably best to be consistent with existing code on the placement of the *

source/gameengine/Ketsji/BL_Shader.cpp
192

@Daniel Stokes (kupoman) we can write a cleanup patch for the other value.

source/gameengine/Ketsji/BL_Shader.h
123

Check the mail discussion starting at http://lists.blender.org/pipermail/bf-gamedev/2015-March/000489.html; the conclusion was that we prefer to be consistent with Blender's code style, even if it means being inconsistent with existing BGE code.

Porteries Tristan (panzergame) accepted this revision.EditedJun 21 2015, 11:29 AM

Nice work, looks good to me.

This revision is now accepted and ready to land.Jun 21 2015, 11:29 AM

Looks good to me but it would be good a style cleanup patch (commited after this) to make the BL_Shader.cpp/.h consistent.

Daniel Stokes (kupoman) requested changes to this revision.Jun 22 2015, 5:57 AM

I still want the shader APIs to be more consistent. Also, I don't see any documentation changes. With those documentation changes I would like to see a note that this is only partial geometry support since it does not support adjacency information.

This revision now requires changes to proceed.Jun 22 2015, 5:57 AM

Changes:

  1. Removed setGeometryProg() and extend setSource() instead.
  2. Add documentation for geometry shader.
  3. Document missing apply argument.

As Jorge Bernal (lordloki) suggested, I will provide a style cleanup patch after this patch is committed.

I dislike this compatibility problem, @Campbell Barton (campbellbarton), @Mitchell Stokes (moguri) for you what the best for function setSource, add the new argument at the end or reorganize arguments order to setSource(vert, frag, geom, apply) ?

I dislike this compatibility problem, @Campbell Barton (campbellbarton), @Mitchell Stokes (moguri) for you what the best for function setSource, add the new argument at the end or reorganize arguments order to setSource(vert, frag, geom, apply) ?

Reorganizing the arguments will make compatibility problems. Because the actually setSource will take the parameters in this order. setSource(vertexProgram, fragmentProgram, apply). If I move geometryProgram between "fragmentProgram" and "apply" it will break compatibility.

So setSource(vertexProgram, fragmentProgram, apply, geometryProgram) is the only way without braking compatibility for the old setSource method.

Reorganizing the arguments will make compatibility problems. Because the actually setSource will take the parameters in this order. setSource(vertexProgram, fragmentProgram, apply). If I move geometryProgram between "fragmentProgram" and "apply" it will break compatibility.

i know, but setSource(v, f, g, apply) seems more logical, we already have function which had these compatibility problems before ?

i know, but setSource(v, f, g, apply) seems more logical, we already have function which had these compatibility problems before ?

Sure setSource(v, f, g, apply) is more logical, but will break all actual shaders. And I don't want to do that.

So we have only three ways to don't break the old API.

  1. Doing it like I have done before with setGeometryProg().
  2. Using the actual setSource(v, f, apply, g).
  3. adding a new API setShader(v, f, g) (apply is useless).

hmmm, well, setSource(v, f, a, g) is the easier currently.

My suggestion is to create a new API and deprecate the old one. This new API should be flexible to changes so this does not happen again if we want to add support for tessellation shaders.

What about use:

shaders = {
    "vertex" : vertex_shader_text,
    "fragment" : fragment_shader_text,
    "geometry" : geometry_shader_text
}
setSource(shaders, true)

By this way you limit compatibility issues. For the moment you have to keep the old API too. I think you can override the setSource function by only checking if the first argument is a dictionnary, if not use the old API. The other way is to use an other function name, maybe:

  • setSources ;
  • setSourceDict ;
  • setSourceList.

Maybe replace string by constants is better, e.g BL_Shader::[VERTEX/FRAGMENT/GEOMETRY]_SHADER_TEXT

This has been done in the UPBGE branch. Closing