Page MenuHome

Shader PyNodes for Blender 2.8
Needs ReviewPublic

Authored by Alexander Romanov (a.romanov) on Mar 23 2017, 5:12 PM.

Details

Summary

One of the main goals of new Viewport is to allow close integration of external engines in Blender.
I see two separate issues:

  1. allow to define render logic (calculating and passing required data to GLSL shader)
  2. allow to define shader Node System.

This patch contains the implementation of python binding for shader nodes. So it is designed to solve the second issue.

The main idea of this patch is to expose existing gpu C-API to Python. It will allow to write GLSL nodes by addon creators.

Note that expanded gpu API is not enough for full implementation of external engine capabilities (e.g. , SSS, transparency, glass shader, reflection). More information you can find here: https://wiki.blender.org/index.php/Dev:2.8/Source/Viewport/ExternalEngines#Python_API

The current state of the patch: https://wiki.blender.org/index.php/Shader_Pynodes_for_Blender_2.8

The patch includes temporary render engine called "PyGLSL". API is not complete and not confirmed yet, so implementation of "PyGLSL" is a good point for review start.

Sample file:


Green nodes are already rewritten with python.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D2577_2
Build Status
Buildable 639
Build 639: arc lint + arc unit

Event Timeline

Dalai Felinto (dfelinto) edited edge metadata.EditedApr 19 2017, 4:10 PM

@Alexander Romanov (a.romanov) could you please:

  1. Update the patch to latest 2.8
  2. Use arcanist from now on (otherwise it very easily conflicts when trying to apply the patch) - poke me if you need help setting this up.
  • Update for the current blender2.8 branch
  • Started replacing of Cycles nodes
  • Use GPUMaterial type for CTX_gpu_material_* and related functions
  • minor code style and naming

First pass Python C/API review, will post higher level review next.

intern/cycles/blender/blender_shader.cpp
744–747

This should be get_enum, suggest enabling WITH_ASSERT_ABORT from CMake so these errors don't go un-noticed.

intern/render_pyglsl/CMakeLists.txt
2

Putting addons in intern/ is a bit strange, for purpose of review it makes some sense to have in same repo. Probably should move to addons repo before merge though.

source/blender/blenkernel/intern/node.c
299

Worth noting this struct owns memory for all members.

299–308

struct naming and * alignment doesn't follow our code style, also other parts of this patch - wont mention every place this happens but please try follow https://wiki.blender.org/index.php/Dev:Doc/Code_Style more closely.

source/blender/python/intern/gpu.c
318

Multiple places in this patch use:

PyUnicode_AsUTF8String, then PyBytes_AsString.

This isn't needed (causes intermediate variable to be created which needs to be dec-ref'd after).

Instead just call _PyUnicode_AsString.

326–330

This should at least give some unique information. eg:

return PyUnicode_FromFormat("<GPUNodeLink at %p>", self);
362–369

Again, no need for intermediate PyUnicode_AsUTF8String conversion.

433–434

Again, no need for intermediate PyUnicode_AsUTF8String conversion.

442

A common pattern in CPy is to do:

if (!(((id = _PyLong_AsInt(item)) == -1) && PyErr_Occurred())) {
    /* id is ok! */
}
else {
    PyErr_Clear();
    ... continue parsing ...
}

This supports any types that support the number interface.

449

There won't be any errors here.

450

mathutils_array_parse is preferred over exact type checks.

Allows tuples/lists too.

457

Again, should use mathutils_array_parse, see matrix construction in mathutils_Matrix.c

477

Again, overly spesific type checks, see _PyLong_AsInt suggestion above.

479

If this fails it wont be clear for the caller why.

487

Again very spesific parsing, (will fail on 0/1), suggest using PyC_ParseBool.

558

No need, its created with a single user.

575

Must return NULL to properly raise an error.

603

Again, must return NULL... also for all other false returns in this function.

664

Again, must return NULL.

696

Again, must return NULL.

714

This function should probably return None, not a boolean.

General Questions

  • Who would use this functionality?

    Artists? ... or is this intended only for engine-authors so they can match Blender's viewport to their engine?

    Of course there would be some technical artists who like to write GLSL, I just try get a sense of it's intended use.

    Would be good to update the patch description with some big-picture info.
  • I assume the temporary render engine is just for review, so it would be removed (or moved to docs/example template) before this feature makes it into master.
  • Would this functionality be useful for regular Blender development?

    My impression is that this is not the case - that you would only define Python GLSL shaders when not developing in C.
  • What kinds of GLSL sharers does this expose? both vertex & fragment?

    Wiki page hints that both are supported (since output and geometry sharers are mentioned) but from looking over API It's not clear to me.

Blender2.8/Complications

While this patch isn't making changes that impact 2.8x viewport work directly, I do worry that this patch adds functionality we will have a hard time to properly support, at the same time as supporting the new viewport.

We could do review overall design, get details sorted out - but postpone merging until viewport development has settled down.

