Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Change OpenGL 3 Renderer to Core Profile #234

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

calebabutler
Copy link
Contributor

This is a much smaller pull request compared to my previous one that only switches to the 3.3 core profile without any of the other major changes. I also cleaned up the code a little bit and made the reasons why I was commenting certain things out more obvious.

This profile does not allow any pre-modern OpenGL practices, making it a
good way to make sure the OpenGL 3 video driver is resilient to
different OpenGL implementations (especially the ones that only support
modern practices).
There are still major graphical glitches that make it unplayable, but
the GUI is rendering as well as many game objects.
The game is now playable and the driver is working correctly!
I added a minor optimization that removed the allocation of new VBOs
every beginDraw call. Instead there is a global VBO used by all
beginDraw calls except ones called by drawHardwareBuffer. Still
inefficient and can be vastly improved, but at least it's better than
before.
I want to explain why I commented out GL_GENERATE_MIPMAP_HINT in case
someone knows a good replacement for this hint. I believe there might be
a performance regression by me commenting out these lines so a
replacement might be needed.
@numberZero
Copy link
Contributor

Thanks for the PR.
Maybe instead of using a global, split beginDraw into pointer-accepting and offset-accepting versions, so that the former prepares the buffer and calls the latter? Would that integrate well with the existing call sites?

Instead of having a global variable UseGlobalVBO, have multiple versions
of beginDraw and drawVertexPrimitiveList that do and do not use
GlobalVBO. This is better coding practice and could prevent strange bugs
from forming in the future due to UseGlobalVBO being set to an
unexpected value.
@calebabutler
Copy link
Contributor Author

Thank you, it required a little bit of reworking the drawHardwareBuffer and drawVectorPrimitiveList functions, but I replaced the global with two functions as I agree that that is a much better way of doing it.

}
}

void COpenGL3DriverBase::beginDraw(const VertexType &vertexType, int vertexCount, uintptr_t verticesBase)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that verticesBase is always a pointer and not an offset, it should likely be const void *

@numberZero
Copy link
Contributor

it required a little bit of reworking the drawHardwareBuffer

I guess EHM_NEVER is going to cause grief anyway. And it’s the default IIUC...

Some time ago, I’ve seen code that simply replaces EHM_NEVER with something else, in all buffers, everywhere, just to make it work with WebGL (which can only render from buffers, just like GL Core). Not the cleanest way but it suggests that treating EHM_NEVER like something else in the whole driver may very well work.

Also, how do you handle index buffers?

Replace unnecessary type uintptr_t with an actual pointer type.
When EHM_NEVER is used, then GL_ARRAY_BUFFER is not bound, so it does
not make sense to use drawVertexPrimitiveListWithBoundVBO. On the other
hand, since all data is on the GPU anyway now, EHM_NEVER does not make
much sense as an option in the first place and doesn't really do what's
expected.
@calebabutler
Copy link
Contributor Author

Thank you for the code correction, and you were absolutely right about changing that type. So, right now my video driver handles index buffers in not such a good way; it assumes GL_ELEMENT_ARRAY_BUFFER is bound before calling drawVertexPrimitiveList when you call the function in one of the modes that ends up calling glDrawElements. I'm sure this can be done much better. I realized I was not handling the EHM_NEVER case right ever, so I changed the code slightly, for vertex buffers it can (kind of) handle it by using the global VBO, and for index buffers it just ignores it. If this is going in the wrong direction let me know.

@numberZero
Copy link
Contributor

numberZero commented Sep 28, 2023

Ignoring EHM_NEVER works fine for WebGL:

//! Draws a mesh buffer
void CWebGL1Driver::drawMeshBuffer(const scene::IMeshBuffer* mb)
{
if ( mb )
{
// OK - this is bad and I hope I can find a better solution.
// Basically casting away a const which shouldn't be cast away.
// Not a nice surprise for users to see their mesh changes I guess :-(
scene::IMeshBuffer* mbUglyHack = const_cast<scene::IMeshBuffer*>(mb);
// We can't allow any buffers which are not bound to some VBO.
if ( mb->getHardwareMappingHint_Vertex() == scene::EHM_NEVER)
mbUglyHack->setHardwareMappingHint(scene::EHM_STREAM, scene::EBT_VERTEX);
if ( mb->getHardwareMappingHint_Index() == scene::EHM_NEVER)
mbUglyHack->setHardwareMappingHint(scene::EHM_STREAM, scene::EBT_INDEX);
COGLES2Driver::drawMeshBuffer(mb);
}
}

So should be fine on GL3 as well, as long as is done properly and systematically and doesn’t cause any leaks (Irrlicht way of caching meshes is prone of that to my knowledge).

@sfan5
Copy link
Member

sfan5 commented Oct 27, 2023

Assuming this is superseded due to #250

@sfan5 sfan5 closed this Oct 27, 2023
@numberZero
Copy link
Contributor

@sfan5 no it isn’t. Only #232 is, with #250 and this one together.

@sfan5 sfan5 reopened this Oct 28, 2023
@sfan5 sfan5 added the WIP Work-in-Progress label Oct 28, 2023
@numberZero
Copy link
Contributor

@calebabutler Any news on this?

And on EHM_NEVER on index buffers: I now think just ignoring one is wrong, it’s better to handle it the same way as vertices, moving to a buffer just before rendering. To reduce potential surprises from the “hw buffer link” and whatever else. I’d not expose any mixed RAM/VBO calls though, keeping all that stuff as an internal detail.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
WIP Work-in-Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants