OpenGL VertexArrayObject class

Question

It seems to play up sometimes and I'm not sure if this is due to the class itself or the way I'm using it. Can someone confirm that this is a 'good' implementation of a VertexArrayObject class?

//The following code is public domain

public class VertexArray : GraphicsResource
{
    protected readonly int ID;
    DrawMode _drawMode;
    VertexLayout _vertexType;

    protected BufferUsageHint bufferUsage = BufferUsageHint.StaticDraw;
    int vertexCount;
    int vertexBufferID;

    public DrawMode DrawMode
    {
        get { return _drawMode; }
        set { _drawMode = value; }
    }
    public int VertexCount
    {
        get { return vertexCount; }
    }
    public BufferUsageHint BufferUsage
    {
        get { return bufferUsage; }
        set { bufferUsage = value; }
    }

    public VertexArray()
        : base()
    {
        ID = createVAO();
        disposed = false;
    }
    public VertexArray(DrawMode drawMode)
        : base()
    {
        _drawMode = drawMode;
        ID = createVAO();
        disposed = false;
    }
    int createVAO()
    {
        int id;
        GL.GenBuffers(1, out vertexBufferID);
        GL.GenVertexArrays(1, out id);
        GL.BindVertexArray(id);
        GL.BindBuffer(BufferTarget.ArrayBuffer, vertexBufferID);
        VenGFX.currentVAO = this;
        return id;
    }
    public void Bind()
    {
        if (vertexCount == 0)
            throw new Exception("Vertex buffer has no data, and therefore can't be used");
        if (disposed)
            throw new ObjectDisposedException(ToString());
        GL.BindVertexArray(ID);
        VenGFX.currentVAO = this;
    }
    public void SetData<T>(T[] Array) where T : struct, IVertex
    {
        GL.BindVertexArray(ID);
        if (Array.Length > 0 && Array[0].Layout != _vertexType)
            setVertexType(Array[0].Layout);
        GL.BindBuffer(BufferTarget.ArrayBuffer, vertexBufferID);
        GL.BufferData(BufferTarget.ArrayBuffer, (IntPtr)(Array.Length * _vertexType.SizeInBytes), Array, bufferUsage);
        vertexCount = Array.Length;
        VenGFX.currentVAO = this;
    }
    public void SetData<T>(T[] Array, int length) where T : struct, IVertex
    {
        GL.BindVertexArray(ID);
        if (Array != null && Array.Length > 0 && Array[0].Layout != _vertexType)
            setVertexType(Array[0].Layout);
        GL.BindBuffer(BufferTarget.ArrayBuffer, vertexBufferID);
        GL.BufferData(BufferTarget.ArrayBuffer, (IntPtr)(length * _vertexType.SizeInBytes), Array, bufferUsage);
        vertexCount = length;
        VenGFX.currentVAO = this;
    }
    void setVertexType(VertexLayout layout)
    {
        _vertexType = layout;
        layout.Set();
    }
    protected override void Release()
    {
        if (VenGFX.currentVAO == this)
        {
            GL.BindVertexArray(0);
            VenGFX.currentVAO = null;
        }

        int id = ID;
        GL.DeleteBuffers(1, ref vertexBufferID);
        GL.DeleteVertexArrays(1, ref id);
    }
}

To draw with it, I write

if (mesh.VertexCount == 0)
    return;
if (currentVAO != mesh)
    mesh.Bind();

GL.DrawArrays((BeginMode)(int)mesh.DrawMode, 0, mesh.VertexCount);

Answer 1

I know nothing of OpenGL and what a correct implementation of a VertexArrayObject should look like, but I do have a couple of observations:

• I like that the ID private field is readonly, ...but its naming breaks the naming convention for private fields, which should be either id or _id - looking at the other private fields _id would be more consistent.
• I don't see why the other fields vertexCount and vertexBufferID (should be vertexBufferId) aren't prefixed with an underscore as well.
• Vertical whitespace between methods/members isn't consistent either. Skip a line between } and the next member, it'll breathe better.

public VertexArray()
    : base()

The : base() part is redundant. Base constructor always gets called.

In the Release() override, you're doing this:

    int id = ID;
    GL.DeleteBuffers(1, ref vertexBufferID);
    GL.DeleteVertexArrays(1, ref id);

Whatever DeleteVertexArrays is doing with id (passed by reference), you're not doing anything with the [possibly] modified value, I'm guessing because ID is readonly. But this is clearly a case where the "why" would be nicely explained with a short comment.

Lastly, properties like DrawMode that use a non-readonly backing field...

DrawMode _drawMode;
public DrawMode DrawMode
{
    get { return _drawMode; }
    set { _drawMode = value; }
}

...would probably be better off rewritten as auto-properties:

public DrawMode DrawMode { get; set; }
public BufferUsageHint BufferUsag { get; set; }
public int VertexCount { get; private set; }

Answer 2