Many changes in this patch only relate to changing how node-trees store engine compatibility & node.is_a(..) taking a string arg, so we could merge this now and postpone other changes.

Python API Comments

This patch adds C-style API's directly into the gpu module, eg: gpu.GPU_link_output, gpu.add_engine_compatibility.

Would prefer all functionality go into a sub-module (and it's own file), eg gpu.shader_node, as we have already for gpu.offscreen.

Note that the Python functions exported by BPY_extern.h are a bit awkward and from what I can tell a workaround for RNA not supporting arrays/sets of strings. Would be nice if that could be avoided - but I couldn't find a good way - besides adding array-of-string support to RNA.

intern/cycles/blender/blender_shader.cpp
249

While it makes sense to support type checks on string identifiers for dynamic types, I'm not convinced its a good idea to convert existing code to use this where the types are available. (typo's will go un-noticed).

source/blender/gpu/intern/gpu_material.c
1692–1708

Suggest using BLI_dynstr here, will make code read more straightforward.

General Questions
====

  • Who would use this functionality?

    Artists? ... or is this intended only for engine-authors so they can match Blender's viewport to their engine?

    Of course there would be some technical artists who like to write GLSL, I just try get a sense of it's intended use.

I think it's for engine-authors. In my opinion the main goal is to make it possible to create/modify Viewport by addon creators. In future if it is required, we can create specific Shader Node, which will allow to write GLSL in Blender.

//Would be good to update the patch description with some big-picture info.//

Ok

  • I assume the temporary render engine is just for review, so it would be removed (or moved to docs/example template) before this feature makes it into master.

Yes, it will be moved/removed

  • Would this functionality be useful for regular Blender development?

The design document suggested by @Clément Foucault (fclem) (https://wiki.blender.org/index.php/Dev:2.8/Source/Viewport/PyNodeGLSL) requires Cycles shader nodes rewritting. I think that Python could be more comfortable for regular development.

//My impression is that this is not the case - that you would only define Python GLSL shaders when not developing in C.//

Yes, but if I understand correctly bl_gpu_features = {'PROBES', 'SHADOWS', 'LIGHTS'...} will help to configure some features, which you will not be able to handle using Python.

Blender2.8/Complications
====

While this patch isn't making changes that impact 2.8x viewport work directly, I do worry that this patch adds functionality we will have a hard time to properly support, at the same time as supporting the new viewport.

We could do review overall design, get details sorted out - but postpone merging until viewport development has settled down.

Many changes in this patch only relate to changing how node-trees store engine compatibility & node.is_a(..) taking a string arg, so we could merge this now and postpone other changes.

Yes of course, I can extract this changes into other diff. Should it be applied to master or blender2.8 ?

Python API Comments
====

This patch adds C-style API's directly into the gpu module, eg: gpu.GPU_link_output, gpu.add_engine_compatibility.

Would prefer all functionality go into a sub-module (and it's own file), eg gpu.shader_node, as we have already for gpu.offscreen.

Ok

Note that the Python functions exported by BPY_extern.h are a bit awkward and from what I can tell a workaround for RNA not supporting arrays/sets of strings. Would be nice if that could be avoided - but I couldn't find a good way - besides adding array-of-string support to RNA.

Yes, also thought about this. I think it can be postponed to the polishing stage.

When @Clément Foucault (fclem) and I thought about the need of PyGLSL, there were two related things in mind:

A) To support 100% Python defined external engines.
B) To support Cycles fallback nodes for Eevee.

After reading the feedback from other developers I agree that we should reconsider the approach here. I put some notes on that in the external engines page. It boils down to: we need a more robust design for both cases :/

Alexander Romanov (a.romanov) marked 23 inline comments as done.
  • fix memleaks
  • rename shader_node_compat_desc structure
  • use dynstr
  • using _PyUnicode_AsString
  • use ((BaseMathObject *)obj)->data to for passing pointer to uniform
  • use PyBool_Check
  • correct Py_INCREF in pygpu_get_shader_node_classes
  • moving shader API to submodule
intern/cycles/blender/blender_shader.cpp
249

What is alternative for Node identification for Cycles if all nodes will be rewritten in Python?

intern/render_pyglsl/CMakeLists.txt
2

Will be moved/removed before pushing

Some minor suggestions for string editing.

source/blender/gpu/intern/gpu_codegen.c
294

again, redundant strlen check. can check *type->gpu_lib_text != '\0'

2036–2042

Suggest to use BLI_dynstr here too.

source/blender/gpu/intern/gpu_material.c
1695

don't think this is needed, are empty strings common?

Or replace with if (*s != '\0') {...}

source/blender/gpu/intern/gpu_codegen.c
2036–2042

On second thoughts BLI_sprintfN would be most straightforward.

