This patch allows the user to add a custom geometry shader to the material.
Details
Diff Detail
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 "( )". |
@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. 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. |
Looks good to me but it would be good a style cleanup patch (commited after this) to make the BL_Shader.cpp/.h consistent.
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.
Changes:
- Removed setGeometryProg() and extend setSource() instead.
- Add documentation for geometry shader.
- 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) ?
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 ?
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.
- Doing it like I have done before with setGeometryProg().
- Using the actual setSource(v, f, apply, g).
- adding a new API setShader(v, f, g) (apply is useless).
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