giawa / opengl4csharp

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

VAO support for instanced drawing. #19

Closed TheAIBot closed 5 years ago

TheAIBot commented 5 years ago

Currently the VAO class only supports drawing with Gl.DrawElements. I would like a way to draw instanced. Because it's hardcoded into the VAO that there is an indice buffer(i like this btw) you would need to call Gl.DrawElementsInstanced to support it.

I made a working solution that uses the VAO class to draw instanced but in doing so i came across a design issues i think should be thought out some more.

To begin with a VBO needs to have concept of a division factor which is used when drawing instanced. So to solve this i added the divisor as a constructor argument like this. public VBO(T[] Data, uint Divisor, BufferTarget Target = OpenGL.BufferTarget.ArrayBuffer, BufferUsageHint Hint = BufferUsageHint.StaticDraw) The problem is that there already exists a constructor like this. public VBO(T[] Data, int Length, BufferTarget Target = BufferTarget.ArrayBuffer, BufferUsageHint Hint = BufferUsageHint.StaticDraw) The only difference between these two constructors is the second argument. In the first one it's an uint and in the second one it's an int. It would be extremely easy to mix up those two which would cause some hard to catch bugs. If the divisor should be part of the constructor then it should set itself more apart than that but i am not sure how i would go about doing it. Not sure if the divisor should even be part of the constructor, but if it isn't then that causes its own problems.

giawa commented 5 years ago

I don't have an immediate solution, but do have some questions that might help arrive at a solution that we're both happy with.

  1. What do you think of having a class specifically for VBOs that will support instanced rendering (could inherit VBO to continue to be supported by existing VAO code)? Something like public class InstancedVBO<T>? Or, if it makes more sense to put this in the VAO, something similar?
  2. What do you think of initializing VBO with a Divisor property that defaults to 0 and must be set non-zero to be able to render as instanced? The VAO draw function could check for the Divisor being non-zero and throw an error in the appropriate situation.
  3. What do you think of making the divisor a more specific constructor of the VBO. So you could simply add the new constructor public VBO(T[] Data, int Position, int Length, uint Divisor, BufferTarget Target = BufferTarget.ArrayBuffer, BufferUsageHint Hint = BufferUsageHint.StaticDraw) and require the developer include position and length values.
TheAIBot commented 5 years ago

To support instanced drawing, each VBO needs a divisor. It's only a single uint so i think making a whole new class for it is a bit overkill. There isn't any reason why a VBO used for instanced drawing couldn't also be used for non-instanced drawing. The difference between those two is primarily which draw call is used.

I quite like 2 and 3 but i am not sure which one is the best. You are right that there is more ways to make the divisor more distint in the constructor. Maybe adding it to the constructor but with a default argument like this. public VBO(T[] Data, int Length, BufferTarget Target = BufferTarget.ArrayBuffer, BufferUsageHint Hint = BufferUsageHint.StaticDraw, uint Divisor = 0) Would be easy enough to do and there would be no need to duplicate constructors just to add the divisor. The user can always get access to the divisor if needed and default values are provided for the preceding arguments if the user doesn't care for them.

I will make a pr with this way. If it doesn't work out like we would like then it's easy to change.

giawa commented 5 years ago

I think that method is fine. I'll accept the pull request once the code review is complete. Thanks!