Bind empty VAO to bufferless quad rendering

[ZzzhHe]

Hey! 👋 resolve #1659

I've made some initial fixes for the bufferless quad VAO issue, but wanted to get some advice before finalizing:

1. I tested my current code and noticed the VAO error was still there but occurred during shader validation in GlShaderProgram::use().

   This happens during program->update_uniforms(in); before geom->draw(); in GlRenderer::render().

2. My current fix is binding the empty VAO in geometry.cpp, but I'm wondering:

• Should we bind VAO before shader program validation?
• Or should we handle/modify the validation error check in GlShaderProgram::use()?

appreciate your thoughts on the best approach!

[heinezen]

Hey thanks a lot!

I read through this while on the train today, so I have not tried the code out yet. But here are my thoughts on the approach:

Defining quad_vao for every GlGeometry class is likely too much as the member is useless for anything that is not a bufferless quad. Usually having std::optional variables is not a problem, but the game will instance a lot of GlGeometry objects over its runtime. Also, OpenGL only allows a certain number of vertex arrays to be defined I think. So it's worth to think about a better solution here.

Making the VAO static would be nice, but since we need the OpenGL context I doubt it's possible. I think the least dirty solution would be:

1. Create a single VAO somewhere else (probably in the GlRenderer initialization)
2. Bind it at the start of every render(..) call
3. Keep the code in GlGeometry as-is, except for making a comment that a VAO needs to be bound in the switch statement branch for bufferless quad.

This ensures that a VAO is always bound during rendering and should also be much more storage/runtime efficient.

[heinezen]

I tested my current code and noticed the VAO error was still there but occurred during shader validation in GlShaderProgram::use().

This happens during program->update_uniforms(in); before geom->draw(); in GlRenderer::render().

This is normal btw ;) OpenGL does not raise errors because they happen on the GPU. Hence, we only learn about them during validity checks when we explicitly request the error state. You can also see the error in "real time" in the logs if you enable OpenGL debug output (by starting with the --gl-debug CLI parameter).

[ZzzhHe]

Got it, thanks! Working on the updates now.

[heinezen]

You can make this member a unique_ptr I think, since it's only used by the GlRenderer class.

[ZzzhHe]

I attempted to use unique_ptr, but encountered an issue since it requires the complete type definition. When I moved the include to the header file to replace forward declaration, it led to circular include dependencies. Did I do something wrong, or should I dive into the dependency cycle to fix it?

[heinezen]

No, you are right. I think you can change it back to shared_ptr.

I think the problem here is that unique_ptr only allows incomplete types under specific circumstances. I also didn't know the specifics for that :'D

std::unique_ptr may be constructed for an incomplete type T, such as to facilitate the use as a handle in the pImpl idiom. If the default deleter is used, T must be complete at the point in code where the deleter is invoked, which happens in the destructor, move assignment operator, and reset member function of std::unique_ptr. (In contrast, std::shared_ptr can't be constructed from a raw pointer to incomplete type, but can be destroyed where T is incomplete). Note that if T is a class template specialization, use of unique_ptr as an operand, e.g. !p requires T's parameters to be complete due to ADL.

https://en.cppreference.com/w/cpp/memory/unique_ptr

[ZzzhHe]

Thanks, I will change it back

[heinezen]

Very good!