Abstraction of OpenGL Data types

[Quaker762]

Very basic abstraction of more complex OpenGL concepts (Vertex Array Objects and Vertex Buffer Objects). Makes the whole thing much more readable and easier to handle. Should also make everything a little bit more typesafe and memory safe (no segfaults).

There's also a nice demo in main as to how it works. As is clearly visible, there is a LOT less OpenGL bullshit to type out, as the object does it all for you! :)

[Quaker762]

Oh, and I've discovered possibly the best thing ever. It's called CodeXL (it's an OpenGL/OpenCL/CPU Debugger). I'll update the readme, as everyone should be using this to debug 3D data (especially when we get to anything cutscene releated).

[Quaker762]

I think AddVBO should either return a handle, which can be passed to SetDataLocation

What handle would I return from AddVBO?? The whole point of it I suppose is to have a list of all current buffers loaded for a particular VAO.

instead of specifying the Attribute, or (preferably) a proxy-object, which you can call SetDataLocation on without having to specify any kind of Attribute.

How would I go about doing this? The only issue I can see is that glVertexAttribPointer requires an attribute in parameter one. I suppose I could add that to the buffer itself, instead of having an enum list. How would I pass a proxy object in??? IMHO it's easier to just have a list of the buffers and bind them, due to the way OpenGL works. Mainly I have it like this because it adheres to the way OpenGL requires us to do things without the annoyance of cluttering up the code with glXYZ calls everywhere and raw data manipulation.

I'm not a huge fan of the way main.cpp is used to test whatever feature was most recently added. It would make more sense to have each of this in a separate main() method in a separate file (and thus program).

I probably should have just taken a screenshot, but I kind of wanted to demonstrate how the API for this works, as it was a little bit fiddly, and there are certain (calling) rules that need to be adhered to so that OpenGL doesn't shit the bed.

Would it be better to have a whole lot of C functions then, rather than implementing this in classes??

[z33ky]

What handle would I return from AddVBO?? The whole point of it I suppose is to have a list of all current buffers loaded for a particular VAO.

And you would keep that. AddVBO would just return a handle, like the index of the most recently added object, to identify the VBO that was added. This is used by SetDataLocation to identify the object. In fact, the index already exists, its value just assumed to be whatever enum Attribute constant.

How would I go about doing this? The only issue I can see is that glVertexAttribPointer requires an attribute in parameter one. I suppose I could add that to the buffer itself, instead of having an enum list. How would I pass a proxy object in??? IMHO it's easier to just have a list of the buffers and bind them, due to the way OpenGL works. Mainly I have it like this because it adheres to the way OpenGL requires us to do things without the annoyance of cluttering up the code with glXYZ calls everywhere and raw data manipulation.

Something like this:

struct ProxyObject final
{
    Attribute self;
    sh3_glvertarray &array;
    void SetDataLocation(...) { array.SetDataLocation(self, ...); }
};

I think you misunderstood what I had in mind with a proxy object.

I probably should have just taken a screenshot, but I kind of wanted to demonstrate how the API for this works, as it was a little bit fiddly, and there are certain (calling) rules that need to be adhered to so that OpenGL doesn't shit the bed.

It makes sense, both to illustrate the API like you stated, and also to have a test ready in case someone wants to change something and test that it still works. I don't think removing/commenting out whatever was last in main() is a good approach though, since the next thing to test is going to come along and main() will be cluttered up some more (or if the code is removed, the example/test case is gone).

Would it be better to have a whole lot of C functions then, rather than implementing this in classes??

I didn't state that the OO encapsulation was bad. In contrast, I think it's a good approach. I just don't like the implicit assumptions about the sh3_vertarray::buffers-index (i.e. the values of enum Attribute to use).

[Quaker762]

And you would keep that. AddVBO would just return a handle, like the index of the most recently added object, to identify the VBO that was added. This is used by SetDataLocation to identify the object. In fact, the index already exists, its value just assumed to be whatever enum Attribute constant.

Ohh I see, that's a much better way of handling it, I didn't think of that.

I think you misunderstood what I had in mind with a proxy object.

So basically something that encapsulates the VAO without having to pass an attribute? I'm not quite sure that will work with what I have planned for the 3D model section.

I don't think removing/commenting out whatever was last in main() is a good approach though, since the next thing to test is going to come along and main() will be cluttered up some more (or if the code is removed, the example/test case is gone).

I see, I have the same sentiment. Are there any testing frameworks we can use (like jUnit), or is better to have multiple mains in a test folder and then have test targets?? Might come in handy for a 3D camera test.

I just don't like the implicit assumptions about the sh3_vertarray::buffers-index (i.e. the values of enum Attribute to use).

My reasoning is that (at least from my deductions), that it's up the programmer to decide which attributes are bound where. For example, we could have it in the order: UV, vertex, normals, but that's a pretty bad idea. It's also not really an implicit assumption per se, as SILENT HILL's vertex format is in this exact same order, so it's technically consistent. But I do agree, the handle idea does make it a lot cleaner and makes a lot more sense, though an attribute ID is technically different to an attribute index. I've gone and changed it to the handle idea, but I still think having a weakly typed regular enum to pass to OpenGL is a good way to handle it.

[Quaker762]

I've just stumbled upon another GL abstraction library here: https://github.com/Overv/OOGL/blob/150aebfa307a76119f62085cb0d73991793c0dbe/include/GL/GL/VertexArray.hpp

It seems that this author came to almost the same conclusion I did for his VAOs and VBOs (I'm taking a look at other C++ abstractions of OpenGL data objects)

[z33ky]

So basically something that encapsulates the VAO without having to pass an attribute? I'm not quite sure that will work with what I have planned for the 3D model section.

Attribute a = vao.AddVBO(o); vao.SetDataLocation(a, ...); is equivalent to Proxy p = vao.AddVBO(o); p.SetDataLocation(...).

I see, I have the same sentiment. Are there any testing frameworks we can use (like jUnit), or is better to have multiple mains in a test folder and then have test targets??

There is no one big C++ testing framework. A colleague of mine recently reviewed testing frameworks of various open source C libraries, I'll ask him if he knows some good C++ ones.

To start with, I think just a test-directory with one target (executable) per test and no automatic testing is fine.

My reasoning is that (at least from my deductions), that it's up the programmer to decide which attributes are bound where. For example, we could have it in the order: UV, vertex, normals, but that's a pretty bad idea. It's also not really an implicit assumption per se, as SILENT HILL's vertex format is in this exact same order, so it's technically consistent.

The assumption being backed by the data layout does not make it explicit. In the current version, the attribute that gets modified by a call to SetDataLocation is not visible in the call itself, it can only be implicitly from the context where (and when) it is called.

But I do agree, the handle idea does make it a lot cleaner and makes a lot more sense, though an attribute ID is technically different to an attribute index.

Right. If I understand this right, an attribute ID is a higher level concept represented by the enum Attribute, right? While the attribute index is the index into sh3_glvertarray::buffers? And it "just happens" that there's a 1-to-1 mapping from ID to index, yeah?

Anyways, if this change is adopted then the enum Attribute is not even needed, at least not in this class. It can instead be moved to the class texture. We might also want to use this VAO abstraction for procedurally generated objects, e.g. quads, boxes or spheres used for debugging, where we might only have vertices (SH3_VERTEX_SLOT) and indices (SH3_INDEX_SLOT), but no texture. So there we would use a different enumeration.

[Quaker762]

Attribute a = vao.AddVBO(o); vao.SetDataLocation(a, ...); is equivalent to Proxy p = vao.AddVBO(o); p.SetDataLocation(...).

Ohhh okay I get it now.

To start with, I think just a test-directory with one target (executable) per test and no automatic testing is fine.

Yeah this sounds much better. Would it perhaps be feasible to compile the engine as libsh3.a or something like that (basically an engine/game detachment). I'll open an issue if you think this might work.

The assumption being backed by the data layout does not make it explicit. In the current version, the attribute that gets modified by a call to SetDataLocation is not visible in the call itself, it can only be implicitly from the context where (and when) it is called.

I think I see what you mean now, so the caller knows what type was passed in, but the actual function itself doesn't know what type was explicitly passed (because of the weak typing)?

Right. If I understand this right, an attribute ID is a higher level concept represented by the enum Attribute, right? While the attribute index is the index into sh3_glvertarray::buffers? And it "just happens" that there's a 1-to-1 mapping from ID to index, yeah?

Yeah, it's sort of hard to explain. At the end of the day, it's all got to do with how the streams are passed into the vertex shader so we can perform operations on them (using the layout(location = 0) in style syntax). It makes more sense to hardcode the positions, because, as you've said, we might bind a quad with only vertex index and uv data, which makes our shaders consistent as well.

Anyways, if this change is adopted then the enum Attribute is not even needed, at least not in this class. It can instead be moved to the class texture.

How so? The attributes are part of the VAO (even part of the glprogram is you can spin it like that I suppose haha).

We might also want to use this VAO abstraction for procedurally generated objects, e.g. quads, boxes or spheres used for debugging, where we might only have vertices (SH3_VERTEX_SLOT) and indices (SH3_INDEX_SLOT), but no texture. So there we would use a different enumeration.

Yes!! This is point of abstracting it the way I did. Not all geometry will be laid out the same way, and having (if you will) "pre-baked" locations makes it much simpler imho when it comes to writing a shader, as we know what to expect in which location. Although, I do see what you mean by having the attributes per class (like having them in texture so we can map vertex to 0, uv to 1 and index to 2 etc, which will work given the current implementation).

[Quaker762]

You skipped many of the requested improvements :( Why is that?

Aside from what I emailed you about, I maaaay have forgotten to do a git stash pop at 12am sorry HAHA.

[Quaker762]

@z33ky

I've changed the way this works to your 'handle' idea. It actually works much better than how I originally had it, and also makes sure correct order is maintained across the VAO and shader. It should work really well, but until we have some real OpenGL up and running, it's hard to tell haha.

[Quaker762]

@z33ky Uh-oh....

Aside from the conflict (which I thought would happen...), it's good to go! No more 2982839428374 number of elements to draw (and grinding the program to a halt haha).

[Quaker762]

Sorry, this drags on so long, but after these superficial things are fixed I'll approve it. I promise ;)

Haha it's no issue, I'd rather have the code nice as well instead of being half arsed.

[z33ky]

Also, to resolve the merge conflicts you should rebase your code on master.

[Quaker762]

Did you test whether Doxygen ignores the dashes after parameter names?

I just checked, it keeps them in there. I'm just used to using dashes in a list of things, but aligning is a lot cleaner. I'll remove all of them.

I also pointed out where exactly documentation is missing.

Whoops, I must have missed some of that sorry.

[z33ky]

Ah, and don't forget to use @c and @p like for the Attribute of vaoparent. It instructs Doxygen to use typewriter font.

[Quaker762]

Ah, and don't forget to use @c and @p like for the Attribute of vaoparent.

Where did you want me to put that?? @tparam @p Attribute???

[z33ky]

No. See https://github.com/Quaker762/sh3redux/blob/8071b83685bfcaaa0686e5deb076b249edbf8c1f/include/SH3/system/glvertarray.hpp#L232. Use @c to refer to values (including symbolic ones like enum members) and @p to refer to the parameter (outside of @param Attribute). For forget what the difference between the two commandos is, but it made sense this way when I looked them up.

[Quaker762]

@z33ky This should be all good to go unless I've missed anything else (which I hope I haven't haha)

[Quaker762]

Now we're rolling ;)