OpenGL 2 compatibility changes

campagnola commented 11 years ago

Modifications fall into a few categories: 1) OpenGL 2 compatibility changes. 2) convenience; these just reduce the amount of boilerplate code needed. 3) Recommended API changes, some of which may break some existing code. The changes reflect the way it seems most intuitive for me to write my code, so please do not hesitate to reject any of them (especially 2 and 3); some of them are a bit contrary to the style of the library so far.

Several of the enums listed in shader.py do not exist in earlier versions of OpenGL. To work around this, I added generate_enum_map, which checks for these at run time.
I made a few changes to automate some of the process of attaching data to shader uniforms and attributes, such that the final code looks cleaner. It also obscures some of the inner workings, which I can understand may not be desirable. In particular, I added getitem and setitem methods to ShaderProgram, as well as Attribute.set_buffer which is needed for OpenGL 2.
Fixed up Buffer.set_data, which appeared to be broken. Also some minor changes that make Buffer easier to use when you just have a single, contiguous array to upload.
In my Buffer.set_data, I removed the assertion and just bind / unbind the buffer to set the data. I noticed you generally avoid this, preferring to let the user take care of binding. Is there a reason for this? I think binding to global variables is one of the most awkward aspects of OpenGL and would prefer to automate it as much as possible.

Below is an example of how I'm using these changes (sans shaders). The major differences are that each VBO is built in a single line, and uniforms / attributes are accessed directly from the shader and automatically configured (Buffer carries a little more information about its data--ndim, shape, and dtype--to make this possible).

class PlotLine:
    def __init__(self, pos, color, width, connect):
        self.pos = pos
        self.color = color
        self.width = width
        self.connect = connect

    def init_gl(self):
        ## shaders define transform uniforms
        ## and in_ attributes
        self.program = pygly.shader.ShaderProgram(
            pygly.shader.VertexShader(vertex_shader),
            pygly.shader.FragmentShader(fragment_shader),
            )

        self.update_buffers()

    def update_buffers(self):
        self.width_max = self.width.max()
        self.vbo = pygly.buffer.Buffer(data=self.pos)
        self.cbo = pygly.buffer.Buffer(data=self.color)
        self.lbo = pygly.buffer.Buffer(data=self.width)
        self.conbo = pygly.buffer.Buffer(data=self.connect)

    def draw(self, transform, view_transform):
        self.program.bind()

        self.program['transform'] = transform
        ## maps final clipping rect to viewport coordinates (needed for antialiasing)
        self.program['view_transform'] = view_transform

        self.program['in_position'] = self.vbo
        self.program['in_color'] = self.cbo
        self.program['in_width'] = self.lbo
        self.program['in_connect'] = self.conbo

        glEnable(GL_BLEND)
        glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA)
        glLineWidth(self.width_max+1)
        glDrawArrays(GL_LINE_STRIP, 0, self.pos.shape[0])
        glBindBuffer(GL_ARRAY_BUFFER, 0)
        glUseProgram(0)

adamlwgriffiths commented 11 years ago

Hi Luke, Can you cancel this pull request and instead direct it at the branch 'pull_request_23'. https://github.com/adamlwgriffiths/PyGLy/tree/pull_request_23

Cheers, Adam

adamlwgriffiths commented 11 years ago

Actually, never mind, I can do this.

adamlwgriffiths commented 11 years ago

I tried following this (http://www.davehking.com/2012/11/24/reverting-a-github-pull-request.html) with regards to merging and making changes, and it just exploded the git repo.

Git and me aren't friends.

So I've had to forcibly reset HEAD to prior to the commit. This is a bad thing, so your repo may be very confused.

I'll manually merge the changes.

Did I mention I don't like git?

adamlwgriffiths commented 11 years ago

Hi Luke,

I've re-worked the buffer and shader code based on your advice. I took your modifications and made some updates. I've credited you in the commit logs though =).

I've renamed Buffer to VertexBuffer to avoid possible confusion in the future with other buffer types (FBOs, PBOs, etc).

VertexBuffer is now a basic buffer wrapper. DtypeVertexBuffer uses a single dtype to define its data format.

Multiple dtypes within a single buffer have been dropped.

The shader code has been updated with your enum generation code. I also added the shader['xyz'] support for finding both uniforms and attributes that you provided. As per your recommendation, getattr and setattr have been removed.

I didn't merge some of the changes.

Re: buffer.ndims and buffer.shape. This didn't get merged because the data contained in the buffer is unstructured, you can use set_data to set any part of the buffer you want, so it's not really appropriate to store these variables, although it would be nice =P.