I know this is an old question and has already been reviewed but I feel it's worth adding to in case anyone else happens to stumble across it or have a similar solution/question.

Firstly, I'd like point out that Matt's answer covers most of the syntactical/small changes that should be made to the code, so I'd like to address the question:

Is this an acceptable implementation of a OpenGL VertexArrayObject class?

Short answer: I would say no

Long answer: Well, here we go...

Vertex Array Objects

Before we proceed with the implementation, it is key to understand what exactly it is that we are implementing. The code in the OP is not what I would say is a Vertex Array Object (I'll delve into why not later), but for now, let us briefly define a VAO.

To summarise the link:

A VAO is a special type of object that encapsulates all the data that is associated with the vertex processor. Instead of containing the actual data, it holds references to the vertex buffers, the index buffer and the layout specification of the vertex itself

Sounds like we will be implementing some object that has to deal with a fair few things right? wrong. Like a lot of OpenGL 'objects', a VAO is simply identified by an integer given to us from the following command (OpenTK bindings):

GL.GenVertexArray();

It is essential to note that by saying we want to define a VertexArrayObject class, we are simply stating that we want a wrapper around the OpenGL 'object'. Therefore, all we have to do is identify the possible members & operations that the class will need.

Where to start? Well, the spec is always a good place.

Briefly, we can identify that it will need an integer member that stores the result of the above OpenGL invocation and also the following operations:

• GL.GenVertexArray()
• GL.BindVertexArray(...)
• GL.DeleteVertexArray(...)

I'd like to emphasise that really, all we are doing is wrapping an OpenGL object in a way such that we can interface to it in a more .NET manner.

Quickly back to OP's implementation It is doing far more than what a VAO is responsible for:

1. It is concerning itself with Vertex Buffers (When really, these are themselves an OpenGL object that should be wrapped separately to the VAO).
2. It is aware of how things will be drawn
3. It is responsible for buffer creation (again, this should be separate)
4. It is responsible for setting vertex data.

One point that really sticks out is the CreateVAO method. This should be responsible for creating the Handle to the OpenGL integer only. Currently, it is also generating handles to VBOs and binding the buffer objects. Also, your implementation is 'hard-coding' the type of Buffer that will be bound - ArrayBuffer. What if you wanted to support a different buffer type?

Another is the Bind method. You're throwing an exception if there is no buffer data, but this is not how VAOs behave. We are free to generate and bind VAOs without the presence of any buffers or with empty buffers.

The ID field would probably be better as a property with a private setter. This represents the 'handle' and will change as pointed out by Mat. You're stating that this integer will never change after initialisation but we know that on the OpenGL side, the integer it represents does mutate. Your code should show its intent and the behaviour between objects should be consistent.

An important note is that your class is dealing with unmanaged resources in the form of handles to OpenGL 'objects' and yet you are not handling this in the recommended way. Currently, you're cleaning up these resources in the Release method. I can see that your GraphicsResource contains a disposed boolean member, yet this is not used in your 'clean-up' code from what I can see.

Consider what happens if this method is invocated more than once. You would be attempting to delete a handle that has already been deleted.

If another developer was to use this class, and he was experienced in using OpenGL, he would soon become confused that this 'wrapped' version does not share the behaviour of the OpenGL one.

With this said, let's get back to an implementation I feel would be better.

Suggested Implementation

This could be improved, but a simple implementation follows:

internal sealed class VertexArrayObject : IDisposable
{
    internal int Handle { get; private set; }

    private bool isDisposed = false;

    internal VertexArrayObject(bool bind = false)
    {
        this.Handle = GL.GenVertexArray();
        if (bind)
        {
            Bind();
        }
    }

    internal void Bind()
    {
        CheckForDisposed();
        GL.BindVertexArray(this.Handle);
    }

    internal void UnBind()
    {
        CheckForDisposed();
        GL.BindVertexArray(0);
    }

    private void CheckForDisposed()
    {
        if (this.isDisposed)
        {
            throw new ObjectDisposedException(GetType().FullName);
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool manual)
    {
        if (!this.isDisposed)
        {
            if (manual){ }
            if (this.Handle != -1)
            {
                GL.DeleteVertexArray(this.Handle);
            }
            this.isDisposed = true;
        }
    }

    ~VertexArrayObject()
    {
        Dispose(false);
    }
}

Note: I've made the class internal only due to the fact that in my experience, I prefer to hide the OpenGL side from users of a game engine, but obviously that depends on how you're using the class/the goals of your application.

Obviously the above does not give you any functionality beside being a basic wrapper around the OpenGL 'object'. However, this has the following advantages:

• It is minimalist - You really are just getting a VAO, which is what we would expect, not a VAO with internally hidden buffers and features that should be separated out into other classes.
• It will be used just as the OpenGL 'object' would, except it hides the OpenGL calls.

In conclusion, I hope this answer addressed the points that were missing regarding the original question. I'm not saying this is the definitive way to implement a Vertex Array Object in C#, just an interesting point at which to begin.