@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.

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

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.

In D2577#62121, @Campbell Barton (campbellbarton) wrote:

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 :/

Some minor suggestions for string editing.

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.

In D2577#62843, @Sergey Sharybin (sergey) wrote:

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).