Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up terrain drawing with glDrawArrays from OpenGL 2.0. #120

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

Conversation

Quipyowert2
Copy link
Contributor

@Quipyowert2 Quipyowert2 commented May 28, 2024

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:

@stephanemagnenat
Copy link
Contributor

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
Copy link
Contributor Author

Quipyowert2 commented Jun 5, 2024

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
Copy link
Contributor Author

Quipyowert2 commented Jun 7, 2024

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
Copy link
Contributor Author

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

@Quipyowert2 Quipyowert2 marked this pull request as ready for review June 8, 2024 20:44
@Quipyowert2
Copy link
Contributor Author

Quipyowert2 commented Jul 30, 2024

@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
Copy link
Contributor Author

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

@stephanemagnenat
Copy link
Contributor

stephanemagnenat commented Oct 8, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor

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

Buildings are drawn in three places now for some odd reason.

Fix building showing up three times bug.
Doesn't compile yet because of a purportedly missing > when compiling native.h.
Creating and deleting two std::vectors for every sprite draw during
every frame took a total of 7s over a profiling run of about 3 minutes.

Fix null pointer deref if no sprite or atlas.

Fix terrain drawing in-game.

Fix nothing to draw causing crash in finishDrawingSprite.
@Quipyowert2
Copy link
Contributor Author

Quipyowert2 commented Oct 11, 2024

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

Fix water not being drawn.

Fix flags, zones, and building rendering.

Fix fog of war; missing icons when placing building.

Fix brush selection not showing up in flag view.

Draw zone animations.

Fix bullets not showing up.
Link against Epoxy.
If all the images in the array are NULL, we don't need to create a
texture atlas/sprite sheet.
Changes Sprite::atlas variable to be a unique_ptr and adds a bunch of
preprocessor macros to find the right make_unique for the C++ and Boost
version.
}
int tileWidth = images[0]->getW();
int tileHeight = images[0]->getH();
int sheetWidth = tileWidth * (static_cast<int>(sqrt(numImages)) + 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should probably use a loop that tries all the possible width and height combinations here where width and height are <= the maximum texture size (glGetIntegerv(GL_MAX_TEXTURE_SIZE)). May need multiple sprite sheets if the maximum texture size is too small.

@Quipyowert2
Copy link
Contributor Author

Quipyowert2 commented Nov 6, 2024

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?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants