Page MenuHome

Python API: GPUBatch for GPUShader
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Oct 9 2018, 2:22 PM.

Details

Summary
import gpu
import bpy
from gpu_extras.batch import batch_for_shader

coords = [(0, 0), (100, 100), (300, 100), (200, 400)]

shader = gpu.shader.from_builtin('2D_UNIFORM_COLOR')
batch = batch_for_shader(shader, "LINE_STRIP", {"pos" : coords})

def draw():
    shader.bind()
    shader.uniform_float("color", (1, 1, 0, 1))
    batch.draw(shader)
    
bpy.types.SpaceView3D.draw_handler_add(draw, (), "WINDOW", "POST_PIXEL")

Diff Detail

Repository
rB Blender
Branch
batch_from_shader (branched from blender2.8)
Build Status
Buildable 2294
Build 2294: arc lint + arc unit

Event Timeline

  • fix
  • new gpu_extras module
  • naming and license
  • pep8
Jacques Lucke (JacquesLucke) retitled this revision from Python API: shader.new_batch function [WIP] to Python API: GPUBatch for GPUShader.Oct 11 2018, 11:23 AM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
  • use POINTS type for index buffer

Not sure what the right choice for that is.
The old setting did not work well.

Germano Cavalcante (mano-wii) requested changes to this revision.Oct 13 2018, 11:44 PM
Germano Cavalcante (mano-wii) added inline comments.
release/scripts/modules/gpu_extras/batch.py
20

Is the module bpy really necessary here?

21

In this case it might be better to put the import gpu inside the function.

28