Here are some concerns/open questions from my side here:

  • The API seems quite low level: simply exposed bf_gpu to python. It should be possible to make it more streamlined and decoupled from internal specifics.
  • The GLSL source management is quite weak. Seems you are requiring to have one single source which contains source of all nodes. Can see following downsides:
    • Source becomes big and unmanageable. It is already quite a problem for the existing material nodes. Bigger code also means slower compilation , even when functions are not used (maybe hard-to-measure but still).
    • Some studios are using GLSL custom shaders, just similar to OSL. If we've got single source for everything then we are automatically going away from similar workflow.
  • Attributes management is not implemented at all. How would GPU module know which attributes and under which locations to bind them? How does one know that some extra tangent layer is needed?
  • How do we support features like "copy texture space form other object" ?
  • Passes are also completely lacking here. How would that integrate with passes done for Eevee rendering? How one implements SSS, mirror, glass shaders with PyGLSL?

Also see points raised by @Campbell Barton (campbellbarton). I agree with those and will not copy-paste them.

This is just some stuff from the top of my head, when you start thinking deeper about such project more questions will arise. To me it seems we should suspend code work and think of good feature-proof and future-proof design first and don't expose low level stuff since that will make us unable to move forward with design changes.

Alexander Romanov (a.romanov) marked 4 inline comments as done.
  • merge from blender2.8
  • updated for eevee
  • fix campbell suggestions

Here are some concerns/open questions from my side here:

  • The API seems quite low level: simply exposed bf_gpu to python. It should be possible to make it more streamlined and decoupled from internal specifics.

I agree with you here. But afaik, booth C and Python implementations should be available. Both implementations should be able to live together. Most important thing here is that python GPULink function allocates C-structure which is unmanaged with Python and freeing is performed in C-core. And another thing is uniform passing. Now shader recompiles every time when some property in node has changed (this is legacy drawback).

  • The GLSL source management is quite weak. Seems you are requiring to have one single source which contains source of all nodes. Can see following downsides:

Actually no. There is common library, which is hardcoded in Blender core. And also you can implement your own libraries.

# library per engine with this member method
def gpu_library(self):
        return '''
            '''

# library per node
bl_gpu_code = {"CYCLES": lib, 'PyGLSL': lib}

# library and gpufunc if you want to add compatibility for some unsupported nodes
add_engine_compatibility("BLENDER_RENDER", "ShaderNodeOutputMetallic", (gpufunc, lib))
  • Source becomes big and unmanageable. It is already quite a problem for the existing material nodes. Bigger code also means slower compilation , even when functions are not used (maybe hard-to-measure but still).

In this case we can e.g. add option "Use common library"

  • Some studios are using GLSL custom shaders, just similar to OSL. If we've got single source for everything then we are automatically going away from similar workflow.

Unswered

  • Attributes management is not implemented at all. How would GPU module know which attributes and under which locations to bind them? How does one know that some extra tangent layer is needed?

For now I've expanded builtins identifiers. It also implemented in GPULink function

  • How do we support features like "copy texture space form other object" ?
  • Passes are also completely lacking here. How would that integrate with passes done for Eevee rendering? How one implements SSS, mirror, glass shaders with PyGLSL?

IMHO this is separate issue and this patch is intended to cover only nodes.

I see 2 ways to implement external real-time render engine.

  1. Use custom nodes, which will not provide GLSL code, and generate shader by node tree processing. Disadvantage is that it will be single entity and it will not allow us to plug own glsl.
  2. Separate render logic and some GLSL logic which corresponds to each node. In this case we can plug/replace parts of GLSL code. So this patch is about nodes and its own GLSL code.

Render logic must be expanded to Python separately. Also, I think that this could be useful to allow reusing of render logic part by external engine. For example for now Blend4Web reuses "Blender Render" logic.

So this patch is intended to add API for plug and replace of GLSL-code corresponded to node.

Also see points raised by @Campbell Barton (campbellbarton). I agree with those and will not copy-paste them.

This is just some stuff from the top of my head, when you start thinking deeper about such project more questions will arise. To me it seems we should suspend code work and think of good feature-proof and future-proof design first and don't expose low level stuff since that will make us unable to move forward with design changes.

I will made some minor updates and will try to suggest something more "streamlined and decoupled from internal specifics"

  • Some studios are using GLSL custom shaders, just similar to OSL. If we've got single source for everything then we are automatically going away from similar workflow.

Unswered

  1. Use custom nodes, which will not provide GLSL code, and generate shader by node tree processing. Disadvantage is that it will be single entity and it will not allow us to plug own glsl.
  2. Separate render logic and some GLSL logic which corresponds to each node. In this case we can plug/replace parts of GLSL code. So this patch is about nodes and its own GLSL code.

Am i allowed to comment here? (If I don't I appologize)
Imho the possibility to use custom GLSL shaders is a very needed feature that would add soooo much flexibility to Blender. I know that PBR is on everyone's lips (and even there using custom shaders would have many benefits imho) but for productions that use some sort of NPR it it would be a big big help. OSL is nice but has, compared to Glsl (to my understanding*) some heavy downsides (its restriction to Cycles und its lesser flexibility for non PBR (e.g. no lightloops, the closures etc.), only cpu).

  • (just startet to learn the ropes so feel free to correct me if i am wrong).