giawa / opengl4csharp

OpenGL 4 Bindings (partially based on OpenTK) for C#
Other
232 stars 61 forks source link

Remove duplicate code from VAO #24

Closed TheAIBot closed 5 years ago

TheAIBot commented 5 years ago

VAO now internally uses GenericVAO. GenericVAO and VAO now support each others features.

The VAO class is now essentially a light wrapper around GenericVAO that makes it easier to create a vao with default attribute names.

I also added the following new constructor

public VAO(ShaderProgram program, GenericVAO.IGenericVBO[] vbos)

So now it's possible to do something like this(taken from my own project).

GenericVAO.IGenericVBO[] vbos = new GenericVAO.IGenericVBO[]
{
    new GenericVAO.GenericVBO<Vector2>(ShipShape, BasicShader.VShaderShapeName),
    new GenericVAO.GenericVBO<Vector3>(ShipNormals, BasicShader.VShaderNormalName),
    new GenericVAO.GenericVBO<Vector2>(ShipPositions, BasicShader.VShaderShipPosName),
    new GenericVAO.GenericVBO<Vector2>(ShipRotations, BasicShader.VShaderRotName),
    new GenericVAO.GenericVBO<byte>(Indices)
};

VAO shipVAO = new VAO(ShipShader, vbos);

Adding this constructor makes VAO<T!>, VAO<T1, T2> and so on obsolete. I want to add an obsolete attribute to the generic VAO classes but am not sure what it should say. Any suggestions?

giawa commented 5 years ago

Thanks for the PR! Do you mind splitting this into multiple PRs to make it a bit easier to follow? I see a couple of distinct new features that would be best split into their own commits/PRs I think.

1) Addition of new constructor public GenericVBO(VBO<T> vbo) and the changes to the generic VAO constructors due to this. 2) Addition of IGenericVAO interface and the modifications to GenericVAO to support this (including the addition of Offset and the code supporting it). 3) Changing VAO to use be a wrapper around the GenericVAO class.

Also, out of curiosity. Why the usage of two different ways of formatting properties?

public uint vaoID { get => vao.ID; } versus public uint ID => vao.ID;

Regarding the obsolete message (which I think could be included in PR#2) I would just write something similar to what you have for the previous VAO constructor with the int element array. Maybe something like:

"This VAO constructor is deprecated in favour of using VAO(GenericVAO.IGenericVBO[] vbos)."

Upon typing that above message, I wonder if it is also worth pulling IGenericVBO out into the OpenGL namespace to match what has been done with IGenericVAO. That way we don't need to prepend that type with GenericVAO every time. What do you think?

giawa commented 5 years ago

Also, it's great to hear you are using this library for your own project. Do you have a link to a blog/vlog/stream/etc where you are showing off development? It would be awesome to see how your project is coming together.

TheAIBot commented 5 years ago

Okay i will split it into multiple smaller PRs. Regarding the way the properties are formatted. Visual studio formatted it like that when i implemented the interface. I will change it so it's consistent with the rest of the project.

My own project isn't currently available anywhere. It's not even on github yet. I will eventually put it on github but it's currently a little too early for me to do that.