I found strange this name batch_for_shader. Is it really descriptive? How about batch_create_for_shader?
I saw that you did a submodule called batch. Is not the use of the "batch_" name in the functions redundant?
(Just an observation, it's not a necessary change).

This revision now requires changes to proceed.Oct 13 2018, 11:44 PM
release/scripts/modules/gpu_extras/batch.py
20

agree, will remove it

21

not sure if I agree on that one. Importing it here does not really increase load times. And we probably won't only have one function that uses this module later on. Doesn't make sense to import it in every function imo.

28

noticed that too. While using the function I also noticed that you'll import it like from gpu_extras.batch import ... because otherwise the line that uses this function becomes so long. For that reason it makes sense to keep "batch" in the name.

Maybe we don't need the batch.py module though. I mean, it deals with all kinds of different types, so maybe it could just be importable from gpu_extras directly.

For the name, personally I don't like changing the order of words like that. create_batch_for_shader (or some synonym for "create") sounds more natural and is easier to remember imo. Otherwise I always have to think how the word order was again...
Having said that, I know that we are using the same naming scheme for other functions as well (e.g. format_calc sounds less natural than calc_format; in that case the word "calc" should maybe be replaced with "get" or so anyway, because we don't really calculate anything)

Regarding the naming, why do we have a gpu_extras rather than making this a native function of the gpu module? It clearly simplifies the code a lot and should be the preferred way for users to do drawing, so it's not an "extra" thing.

For bpy we already define the real module in Python, and import _bpy into that. Some types gets extra functions in bpy_types.py. Is there any reason we shouldn't do the same thing for gpu?

Yes, it would be nicer if this function would be part of the gpu module directly. However, I was told that it would be better to not mix C with Python for the gpu module.

There are a few possible ways to do it.

  • The method you describe. I'm just not sure how to extend individual types with Python then. We could subclass the gpu types, though by doing that we might need to change the type checks in the gpu module. We could also give the gpu types a __dict__ attribute, so that they can be extended dynamically. This would also also allow users of the api to change the types dynamically.
  • Something similiar to what I did at first in this Patch. Define the functions in Python in some _gpu_utils module. Then import this module into C and extend the gpu types from within C.

Personally I think it would not be very bad to have the C code for the python api depend on some python code (as long as other parts of Blender do not depend on it). We have a dependency in the same direction for the bpy module as well.

Regarding the naming, why do we have a gpu_extras rather than making this a native function of the gpu module? It clearly simplifies the code a lot and should be the preferred way for users to do drawing, so it's not an "extra" thing.

Currently it may seem unnecessary, but a gpu_extras module comes in handy. In the future other functions can be added (eg something like batch_create_line_strip). These functions could also be a reference for python developers.

release/scripts/modules/gpu_extras/batch.py
28

format_get would also not be a good name because, at least in C, several formats are compatible. It depends on the fetch.

Currently it may seem unnecessary, but a gpu_extras module comes in handy. In the future other functions can be added (eg something like batch_create_line_strip). These functions could also be a reference for python developers.

But why not put that hypothetical batch_create_line_strip in gpu? To me it seems the API is being split up into gpu and gpu_extras because of an implementation detail, not because it's a better API design for users.

General note, the gpu module is quite low level and some simple operations aren't available as a single step.

I would like to keep it low level (mostly wrapping C code as-is) and add higher level utilities into a module as has been suggested (gpu_extras).

A hint as to where functionality belongs...

  • Does it combine other functions for convenience?
  • Does it make opinionated decisions?
  • Does it add code not available in Blender's GPU module?

If this is the case, I'd prefer to put these in a utility module. We can be a bit more relaxed about this module during 2.8 beta.
Then see how the API looks and make any needed changes before release (those changes could involve moving functionality between modules too).

I would like to keep it low level (mostly wrapping C code as-is) and add higher level utilities into a module as has been suggested (gpu_extras).

I did not know that this was the goal of the gpu module.
Still I consider it fairly bad API design to just put everything useful into individual functions in a gpu_extras module.
Brecht made a good point with this:

To me it seems the API is being split up into gpu and gpu_extras because of an implementation detail, not because it's a better API design for users.

If we want the gpu module to be a thin wrapper only (and yes, that might be a good thing to have), it might make sense to design a more user friendly pythonic API on top of the gpu module. Something like a "bdraw" module or whatever. This module could be written in pure python. It could abstract away the gpu, bgl and blf module. I'm not sure how this api could look like but I'm pretty sure that could come up with something if we agree to have such a module. Also it is not necessary to have this module ready when we first release Blender 2.8.

Before the release just the gpu, bgl and blf modules should be considered stable, so that addon developers can at least use those.

There could indeed be a higher level API. But I think that would be e.g. draw lines or images with a single line of code, without being aware of batches and shaders.

@Campbell Barton (campbellbarton), can you explain why you want this division between gpu and gpu_extras? To me it seems clear that this makes the API harder to use, but what is the benefit that justifies this?

batch_for_shader is what we want the majority of users of the gpu module to use, it should be the main function to create batches unless we come up with something higher level.

@Brecht Van Lommel (brecht), my impression of this patch made some decisions we would want to avoid for a generic function.

  • Why does it use the maximum length of all inputs, unless I miss some use case, I'd rather it ensures all lists are the same size since a mismatch is most likely a mistake.
  • Why does it assume we wan't to use "POINTS" for indices? instead of "LINES", "TRIS"... etc.

Having said that, these could be fairly easily addressed, so if this really is one of a small number of utility functions which prevent copy-pasting exact same code about, +1 for making it part of gpu.

I'd expect that the current implementation already checks if all lists have the same length (I thought the fill_attribute function does that).

I currently use POINTS as type for the index buffer because otherwise I could not pass a flat list of indices into the buffer. This is a limitation of the constructor of the GPUIndexBuf though and should be fixed in any case.

I'll rewrite this patch to make the function part of the gpu module again. I still think this function should still be implemented in Python but to avoid long discussions about how to extend the gpu module with Python I'll implement it in C.

How should the function be called? The two main options are: shader.new_batch(...), gpu.types.GPUBatch.ForShader(...).

For the sake of simplicity I'd prefer something like the first option.

  • ugly shader.new_batch(...) implementation in C

I already regret my decision to implement this function in C. It is just a mess.
In the patch I use the other API functions directly so that I can benefit from their error handling.
So basically the c function is very close to a direct translation of the python function.

I don't really want to be responsible this code ^^ I'd prefer to implement it in Python again and load it into the gpu module at runtime. Maybe similiar to what I did in the first version of this patch.

Regardless of how we'll implement this function, I'll write examples using the shader.new_batch function now.

The batch_for_shader function is still there, just for reference. It will be removed later on.

  • ugly shader.new_batch(...) implementation in C

It really did get weird.
Now I understand why this function is best done in Python.
I would prefer to use the .tp_new callback than exposing the bpygpu_Batch_new, bpygpu_IndexBuf_new and bpygpu_VertBuf_new functions.
Also bpygpu_VertBuf_attr_fill is best to stay internal.
The ideal would be to make a batch with the GPU functions and then use the BPyGPUBatch_CreatePyObject.
But it would not be easy to use content and index PyObjects without exposing more functions that should be internal.

I agree it should be done entirely in Python. (either by changing the C module name to _gpu or by making a gpu_extras module).

I agree it should be done entirely in Python. (either by changing the C module name to _gpu or by making a gpu_extras module).

I think we agreed that this function should be part of the gpu module.
Renaming the C module to _gpu and extending it through Python is one possibility. If we choose this route I think someone else than me should do it because I'm not sure how this is supposed to look like in the end.

We could also just go back to my initial patch. The design might not be perfect but it is a very localized "hack" and easy to replace later on. We just have to agree on the API and I think shader.new_batch(type, content, indices=None) works well.
We have to agree in that before the beta, the implementation details can change later on.

I wrote some simple examples that use the API here: https://wiki.blender.org/wiki/Reference/Release_Notes/2.80/Python_API/Draw_API

  • Update to latest blender2.8

@Campbell Barton (campbellbarton) has already taken the initiative in rBf606ee7637eb to add an gpu_extras module.
So I don't see much need to add a module in C _gpu and another in pure python gpu.

This revision is now accepted and ready to land.Oct 25 2018, 6:14 PM

Committed with minor corrections. rB0264c050bf277494a187b32d51be13b0760f81c0

release/scripts/modules/gpu_extras/batch.py
55

Converted into an exception, since assert can be disabled.