Re: shader.set_buffer. I can see what you're doing here, but the shader isn't the appropriate place for it. Without buffer.ndims and buffer.shape, I'm not sure how to apply it. It could perhaps be worked into DtypeVertexBuffer. The problem is, if you did this, 2 different calls to set_data could use 2 different shapes for the data. Especially if the user is just setting 1 or 2 values that aren't an entire vertices worth of data.

From what I can see, there are too many assumptions in providing this functionality. If you can think of a better way using the current code, I'm all ears =).

The easiest replacement for this is to use the DtypeVertexBuffer with a complex dtype like in the example code. An even easier way is to create an Nd array with a basic dtype (eg, 'float32') and then take a .view of it using a complex dtype. It avoids the messy tuple style value construction required with complex dtypes.

Something to be aware of, the shader attributes and uniforms no longer use getattr and setattr, so attributes.xyz will no longer work. Be aware however, that calling something like: shader.attributes.my_attribute = 0 Will succeed as python will set a new member variable. I got caught by this one today after updating the code =P.

If you find ways to add the changes that weren't merged into the new code base, by all means let me know.

I hope this doesn't cause too much of a problem in updating any code you have.

Cheers, Adam

adamlwgriffiths commented 11 years ago

When I say "the data in the buffer is unstructured", I'm referring to VertexBuffer.

DtypeVertexBuffer is obviously structured, so it may be possible to derrive these values from the dtype.

The only problem is, when the dtype is simple (ie, 'float32'), the element count is 1 even if the data in the array is shape (-1, 3). You can work around this by using a complex dtype of ('float32', (3,)). It just means that you need to use the complex dtypes rather than relying on array shape.

Perhaps there is a better way to handling the DtypeVertexBuffer than the dtype itself.

campagnola commented 11 years ago

Have a look at the way ShaderProgram/.set_buffer is done in my latest commit. That one is very different from the one you merged here and more true to the way the old Buffer class was structured.. I'm sorry, I should have updated the pull request to point to those changes instead (I only mentioned it in email) :/

I'll try merging in your changes and see what can be done..

adamlwgriffiths commented 11 years ago

I have to admit I haven't come to terms with Git. I had trouble managing the merge because it was from your fork which happened just before I made some big changes, so it was hard to keep track of things. I tried for a half a day to get git to behave but I couldn't get it to do what I wanted. Hence manual merge. So I apologize for this.

The 'set_buffer' function in the shader has a few issues. It relies on 'ndim' and 'shape' in Buffer. It is also calling vertex buffer functions in the shader. It should really be the other way around, passing the shader to the vertex buffer. It also doesn't unbind the buffer, that is trivial. The problem is that it is binding a buffer, which could be unexpected by the user and over-ride their existing binding. This is another reason it would be better to be inside buffer, as I can assert that the buffer is bound instead.

I'm happy to add a similar function, I'm just not sure how to implement it without shape / ndim. And I'm not sure how to integrate those cleanly. Any suggestions are more than welcome =)

On Thu, Apr 11, 2013 at 11:33 AM, Luke Campagnola notifications@github.comwrote:

Have a look at the way ShaderProgram/.set_buffer is done in my latest commit. That one is very different from the one you merged here and more true to the way the old Buffer class was structured.. I'm sorry, I should have updated the pull request to point to those changes instead (I only mentioned it in email) :/

I'll try merging in your changes and see what can be done..

— Reply to this email directly or view it on GitHubhttps://github.com/adamlwgriffiths/PyGLy/pull/23#issuecomment-16212475 .

adamlwgriffiths commented 11 years ago

Oh sorry, you're referring to changes not in the commit, I'll take a look now.

On Thu, Apr 11, 2013 at 11:45 AM, Adam Griffiths < adam.lw.griffiths@gmail.com> wrote:

I have to admit I haven't come to terms with Git. I had trouble managing the merge because it was from your fork which happened just before I made some big changes, so it was hard to keep track of things. I tried for a half a day to get git to behave but I couldn't get it to do what I wanted. Hence manual merge. So I apologize for this.

The 'set_buffer' function in the shader has a few issues. It relies on 'ndim' and 'shape' in Buffer. It is also calling vertex buffer functions in the shader. It should really be the other way around, passing the shader to the vertex buffer. It also doesn't unbind the buffer, that is trivial. The problem is that it is binding a buffer, which could be unexpected by the user and over-ride their existing binding. This is another reason it would be better to be inside buffer, as I can assert that the buffer is bound instead.

I'm happy to add a similar function, I'm just not sure how to implement it without shape / ndim. And I'm not sure how to integrate those cleanly. Any suggestions are more than welcome =)

On Thu, Apr 11, 2013 at 11:33 AM, Luke Campagnola < notifications@github.com> wrote:

Have a look at the way ShaderProgram/.set_buffer is done in my latest commit. That one is very different from the one you merged here and more true to the way the old Buffer class was structured.. I'm sorry, I should have updated the pull request to point to those changes instead (I only mentioned it in email) :/

