giawa / opengl4csharp

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

VAO now support element types byte, ushort and uint #23

Closed TheAIBot closed 5 years ago

TheAIBot commented 5 years ago

Implemented the support as you suggested in #22. Unfortunately it's still a breaking change because int isn't a valid element buffer type. According to https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glDrawElements.xhtml only byte, ushort and uint are valid element types. Not sure how we can avoid a breaking change here. Sure i could add int as a valid type. It would probably work in most cases but it's still technically a bug.

giawa commented 5 years ago

There's no point maintaining the backwards compatibility of the constructors if the code won't actually work with it. I think we should add int as a valid type and mark the old constructors as Obsolete (or similar) for now. Or have a different BindAttributes used by VAOs that were created using the old constructor that doesn't perform the same valid type checks. I know this is a pain, but in this case I think it is worth the headache to maintain support for the old VAO constructors.

Edit: I'm less happy with marking int as valid, since we would like to warn the user if they provide a VBO<int> as an element array. I'm leaning more towards any VAO constructed using the legacy constructor be allowed to use VBO<int>, and that's it. Those constructors should also be marked as Obsolete to discourage future usage.

TheAIBot commented 5 years ago

Alright all constructors that uses VBO<int> elementArray has been marked obsolete. By default int is an allowed element type so backawards compatibilty is preserved. All new constructors that specify the element type does not allow int as an element type.

giawa commented 5 years ago

Wanted to leave a note about this commit here: https://github.com/giawa/opengl4csharp/commit/7b3e0c4f228047d1c73284bf04ba7b8645e5ef6c This is to fix the non-generic version of the VAO and is currently in master. I'll bring this into the dotnetcore branch once we finish up with your PR, but want to make sure we don't accidentally duplicate work.

TheAIBot commented 5 years ago

I haven't really used the non generic version of VAO yet so i din't want to touch it. Nice to see that i don't have to now! One thing though. You may want to consider making the non generic VAO use the generic VAO. Something along the lines of.

public IGenericVAO
{
    int VertexCount { get; set; }
    bool DisposeChildren { get; set; }
    bool DisposeElementArray { get; set; }
    ShaderProgram Program { get; private set; }
    BeginMode DrawMode { get; set; }

    void BindAttributes(ShaderProgram program);
    DrawFunc Draw;
    DrawInstancedFunc DrawInstanced;
}

public class VAO : IGenericVAO, IDisposable
{
    //
    //Implement interface here
    //

    private IGenericVAO vao;
    public VAO(ShaderProgram program, VBO<Vector3> vertex, VBO<Vector3> normal, VBO<Vector3> tangent, VBO<Vector2> uv, VBO<int> element)
    {
        vao = new VAO<Vector3, Vector3, Vector3, Vector2>(program, vertex, normal, tangent, uv, new string[]
        {
            "in_position",
            "in_normal",
            "in_uv",
            "in_tangent"
        }, element);
    }

    //
    //Implement remaining constructors
    //
}

Very much like you did with IGenericVBO. Should cut down on code duplication and allow the non generic version to use new features such as DrawInstanced.

giawa commented 5 years ago

Agreed - will try to make this happen some time today. Thanks again for your work on this PR!