Speed up terrain drawing with glDrawArrays from OpenGL 2.0.

[Quipyowert2]

Generates a sprite sheet on the fly, queues UV and vertex coordinates in GC::drawSurface and finally in finishDrawingSprite, many different sprites in the texture atlas can be drawn at the same time with glDrawArrays.

With VSync off and after commenting out the SDL_Delays in GUIBase.cpp and Engine.cpp, I get a frame rate of about 500 on the title screen and around 100 or so in tutorial 4.

With the new sprite sheet code, the frame rate is roughly ~60 FPS without the two SDL_Delay calls. It was only about ~40 FPS before I optimized it, so about a 50% increase in FPS.

Note: This PR adds a new dependency, glad epoxy. The functions createTextureAtlas and finishDrawingSprite require OpenGL 2.0.

TODO:

• Fix crash in USL when starting tutorial. (Fixed by Fix libUSL crash due to a circular reference #121)

[stephanemagnenat]

Nice to see that there is optimization work in progress!
One question though: for the new glad dependency, could you adjust the SConstruct file. For example, on Ubuntu, I do not know what to install to have glad.

[Quipyowert2]

Sorry, I forgot to add the glad files to the PR. I downloaded the zip from Glad generator but forgot to extract and commit it.

[Quipyowert2]

On second thought, maybe Epoxy or OpenGL Extension Wrangler would be better options. The glad header and source file the website generated is over 4,000 lines and the license for the generated code is unclear. Glad is MIT licensed, the XML file it uses to generate the code is Apache licensed, and I don't know what license the generated code is under. I found this though: License of generated code.

I got it working using glad, but then decided to switch to epoxy.

[Quipyowert2]

Rebased onto master branch. Now it should not crash when I run the tutorial.

[Quipyowert2]

@stephanemagnenat My latest commit changes Sprite::atlas to be a unique pointer because it is the only pointer to the sprite sheet. createTextureAtlas should be slightly more exception-safe now.

Please let me know if you need me to squash the several "Fix foo" commits into one or remove the old commits referencing glad from this PR, or if you would like any other changes.

PS: FPS measured with this patch.

[Quipyowert2]

Fixed flags, zones, fog of war, and bullets not being drawn.

[stephanemagnenat]

@Quipyowert2 is this PR completed? If so, yes, please squash commits.

I have some questions though:

• DrawableSurface has now both an optional sprite pointer and an usingAtlas boolean. Are there cases when sprite is non-null but usingAtlas is false?
• In DrawableSurface, is there a reason why texX and texY are explicitly set to 0 but w and h are not? It might actually make sense to use std::optional here to jointly denote the using of a sprite sheet and the related data if it is used. You can imagine having a simple struct with the sprite sheet data, and have an optional of it.
• Why is the atlas code in DrawableSurface::initTextureSize(void) commented out?
• Why is the dirty set in void DrawableSurface::uploadToTexture(void) commented out?
• What happens if user code calls a draw function (e.g. drawPixel(...)) on a DrawableSurface that is using an atlas? Is a new surface allocated, the sprite data copied, and the link to the atlas severed? If not, there should be a note somewhere regarding this limitation.

[Quipyowert2]

The PR is just about done, but I had to add some missing finishDrawingSprite calls because some of the right panel items weren't being drawn. Regarding the first bullet point, usingAtlas variable is probably unnecessary.

Bullet point 2, yeah optional is probably a good idea here.

Re commented out code: The atlas code in initTextureSize is commented out because I was testing something and forgot to remove it. The line setting dirty is commented out because I didn't want to overwrite the sprite sheet with a single tile.

I'm not sure about what happens if you call drawPixel on a sprite-sheet. I might have to test that tomorrow.

[stephanemagnenat]

Ok, thank you for your quick answer. Please ping me when everything is done and you want this merged!

[Quipyowert2]

I have pushed a commit to prevent drawing to the Sprite::atlas surface after it has been generated. It is a little difficult to work on graphics programming right now as my laptop screen has started turning purple and flickering after every reboot for 15 minutes to several hours

[Quipyowert2]

I extended createAtlas to be able to handle a limited texture size, but I haven't pushed my changes to this branch yet. I have it on another branch (sprite-sheet-after-rebase). I thought I squashed the Fix stuff not being drawn in map editor commit into the other similar "Fix" commits, but apparently I didn't. I tried using std::optional, but it wouldn't compile until I changed Visual Studio's C++ standard to C++17. Then it complained about mem_fun, which was available in C++14 (the default standard in VS, I think), but has been removed in 17.

Things we use that have been removed in C++17:

• mem_fun (can be replaced with lambdas)
• random_shuffle (can be replaced with shuffle, maybe?)

[Quipyowert2]

I used boost::optional instead of std::optional to avoid requiring C++17. I ended up moving finishDrawingSprite to the Sprite class, then moving it back to the GraphicContext class. Moving the method to the Sprite class required moving the GLState object to the SDLGraphicContext.h header and passing it somehow to the Sprite class. I guess it makes more sense to queue drawing to a surface and then actually drawing it in the same class, but the code looks nicer in the Sprite class. Although, if we were to support DirectX as a rendering backend in the future, there would have to be two finishDrawing methods, one for GL and one for DX.

What do you think, should I keep finishDrawingSprite in the GC class or move it to the Sprite class?

The reason I am using two-dimensional vectors in the drawing code instead of 1D is that I was thinking that the graphics card might have a small max texture size, so would need multiple sprite-sheets to hold the entire sprite. Although OpenGL 2.0 supports at least 64x64 texture size (comment), my laptop's GTX 1060 supports textures up to a size of 32768, and the onboard Intel 630 graphics support textures with a size of 16384. What is the optimal texture size says to use 2k textures. Maybe YAGNI? Probably should make it a one-dimensional vector, I guess.

[Quipyowert2]

Reverted the changes that handled a limited max texture size, as they make the PR more complex for not much benefit. If the graphics card doesn't support at least 1024x1024 texture size, it probably can't run the game anyway. I also squished the commit casting the result of sqrt to integer into the first commit.

I rebased the PR several times and whittled down the number of commits from 20 to just 12.

@stephanemagnenat Does this look ready to merge to you? Please review.

• Compiles and runs on my machine

Edit: force-pushed to fix uninitialized vbo and texCoordBuffer variables and changed an outdated comment.

[stephanemagnenat]

Thank you very much for this work! Overall very good. My key blocking point is the disabling of software-only rendering, of which I do not see the necessity.

[stephanemagnenat]

An absolute detail, but I would find the writing of this as assert(sprite->vertices.size() == 0); closer to the conceptual intent.

[Quipyowert2]

Changed to assert that it is empty.

[stephanemagnenat]

Another small point: is there a way to check OpenGL version in SConstruct, in order to report warning and disable GL rendering if it is <= 2.0?

[Quipyowert2]

I'm working on this, but it's getting late here. I might have time to work on this tomorrow.

[Quipyowert2]

Does the PR look good now? I wasn't sure what to do if sprite sheet creation fails, but I changed it to just print a message for now.

Another small point: is there a way to check OpenGL version in SConstruct, in order to report warning and disable GL rendering if it is <= 2.0?

Yes. This can be done by compiling and running a toy SDL/OpenGL program that opens a window with corresponding OpenGL context, and then calls glGetString(GL_VERSION). Note that the version returned could differ based on whether a core profile or compatibility profile is requested, but that will be a problem if and/or when we start using OpenGL 3.

[stephanemagnenat]

We are almost there. Actually I realized that non-OpenGL support was already broken, as "OpenGL" and "GLU" are added to missing, which is wrong. I believe that the right thing to do is:

• Do not add these to missing.
• Move the search for epoxy to line 191 and add it to gl_libraries.
• In line 192, check that gl_libraries has size 3 (if not Darwin) or size 1 (if Darwin).

Do you agree? We could also merge your PR and fix the GL detection stuff afterwards.

[stephanemagnenat]

I have decided to go on and merge!

[stephanemagnenat]

On my system, this PR didn't build due to a missing memory header, I fixed it in 7dea3ab.

[stephanemagnenat]

I (believe I) resurrected the non-OpenGL building in 21c07a5.