I'll try merging in your changes and see what can be done..

— Reply to this email directly or view it on GitHubhttps://github.com/adamlwgriffiths/PyGLy/pull/23#issuecomment-16212475 .

campagnola commented 11 years ago

Maybe we're looking at different commits--my set_buffer does not make use of shape/ndim. I'm also pretty new to this Git thing.. going through my repo right now to see where things are..

campagnola commented 11 years ago

OH! This is my fault. I have been pushing my changes up to the pyvis fork of pygly. I did not realize we were still talking about my personal fork (which I should probably get rid of)

Ugh. My apologies.

Have a look at set_buffer here: https://github.com/pyvis/PyGLy/blob/master/pygly/shader.py

and the BufferRegion/Buffer classes here: https://github.com/pyvis/PyGLy/blob/master/pygly/buffer.py

In THIS commit (I am such an idiot), Buffer is a subclass of BufferRegion, which was the major reorganization that I mentioned over email. It also has an optional element_count argument in BufferRegion.init that enables the use of arrays without compound dtype.

I suspect that at this point, our codebases are so different that I should just take the code you have and see whether I can (or should) recreate those changes..

adamlwgriffiths commented 11 years ago

Sorry about shuffling the code about on you. My bad.

'git stash' any modifications you've got (so you dont lose them) and try updating pygly and see how things go. I can add an optional element_count over-ride that if not set, gets it from the dtype. I can probably squeeze something similar to set_buffer into VertexBuffer.

On Thu, Apr 11, 2013 at 11:58 AM, Luke Campagnola notifications@github.comwrote:

OH! This is my fault. I have been pushing my changes up to the pyvis fork of pygly. I did not realize we were still talking about my personal fork (which I should probably get rid of)

Ugh. My apologies.

Have a look at set_buffer here: https://github.com/pyvis/PyGLy/blob/master/pygly/shader.py

and the BufferRegion/Buffer classes here: https://github.com/pyvis/PyGLy/blob/master/pygly/buffer.py

In THIS commit (I am such an idiot), Buffer is a subclass of BufferRegion, which was the major reorganization that I mentioned over email. It also has an optional element_count argument in BufferRegion.init that enables the use of arrays without compound dtype.

I suspect that at this point, our codebases are so different that I should just take the code you have and see whether I can (or should) recreate those changes..

— Reply to this email directly or view it on GitHubhttps://github.com/adamlwgriffiths/PyGLy/pull/23#issuecomment-16213071 .

adamlwgriffiths commented 11 years ago

Regarding your fork, if you keep your personal copy of PyGLy as a self contained repository, you can use a 'git submodule' to refer to it inside PyVis. It's the same method I use for PyGLy to use Pyrr. That might make any merges easier. Depends on your work flow though.

campagnola commented 11 years ago

I'm actually using subtree merges. PyGLy is currently embedded inside pyvis that way.. I haven't used submodules, though, so I can't say whether subtrees are better.

I've just merged the essential bits of my changes to your latest here: https://github.com/pyvis/PyGLy/commit/c2e6c0a571233ac5f7e7a1c43b29b486a2e8d695

This works just as it did before, but we should probably still discuss whether my changes are appropriate.

I'll write up a detailed list of the changes tomorrow (it's bedtime)

Luke

On Thu, Apr 11, 2013 at 12:16 AM, Adam Griffiths notifications@github.comwrote:

Regarding your fork, if you keep your personal copy of PyGLy as a self contained repository, you can use a 'git submodule' to refer to it inside PyVis. It's the same method I use for PyGLy to use Pyrr. That might make any merges easier. Depends on your work flow though.

— Reply to this email directly or view it on GitHubhttps://github.com/adamlwgriffiths/PyGLy/pull/23#issuecomment-16216177 .

adamlwgriffiths commented 11 years ago

Cheers Luke =)

adamlwgriffiths commented 11 years ago

Hmm, I like the VertexBufferField. It would remove the need for the 'name = None' methods in DtypeVertexBuffer. Any named properties could be extracted via the Field object.

adamlwgriffiths commented 11 years ago

I'm playing around with a few ideas in this gist.

https://gist.github.com/adamlwgriffiths/5361760

It looks like what you want is basically, to put a numpy array in a buffer and just provide enough information to process it as a single block of information.

But any design needs to take into account some other use cases. Essentially:

