Details

Reviewers

Clément Foucault (fclem)
Campbell Barton (campbellbarton)

Maniphest Tasks

T94975: Python API for GPUShaderCreateInfo

Commits

rB9bc678969aae: pyGPU: Port 'StageInterfaceInfo' and 'ShaderCreateInfo' types

Summary

Since the gpu module is being worked on to support different rendering
interfaces on the backend, the current way of creating and compiling
GLSL shaders will be deprecated.

So an alternative is needed to keep existing addons still working in
non-OpenGL interfaces.

This patch exposes the ShaderCreateInfo strategy.

The new features can be listed as follows:

>>> gpu.types.GPUShaderCreateInfo.
                                  define(
                                  fragment_out(
                                  fragment_source(
                                  push_constant(
                                  sampler(
                                  typedef_source(
                                  uniform_buf(
                                  vertex_in(
                                  vertex_out(
                                  vertex_source(

>>> gpu.types.GPUStageInterfaceInfo.
                                    flat(
                                    name
                                    no_perspective(
                                    smooth(

>>> gpu.shader.create_from_info(

Usage example:

import gpu

shader_info = gpu.types.GPUShaderCreateInfo()

shader_info.define("USE_CLIP_PLANES")
shader_info.typedef_source(
    "struct Data {\n"
    "#ifdef USE_CLIP_PLANES\n"
    "  mat4 ModelMatrix;"
    "  vec4 WorldClipPlanes[4];\n"
    "#endif\n"
    "  int offset;\n"
    "#ifdef USE_CLIP_PLANES\n"
    "  bool use_clip_planes;\n"
    "#endif\n"
    "};\n"
)
shader_info.push_constant("MAT4", "ModelViewProjectionMatrix")
shader_info.uniform_buf(0, "Data", "g_data")
shader_info.vertex_in(0, "VEC3", "pos")
shader_info.vertex_source(
    # #define USE_CLIP_PLANES
    # uniform mat4 ModelViewProjectionMatrix;
    # uniform Data g_data;
    # in vec3 pos;
    "void main()"
    "{\n"
    "#ifdef USE_CLIP_PLANES\n"
    "  if (g_data.use_clip_planes) {"
    "    vec4 wpos = g_data.ModelMatrix * vec4(pos, 1.0);"
    "    gl_ClipDistance[0] = dot(g_data.WorldClipPlanes[0], wpos);"
    "    gl_ClipDistance[1] = dot(g_data.WorldClipPlanes[1], wpos);"
    "    gl_ClipDistance[2] = dot(g_data.WorldClipPlanes[2], wpos);"
    "    gl_ClipDistance[3] = dot(g_data.WorldClipPlanes[3], wpos);"
    "  }\n"
    "#endif\n"
    "  gl_Position = ModelViewProjectionMatrix * vec4(pos, 1.0);"
    "}"
)

shader_info.fragment_out(0, "UINT", "fragColor")
shader_info.fragment_source(
    # uniform Data g_data;
    # out uint fragColor;
    "void main() {fragColor = uint(gl_PrimitiveID + g_data.offset);}"
)

shader = gpu.shader.create_from_info(shader_info)

Diff Detail

Repository

rB Blender

Branch

arcpatch-D14497 (branched from master)

Build Status

Buildable 21458
Build 21458: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Mar 30 2022, 3:10 AM

Germano Cavalcante (mano-wii) created this revision.

Harbormaster completed remote builds in B21340: Diff 49882.Mar 30 2022, 3:10 AM

I focused on the doc an on the exposed syntax. Functionallity wise, I think we are here. Just a few touch up on the doc.

source/blender/python/gpu/gpu_py_shader_create_info.cc
185	Use similar expression for other function. "inform" is not really appropriate in this case. Also do not use third person.
289	why? this is only for internal use.
363–364	This avoid mentionning vert and frag shader since it might not stay the only two stages.
429
455
513	Wrong description.
517	Note that the type can be a simple `int` or `float`.
548
617
653	I am not sure if we should enforce the use of the vulkan limit (which is pretty low). We could emulate this by using a UBO under the hood but this might not offer the same performance. This is also a bit more work for us. The limit is not currently enforced because we are in transition.
692	I dont know if we should refer to the differences we have with the GLSL standard for MSL compat here. Maybe a link to a documentation page? Same comment for frag shader.
767	Note that this source code is included before resource declaration.
808	Note this is added before any other code/source.
814–816	Think the description is off.

This revision now requires changes to proceed.Mar 30 2022, 10:43 AM

Thomas Dinges (dingto) mentioned this in BF Blender (3.2).Mar 30 2022, 2:07 PM

Germano Cavalcante (mano-wii) marked 13 inline comments as done.Mar 30 2022, 3:40 PM

Germano Cavalcante (mano-wii) added inline comments.

source/blender/python/gpu/gpu_py_shader_create_info.cc
289	It would be a way to check what has been added. But I don't really see a good use other than debugging. This ToDo doesn't add much anyway, removed.
653	I don't think using a UBO under the hood is a good idea. It is good for the user to be aware of the limits and look for other strategies (such as UBO) to workaround the problem. Since this is still under development in the C module, we can monitor the size of `constants` pushed and return an error if the limit is reached. Is this limit specified somewhere in the code?
692	The documentation still needs to be written, but I've linked the main examples page in the meantime https://docs.blender.org/api/current/gpu.html#examples

Improve documentation

Harbormaster completed remote builds in B21359: Diff 49910.Mar 30 2022, 3:40 PM

Clément Foucault (fclem) accepted this revision.Mar 30 2022, 8:16 PM

Clément Foucault (fclem) added inline comments.

source/blender/python/gpu/gpu_py_shader_create_info.cc
653	The limit is 128 bytes and is set by the Vulkan specs. I don't know if values are tightly packed in push constant buffers or if there is padding for vec3 types and such like in std130.

This revision is now accepted and ready to land.Mar 30 2022, 8:16 PM

Would still be nice to have a quick look from @Campbell Barton (campbellbarton) before committing.

Campbell Barton (campbellbarton) requested changes to this revision.Apr 4 2022, 12:45 PM

Campbell Barton (campbellbarton) added inline comments.

source/blender/gpu/intern/gpu_shader.cc
261	`r_error[127] = '\0';` should be added so the string is always nil terminated. Although I think `BLI_strncpy` might make more sense in this case.
source/blender/python/gpu/gpu_py_shader_create_info.cc
366	GCC throws an error: /src/blender/source/blender/python/gpu/gpu_py_shader_create_info.cc: In function ‘constexpr PyTypeObject pygpu_interface_info_type()’: /src/blender/source/blender/python/gpu/gpu_py_shader_create_info.cc:370:23: error: ‘reinterpret_cast’ is not a constant expression 370 \| pytype.tp_dealloc = (destructor)pygpu_interface_info__tp_dealloc; \| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
485–500	This won't print a useful error when a bad value is used, check on `PyC_StringEnum`.
498	No exception is set, although I think this can be replaced by a utility function.
628	The `PyObject *kwds` argument should be removed as this isn't flagged with `METH_KEYWORDS`
857–860	Getting warnings here: /src/blender/source/blender/python/gpu/gpu_py_shader_create_info.cc:857:6: warning: cast between incompatible function types from ‘PyObject* ()(BPyGPUShaderCreateInfo, PyObject, PyObject)’ {aka ‘_object* ()(BPyGPUShaderCreateInfo, _object, _object)’} to ‘PyCFunction’ {aka ‘_object* ()(_object, _object)’} [-Wcast-function-type] 857 \| (PyCFunction)pygpu_shader_info_fragment_out, \| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /src/blender/source/blender/python/gpu/gpu_py_shader_create_info.cc:871:6: warning: cast between incompatible function types from ‘PyObject ()(BPyGPUShaderCreateInfo, PyObject, PyObject)’ {aka ‘_object* ()(BPyGPUShaderCreateInfo, _object, _object)’} to ‘PyCFunction’ {aka ‘_object* ()(_object, _object)’} [-Wcast-function-type] 871 \| (PyCFunction)pygpu_shader_info_sampler, \| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /src/blender/source/blender/python/gpu/gpu_py_shader_create_info.cc:875:6: warning: cast between incompatible function types from ‘PyObject ()(BPyGPUShaderCreateInfo, PyObject, PyObject)’ {aka ‘_object* ()(BPyGPUShaderCreateInfo, _object, _object)’} to ‘PyCFunction’ {aka ‘_object* ()(_object, _object)’} [-Wcast-function-type] 875 \| (PyCFunction)pygpu_shader_info_push_constant, \| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ It looks like other projects also ran into this without good solutions. The only options I could find to quiet the warnings was to add a `void ` cast before the function cast.
958	/src/blender/source/blender/python/gpu/gpu_py_shader_create_info.cc: In function ‘constexpr PyTypeObject pygpu_shader_info_type()’: /src/blender/source/blender/python/gpu/gpu_py_shader_create_info.cc:962:23: error: ‘reinterpret_cast’ is not a constant expression 962 \| pytype.tp_dealloc = (destructor)pygpu_shader_info__tp_dealloc; \| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This revision now requires changes to proceed.Apr 4 2022, 12:45 PM

Campbell Barton (campbellbarton) added inline comments.Apr 4 2022, 12:47 PM

source/blender/python/gpu/gpu_py_shader.h
9	This should have a description about what it's doing and why it's needed.

Use BLI_strncpy
Fix GCC errors (‘reinterpret_cast’ is not a constant expression)
Silence GCC warnings
Print a message indicating that the constants limit has already been reached.

About the vulkan limit I decided not to raise an error message for now. Just print a message to the console since vulkan is not yet implemented.

I'll update the template code in the description to use a UBO instead of push constants and also do some more testing.

But I believe it's ready for the next review.

Harbormaster completed remote builds in B21458: Diff 50086.Apr 4 2022, 9:01 PM

Campbell Barton (campbellbarton) accepted this revision.Apr 5 2022, 6:56 AM

This revision is now accepted and ready to land.Apr 5 2022, 6:56 AM

Campbell Barton (campbellbarton) added inline comments.Apr 5 2022, 6:58 AM

source/blender/python/gpu/gpu_py_shader_create_info.cc
758–765	These examples could be moved into external files if there is value in making them longer / commented etc (which isn't as well suited to inlining here).

Campbell Barton (campbellbarton) added inline comments.Apr 5 2022, 7:01 AM

source/blender/python/gpu/gpu_py_shader_create_info.cc
359	By convention `Py_CLEAR(py_interface->references)` is normally used in this cases, the only difference is it sets the value to NULL.

Change link indicating changes to 'GLSL Cross Compilation'
Py_XDECREF --> Py_CLEAR

I will do some more tests...

Harbormaster completed remote builds in B21588: Diff 50338.Apr 11 2022, 6:50 PM

Germano Cavalcante (mano-wii) added inline comments.Apr 11 2022, 6:50 PM

source/blender/python/gpu/gpu_py_shader_create_info.cc
758–765	It wasn't exactly the examples we should point to. I changed the link to https://wiki.blender.org/wiki/EEVEE_%26_Viewport/GPU_Module/GLSL_Cross_Compilation It's a more suitable place.