search

giawa / opengl4csharp

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

GenericVAO supports Offset #26

Closed TheAIBot closed 5 years ago

TheAIBot commented 5 years ago

Part 2 of 3 from #24.

Added IGenericVAO which GenericVAO now implements. Added Offset support to GenericVAO.

giawa commented 5 years ago

Thanks for taking a look at that code review. The offset caching looks good.

Sorry, thinking this through again. Can you explain the rationale for having the IGenericVAO interface? I know the next PR will be to mark VAO as IGenericVAO, but what is the justification for doing so? IGenericVBO exists because GenericVBO is actually using generics and we need a way to store collections of them. However, VAO and GenericVAO do not actually use generics (despite their name). Unless there is a reason to have a collection of GenericVAO and VAO objects together, I don't see the need for IGenericVAO. I'd like to get an understanding of use case for IGenericVAO before approving this merge request.

TheAIBot commented 5 years ago

There is three reasons for these PRs. The first is to merge the feature sets of VAO and VAO<...>. VAO<...> inherits from GenericVAO so essentially it's to merge the feature set of VAO and GenericVAO. I want to make sure the feature set of both VAO and GenericVAO are the same so i added the interface IGenericVAO that both GenericVAO and VAO(VAO will implement it in the next commit) has to implement. GenericVAO doesn't support offset and VAO doesn't support all the new features which were recently added to GenericVAO such as new element array types, support for integral attributes, support for double attributes and draw instanced.

The second reason is to reduce code duplication. VAO has a lot of code for binding and drawing that really isn't necessary as GenericVAO can do the exact same thing. So the next commit would rewrite how VAO binds buffers and draws by using GenericVAO for these things. But for VAO to use GenericVAO for everything their feature set has to be the same. This would change VAO to be nothing more than than a thin wrapper around GenericVAO that adds an easy way to setup a VAO with specific attribute names.

Now VAO is a non generic way to setup a VAO so i would like to add a new constructor in VAO that simply takes an array of IGenericVBO. That way you now have a way to make a VAO without specifying each buffer type and without being limited to 7-8 buffers.

Now all VAO<...> classes are obsolete and GenericVAO is now the only class that really needs to be maintained.

There is more than one way to do achieve this but these are the reasons. VAO could also inherit GenericVAO. Then there would be no need for an interface.

giawa commented 5 years ago

Right, I understand the reasons in general for these PRs. I was more curious about the need for IGenericVAO. I think this reply from you is the reason for the existence of the interface (let me know if I am wrong):

I want to make sure the feature set of both VAO and GenericVAO are the same so i added the interface

So the interface is there to ensure feature parity between these two objects. To be honest, I'm torn on this. If the user has no need for IGenericVAO and it is just to ensure us developers of the library maintain feature compatibility between these classes, then I think exposing IGenericVAO as a public interface is misleading. It also doesn't force us to ensure compatibility, since we would need to add new features to the interface (there's nothing forcing us to add those new features). That leaves your suggestion:

VAO could also inherit GenericVAO. Then there would be no need for an interface.

Which I think actually accomplishes the goal of feature parity better, since any additional features added to GenericVAO will automatically propagate to VAO, correct?

Or even just leaving VAO and GenericVAO as separate classes with no inheritance linking them. However, VAO would store a private GenericVAO vao still to avoid code duplication.

What do you think?

TheAIBot commented 5 years ago

Yeah i will make the changs. By inheriting, the interface and the redirecting properties in VAO aren't necessary. VAO will only contain constructors just like VAO<...> does. I suppose this PR is only adding Offset to GenericVAO so GenericVAO is a superset of the features in VAO