single contiguous block of values for a single purpose, ie numpy array of (-1,3) of colours.
single contiguous block of interleaved values, ie numpy array of (-1,6) or (-1,2,3) of positions and colours.
multiple blocks of values, ie numpy array of (2,-1,3) of N positions then N colours.
multiple blocks with different sizes, ie 1 numpy array of (-1,3) and a second array of (-1,2).
interleaved data of different data types, ie numpy array with dtype of [('position', 'float32', (3,)), ('colour', 'int16', (3,))
multiple blocks with different data types, ie 1 numpy array of (-1,3) 'float32' and a second array of (-1,2) of 'int16'.

The current design can do all of these using the basic buffer, but you don't get the helper functions. The dtype buffer can do: 1, 2, 5.

Perhaps adding "view"s to Buffer would be a better way to support this (check the gist). You could always create a concrete type class that wraps setting of data via the views.

campagnola commented 11 years ago

On Thu, Apr 11, 2013 at 4:48 AM, Adam Griffiths notifications@github.comwrote:

I'm playing around with a few ideas in this gist.

https://gist.github.com/adamlwgriffiths/5361760

It looks like what you want is basically, to put a numpy array in a buffer and just provide enough information to process it as a single block of information.

But any design needs to take into account some other use cases. Essentially:

single contiguous block of values for a single purpose, ie numpy array of (-1,3) of colours.

single contiguous block of interleaved values, ie numpy array of (-1,6) or (-1,2,3) of positions and colours.

multiple blocks of values, ie numpy array of (2,-1,3) of N positions then N colours.

multiple blocks with different sizes, ie 1 numpy array of (-1,3) and a second array of (-1,2).

interleaved data of different data types, ie numpy array with dtype of [('position', 'float32', (3,)), ('colour', 'int16', (3,))

multiple blocks with different data types, ie 1 numpy array of (-1,3) 'float32' and a second array of (-1,2) of 'int16'.

The current design can do all of these using the basic buffer, but you don't get the helper functions. The dtype buffer can do: 1, 2, 5.

Perhaps adding "view"s to Buffer would be a better way to support this (check the gist). You could always create a concrete type class that wraps setting of data via the views.

The idea of views is interesting; that you have a very thin object managing the actual buffer, and above that are views which each have their own offset/stride/type, possibly corresponding to regions/fields within the low-level buffer. This is roughly the relationship between Buffer, BufferRegion, and BufferField that I had proposed, and similar to your original implementation as well. The trouble was that having all these different classes made the API confusing and it was actually more difficult to accomplish very simple tasks like buffer = Buffer(my_data).

These are my thoughts at present:

1) VertexBuffer is a view on an underlying Buffer object. It has an offset, stride, size, and a type specification (see below).

2) VertexBuffer can be instantiated simply, in which case it automatically creates a low-level Buffer for itself. It can also be created as a view into a pre-existing Buffer. Most users will never need to create a Buffer manually.

3) VertexBufferField is not needed, since VertexBuffer has offset/stride. So asking for vertex_buffer['field_name'] would just return another VertexBuffer. This is nice because sub-fields , sub-regions, and complete buffers can all be used interchangeably.

4) The basic VertexBuffer class should definitely have some kind of built-in field management. To keep it as general as possible, it should allow but not require dtypes:

buffer = VertexBuffer(rows=100, fields=[('pos', GL_FLOAT, 3), ('color',

GL_FLOAT, 4)])

The type specification could accept python types (float, int) or numpy types (np.ubyte, ...) as well. If you already have your dtype, this could just be automated as:

buffer = VertexBuffer(rows=100, fields=data.dtype)

And if you already have you data, then it's even easier:

buffer = VertexBuffer(data)

To support arrays with simple dtypes, you would just specify the fields manually:

data = np.empty((100,7), dtype=float)
buffer = VertexBuffer(data, fields=[('pos', float, 3), ('color', float,

4)])

The VertexBuffer class needs only be aware of dtypes inside init. Past that point, it just uses the generic representation used by the 'fields' argument.

You could still have subclasses of VertexBuffer for more advanced memory management, but I think the features above would handle the most common cases (it covers everything except #4 and #6 above). A MultiVertexBuffer(VertexBuffer) class might be used to include support for subregions, or you could arrange this manually:

buf = Buffer(total_size)
vbuf1 = VertexBuffer(buffer=buf, offset=0, type=[...])
vbuf2 = VertexBuffer(buffer=buf, offset=len(vbuf1), type=[...])

Luke

adamlwgriffiths commented 11 years ago

Yep, I like this idea (so does the other person I'm working with =). I like that the underlying basic Buffer object remains, and that you can use a basic Buffer but provide extra information.

I'll give this a go next time I've got some free time.

Cheers for your input Luke, always good to have a second set of eyes.

adamlwgriffiths commented 11 years ago

I'm thinking buffer should mimic numpy.array. It's what im using underneath, so I may as well mimic it in buffer. buffers therefore, are static in size like numpy. But a handle can be taken from on buffer and used to create another which will resize it. This removes allocate and also means that shape and dtype are both valid.

creation

# buffer size = data.nbytes, shape = data.shape, dtype = data.dtype
Buffer( data )

# empty with specified shape
Buffer( shape = (100,3), dtype = 'float32' )

# defaults to numpy.float (ie, float64 for 64bit, float32 for 32bit)
Buffer( shape = (100,3) )

this could be made similar to python using vertex_array.[empty,zeroes,ones]( (100,3) ). otherwise it could just be left up to the user to pass the result of numpy.zeros( ) to the function.

# dtype becomes 'float32' because of numpy.array param converter decorator
Buffer( [ 1., 2., 3. ] )

# specify own dtype
Buffer( [ 1, 2, 3 ], dtype = 'float32' )

# complex dtypes (anything numpy supports
Buffer( [ (1., 2., 3.) ], dtype = [('position', 'float32', (3,2,)] )

I'm not sure about supporting GL_FLOAT as a dtype value, I wouldn't trust my code to be able to accurately parse and convert all possible values inside the dtype before passing it on. This could be added later though should it be important.

setting data

# assign
buffer[:] = data

# slice assignment
buffer[0,2] = [ x,y,z ]
buffer[-1,3] = data

# stride
buffer[0:5:2] = [ 1, 2, 3 ]

named param buffer['position'] = position_data

getting gl pointers

# not sure how this one would work if the return type is data
# this ability would mandate that array access returns an object
shader['in_position'] = buffer['position']

# set buffer attribute to shader attribute 'in_position'
buffer['position'] = shader['in_position']

# slicing
shader['in_position'] = buffer[-1,0]

# getting data out
# if slice returns object
nparray[:] = buffer[ 1, 2 ].value
nparray = buffer.value

# not sure if / where this could be useful
buffer[:]

the __getitem__ funcs may need to return an object that way you can pass it to shaders, etc. you can also later add .value to the object to let you get the actual data out.

This way you could set a gl pointer into the buffer just using slicing.

I can still leave in the existing set pointer functions for non-numpy like access buffer.set_vertex_pointer( x,y,z )

I haven't implemented slicing or stride syntax before, im not sure how it works internally in numpy. But i'll give it a shot.

campagnola commented 11 years ago

I need to roll this around in my head. Before I read it again, please clarify: Are you using 'Buffer' to refer to the same 'Buffer' I suggested on April 11, or to refer to 'VertexBuffer' or both?

On Tue, Apr 16, 2013 at 9:16 PM, Adam Griffiths notifications@github.comwrote:

I'm thinking buffer should mimic numpy.array. It's what im using underneath, so I may as well mimic it in buffer. buffers therefore, are static in size like numpy. But a handle can be taken from on buffer and used to create another which will resize it. This removes allocate and also means that shape and dtype are both valid.

creating, buffer size = data.nbytes, shape = data.shape, dtype = data.dtype Buffer( data )

empty with specified shape Buffer( shape = (100,3), dtype = 'float32' )

defaults to numpy.float (ie, float64 for 64bit, float32 for 32bit) Buffer( shape = (100,3) ) this could be made similar to python using vertex_array.empty,zeroes,oneshttp://(100,3). otherwise it could just be left up to the user to pass the result of numpy.zeros( ) to the function.

dtype becomes 'float32' because of numpy.array param converter decorator Buffer( [ 1., 2., 3. ] )

specify own dtype Buffer( [ 1, 2, 3 ], dtype = 'float32' ) complex dtypes (anything numpy supports Buffer( [ (1., 2., 3.) ], dtype = [('position', 'float32', (3,2,)] )

I'm not sure about supporting GL_FLOAT as a dtype value, I wouldn't trust my code to be able to accurately parse and convert all possible values inside the dtype before passing it on. This could be added later though should it be important.

I could further mimic numpy by making Buffer( shape ) use an empty / zeros function, either way.

setting data: assign

buffer[:] = data slice assignment

buffer[0,2] = [ x,y,z ] buffer[-1,3] = data stride

buffer[0:5:2] = [ 1, 2, 3 ] named param

buffer['position'] = position_data

getting gl pointers not sure how this one would work?

shader.in_position = buffer['position'] set buffer attribute to shader attribute 'in_position'

buffer['position'] = shader.in_position slice get

buffer[-1,0] buffer[ 1, 2 ] not sure if this is useful

buffer[:]

getitem funcs may need to return an object that way you can pass it to shaders, etc. you can also later add '.value' to the object to let you get the actual data out.

This way you could set a gl pointer into the buffer just using slicing.

you can still leave in the existing set pointer functions for non-numpy like access buffer.set_vertex_pointer( x,y,z )

I haven't implemented slicing or stride syntax before, im not sure how it works internally in numpy. But i'll give it a shot.

— Reply to this email directly or view it on GitHubhttps://github.com/adamlwgriffiths/PyGLy/pull/23#issuecomment-16481623 .

adamlwgriffiths commented 11 years ago

VertexBuffer is Buffer renamed. It was to avoid future ambiguities with Frame Buffers, Texture Buffers, etc. Sorry for the confusion =P

Essentially, VertexBuffer should just act like a numpy array.

adamlwgriffiths commented 11 years ago

Just FYI, I haven't forgotten about this. I've just been busy moving house and am currently waiting for internet connection.

Essentially I want the buffer to act like a numpy array buffer[:] = numpy_data buffer[::2] = data shader.in_position = buffer.attribute[::3]

I've got a basic idea of how to do this. I'll put some concepts up as Gists to discuss this further.

adamlwgriffiths commented 11 years ago

Hi Luke, I've opened an issue regarding the vertex buffer: https://github.com/adamlwgriffiths/PyGLy/issues/24

I've committed the initial re-write in a branch: https://github.com/adamlwgriffiths/PyGLy/tree/vertex_buffer

This is really just so you can see and comment on the code.

I've implemented the VertexBuffer get/set logic. It should mimic numpy arrays a fair bit. It doesn't support getting 'views' like numpy does. Perhaps I shouldn't bother trying. Any suggestions from you and your team would be appreciated.

Setting attributes is 'implemented' for named values, but its just a 'proof of concept' to show that it works using the new class. Im going to re-work the design into a more flexible solution.

Cheers, Adam

campagnola commented 11 years ago

Thanks Adam! I've been rather busy trying to graduate, but I might have some time to peek at this later in the week. I should have a lot more time to focus on this at the end of July.

On Tue, May 14, 2013 at 4:51 AM, Adam Griffiths notifications@github.comwrote:

Hi Luke, I've opened an issue regarding the vertex buffer:

24 https://github.com/adamlwgriffiths/PyGLy/issues/24

I've committed the initial re-write in a branch: https://github.com/adamlwgriffiths/PyGLy/tree/development

This is really just so you can see and comment on the code.

I've implemented the VertexBuffer get/set logic. It should mimic numpy arrays a fair bit. It doesn't support getting 'views' like numpy does. Perhaps I shouldn't bother trying. Any suggestions from you and your team would be appreciated.

Setting attributes is 'implemented' for named values, but its just a 'proof of concept' to show that it works using the new class. Im going to re-work the design into a more flexible solution.

Cheers, Adam

— Reply to this email directly or view it on GitHubhttps://github.com/adamlwgriffiths/PyGLy/pull/23#issuecomment-17864118 .

campagnola commented 11 years ago

On Tue, May 14, 2013 at 4:51 AM, Adam Griffiths notifications@github.comwrote:

Hi Luke, I've opened an issue regarding the vertex buffer:

24 https://github.com/adamlwgriffiths/PyGLy/issues/24

I've committed the initial re-write in a branch: https://github.com/adamlwgriffiths/PyGLy/tree/development

This is really just so you can see and comment on the code.

I've implemented the VertexBuffer get/set logic. It should mimic numpy arrays a fair bit. It doesn't support getting 'views' like numpy does. Perhaps I shouldn't bother trying. Any suggestions from you and your team would be appreciated.

I had a quick look at the new vertex_buffer code and it's not really clear to me how the new design is supposed to be used. Are any of the examples updated such that I can see how this should be used? Or can you explain how this works in relation to the suggestions I gave on Apr. 11?

Setting attributes is 'implemented' for named values, but its just a 'proof

of concept' to show that it works using the new class. Im going to re-work the design into a more flexible solution.

The attribute-setting is something in pygly that I have found conceptually counterintuitive. I'll give you an example: In python, imagine you have a class that does some computation on array data, and you have an array you would like to do work on. There are two ways you could conceive of writing this:

number_cruncher = MyNumberCruncherClass()
data = MyArrayClass()

# method 1:
output = number_cruncher.process(data)

# method 2:
output = data.process_by(number_cruncher)

Both methods are valid, although the first is the more popular and intuitive way to write that statement. PyGLy, though, seems to prefer the second method when assigning data to VAOs or shader attributes. For example (from renderable_cube):

self.vao.bind()
self.buffer.bind()
self.buffer.set_attribute_pointer_dtype( self.shader, 'in_position',

'position' )

So somewhere we have a Shader (which is analogous to number_cruncher above), we have a buffer (which is analogous to data above), and we have a VAO, which is essentially just a collection of pointers between shader attributes and buffers. Shader <=> VAO <=> buffer

So to me, a more intuitive approach looks like this:

self.vao.set_attribute_pointer(self.shader.in_position,

self.buffer['position'])

Or in the case of OpenGL 2.1 as we are using:

self.shader.in_position.set_data(self.buffer['position'])

The essential difference is that the Shader and VAO are the 'active' components, rather than the buffer. Is there a reason that PyGLy prefers to use the buffer as the active component?

Luke

campagnola commented 11 years ago

Oh, this is hideous (I just realized github was reformatting my email in the worst possible way). Here's a re-post:

On Tue, May 14, 2013 at 4:51 AM, Adam Griffiths notifications@github.com wrote:

I've committed the initial re-write in a branch: https://github.com/adamlwgriffiths/PyGLy/tree/development

I've implemented the VertexBuffer get/set logic. It should mimic numpy arrays a fair bit. It doesn't support getting 'views' like numpy does. Perhaps I shouldn't bother trying. Any suggestions from you and your team would be appreciated.

I had a quick look at the new vertex_buffer code and it's not really clear to me how the new design is supposed to be used. Are any of the examples updated such that I can see how this should be used? Or can you explain how this works in relation to the suggestions I gave on Apr. 11?

Setting attributes is 'implemented' for named values, but its just a 'proof of concept' to show that it works using the new class. Im going to re-work the design into a more flexible solution.

The attribute-setting is something in pygly that I have found conceptually counterintuitive. I'll give you an example: In python, imagine you have a class that does some computation on array data, and you have an array you would like to do work on. There are two ways you could conceive of writing this:

    number_cruncher = MyNumberCruncherClass()
    data = MyArrayClass()

    # method 1:
    output = number_cruncher.process(data)

    # method 2:
    output = data.process_by(number_cruncher)

Both methods are valid, although the first is the more popular and intuitive way to write that statement. PyGLy, though, seems to prefer the second method when assigning data to VAOs or shader attributes. For example (from renderable_cube):

    self.vao.bind()
    self.buffer.bind()
    self.buffer.set_attribute_pointer_dtype( self.shader, 'in_position', 'position' )

So somewhere we have a Shader (which is analogous to number_cruncher above), we have a buffer (which is analogous to data above), and we have a VAO, which is essentially just a collection of pointers between shader attributes and buffers. Shader <=> VAO <=> buffer

So to me, a more intuitive approach looks like this:

    self.vao.set_attribute_pointer(self.shader.in_position, self.buffer['position'])

Or in the case of OpenGL 2.1 as we are using:

    self.shader.in_position.set_data(self.buffer['position'])

The essential difference is that the Shader and VAO are the 'active' components, rather than the buffer. Is there a reason that PyGLy prefers to use the buffer as the active component?

adamlwgriffiths commented 11 years ago

Hi Luke,

I've committed my re-work of VertexBuffer. I'm quite happy with it. https://github.com/adamlwgriffiths/PyGLy/blob/vertex_buffer/pygly/vertex_buffer.py

The examples have been updated too. https://github.com/adamlwgriffiths/PyGLy/blob/vertex_buffer/pygly/examples/renderable_colour_cube.py https://github.com/adamlwgriffiths/PyGLy/blob/vertex_buffer/pygly/examples/renderable_cube.py https://github.com/adamlwgriffiths/PyGLy/blob/vertex_buffer/pygly/examples/renderable_textured_quad.py https://github.com/adamlwgriffiths/PyGLy/blob/vertex_buffer/pygly/examples/renderable_triangle.py

I've scaled the VertexBuffer back to just being a simple wrapper around the GL buffer calls. Properties like shape, dtype, ndim, etc are quite complex to implement properly.

I managed to write 90% of the funcitonality required to mimic numpy arrays (slicing, dtype changes, shape changes, etc) before deciding it wasn't the way to go. Managing the buffer in a numpy style way can be done by having a 'shadow buffer', a numpy array which you keep and update and then push into the VertexBuffer when you're ready to push to opengl.

The buffer attributes were previously kept inside the buffer because they are not shader attributes, theyre more of a VAO attribute. But VAOs arent in GL legacy, so it made more sense to put them into the buffer class itself. This is the problem with GL evolving over time.

GL shader attributes are essentially an index. This is really a byte offset of the attribute / uniform memory block. Attributes can be linked 2 ways.

The first manually assigns indices. You risk creating invalid indices if you're not careful.

location = 1
shader.attributes['xyz'] = location
gl*AttribPointer( location, ... )

The second uses the pre-assigned shader location and passes it to the buffer pointer function

gl*AttribPointer( shader.attributes['xyz'].location, ... )

By adding gl*AttribPointer logic to Shader, you're adding buffer reading logic to the Shader, which really isn't right. It also means it needs to check if the buffer is bound, which means you may accidently unbind another buffer bound by the user. This is why the logic was inside the buffer. It is aware of it's binding state. It also provided a common access point between GL Core and Legacy.

However, I've changed how this is done in PyGLy. The attribute pointer calls are now external to the buffer and performed by BufferAttribute objects. I later found out that this is how Glumpy does it (https://code.google.com/p/glumpy/source/browse/glumpy/graphics/vertex_buffer.py).

The separation of attribute / data means that the buffer API is far simpler and won't need to be changed. Adding new pointer logic can be done by adding more classes or more classmethods to BufferAttribute (as you can see from the BufferAttribute.from_dtype classmethod).

Hope this helps =)

Cheers, Adam

campagnola commented 11 years ago

This looks good! The separation between buffer and attributes is very natural and, I think, consistent with

By adding gl*AttribPointer logic to Shader, you're adding buffer reading logic to the Shader, which really isn't right.

The buffer-reading logic (determining size, offset, stride, data type, etc.) would all be encapsulated in the call to buffer['field_name'], which would return an object very similar to your BufferAttribute class (sans 'location').

Conceptually it 'feels' right to me, since the shader is actually the code that will operate on the buffer. I realize this isn't exactly how OpenGL implements its API, but on the other hand I think it's safe to say that the OpenGL API is not remotely intuitive. In any case, removing AttribPointer from the buffer largely resolves this issue.

It also means it needs to check if the buffer is bound, which means you may accidently unbind another buffer bound by the user.

I find OpenGL's use of global variables (binding) to be terribly unintuitive, so my temptation when looking at this code is to try to hide all binding operations from the user. I realize this is entirely counter to the intended design of pygly, though, which prefers to hide nothing from the user.

I think, with regard to vispy, my inclination at this point is to write a slightly-heavier layer on top of pygly that provides some of the code simplification I'm looking for, while allowing pygly to stay true to the OpenGL API. Either that, or I will learn in the process of doing that why it is a bad idea to hide OpenGL's ugliness. (Is there anything that becomes more difficult if all binding is hidden?)

adamlwgriffiths commented 11 years ago

I totally encourage you to make a heavier layer onto. Pygly isn't meant to be high layer. just a light glue. a big goal is to not restrict coding style. If you can find a way to hide binding I'd to hear about it. Numpy style slicing would bethe bestbut unfortunately OpenGL can't handle the complex shapes and strides that are so natural for Numpy.

The best method I've seen are cg style material passes for shaders. But for the actual pointer bindings. the Easiest may a single buffer per attribute. You will suffer a slight performance penalty though.

I want to get working is in memory buffers. The gl*attribpointer functions can take a buffer pointeras the offset parameter if you don't bind a buffer I tried get this working today with Numpy arrays without success. it may be a python thing.

Sorry for grammar. nexus keyboard is being insane. I'll provide more info tomorrow

Cheers

adamlwgriffiths commented 11 years ago

Regarding bindings:

Pyglet vertex buffers are defined using strings.

Pyglet formatting looks like this: ('v3f', (data)), ('n3f', (data)), ('0g3f', (data)), ... v = vertex, n = normal, t = texture coord, g = generic attribute http://www.pyglet.org/doc/programming_guide/vertex_attributes.html

because you also need the binding location for the shader, these must be printf'ed into the string (0g3f, 1g3f, 2g3f).

I think this is a good progression. You can quite easily parse the format strings and calculate the buffer size, then push the data into the vertex buffer. The format strings are also trivial to convert to a gl*AttribPointer.

I used something similar for my texturing code, but I'm not happy with it because it's forced upon the user at the lowest level API. Anything like this should be optional IMO. Which is why I like the BufferAttribute.from_dtype(...) method in the new VertexBuffer. Adding new functions like this is easy. Adding wrappers is also easy.

You can also write methods to interleave the data for you automatically. The only problem with doing this is updating data, because GL doesn't let you update indices selectively, you must update the entire buffer. So it's pros and cons.

The reason I moved away from this style formatting ('v3f') was I wanted to use the format used by numpy complex dtypes and python structs.

That being said, they aren't descriptive about 'what' is put into the buffer, just how.

campagnola commented 11 years ago

On Mon, May 20, 2013 at 9:43 PM, Adam Griffiths notifications@github.com wrote:

Regarding bindings:

I'm not sure what this has to do with bindings.. perhaps we are talking about different things when I say that I want to hide binding from the user? (I simply mean the user should never call anything.bind() )

Pyglet formatting looks like this: ('v3f', (data)), ('n3f', (data)), ('0g3f', (data)), ... v = vertex, n = normal, t = texture coord, g = generic attribute

I think I prefer the way you have done it in the past--using dtype or a structure like [('pos', float, 3), ...] is much more natural and pythonic as well as easier to parse.

adamlwgriffiths commented 11 years ago

Sorry, by bindings I meant the OpenGL pointer calls.

I'm glad the changes are to your liking =)

adamlwgriffiths / PyGLy