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

Fix GLES2 rendering performance #20077

Closed
wants to merge 1 commit into from

Conversation

dragmz
Copy link
Contributor

@dragmz dragmz commented Jul 9, 2018

Fixes a problem described in #19943 but for GLES2 only. The fix reduces the number of glBufferSubData calls by splitting canvas rendering into two phases:

  1. Prepare polygons data and send the data to GPU
  2. Render the polygons from the GPU buffers

I'm getting 4-10 times performance boost for the project attached to #19943 when run with "--video-driver GLES2" flag.

Performance tests (FPS comparison [after vs before]):

Core i5-650, GTX 950:

  • 480 polygons: 730 vs 330 = 2,21
  • 1920 polygons: 270 vs 92 = 2,93
  • 7680 polygons: 72 vs 25 = 2,88
  • sprites: no change

Core i7-4790K, Intel 4600:

  • 480 polygons: 325 vs 60 = 5,42
  • 1920 polygons: 185 vs 19 = 9,74
  • 7680 polygons: 77 vs 5 = 15,4
  • sprites: no change

Core i3-2310M, Intel 3000:

  • 480 polygons: 60 vs 10 = 6
  • 1920 polygons: 24 vs 3 = 8
  • 7680 polygons: 7 vs 1 = 7
  • font rendering (using text_rendering.zip project attached below): 140 vs 10 = 14
  • sprites: no change

Android (Moto C):

  • font rendering: 57 vs 4 = 14,25

Tasks:

@dragmz dragmz requested a review from karroffel as a code owner July 9, 2018 22:37
@dragmz dragmz changed the title fix GLES2 performance when rendering polygons [WIP] fix GLES2 performance when rendering polygons Jul 9, 2018
@dragmz dragmz force-pushed the gles_poly_perf branch 9 times, most recently from 1bb38f6 to d3bc03d Compare July 10, 2018 23:39
@dragmz dragmz requested a review from leonkrause as a code owner July 10, 2018 23:39
@dragmz dragmz force-pushed the gles_poly_perf branch 9 times, most recently from d8d83dd to 7b6ea72 Compare July 11, 2018 14:13
@dragmz dragmz changed the title [WIP] fix GLES2 performance when rendering polygons [WIP] fix GLES2 rendering performance Jul 11, 2018
@dragmz dragmz force-pushed the gles_poly_perf branch 2 times, most recently from 71d5ce8 to 8b60c73 Compare July 11, 2018 14:29
@dragmz dragmz changed the title [WIP] fix GLES2 rendering performance Fix GLES2 rendering performance Jul 11, 2018
@leonkrause leonkrause removed their request for review July 11, 2018 21:29
@dragmz dragmz force-pushed the gles_poly_perf branch 4 times, most recently from d1b5da4 to 90c1ce3 Compare July 22, 2018 07:05
@dragmz
Copy link
Contributor Author

dragmz commented Jul 22, 2018

Test project (from #19917) with vsync turned off, Add More Text modified to add 50x more text at once, GLES2 video driver set by default and number of draw calls displayed:
text_rendering.zip

@dragmz dragmz force-pushed the gles_poly_perf branch 7 times, most recently from bffd948 to c5f2798 Compare July 24, 2018 10:04
Copy link
Contributor

@karroffel karroffel left a comment

Choose a reason for hiding this comment

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

Looks good generally! I didn't completely check every single memcpy and glXXX command, but the general architecture is pretty good! I think time will tell if there are bugs 😛


Item *material_owner = ci->material_owner ? ci->material_owner : ci;
const uint8_t count = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think most compilers will deal with this just fine, but technically this would create a VLA (which are not part of the C++ standard), so making this static const uint8_t count = 6; would be more "safe", but not priority.

@dragmz
Copy link
Contributor Author

dragmz commented Jul 25, 2018

Closing because the canvas rasterizer is expected to have all or none batching and not a partial batching that's been proposed here. See https://godot.eska.me/irc-logs/devel/2018-07-25.log [16:56:58] - [17:13:35] for details.

@dragmz dragmz closed this Jul 25, 2018
@akien-mga
Copy link
Member

I understand that @reduz wants something even more exhaustive, but what are the drawbacks of the current implementation? It fixes real world issues that should not be disregarded.

@dragmz dragmz deleted the gles_poly_perf branch July 25, 2018 20:54
@groud
Copy link
Member

groud commented Jul 26, 2018

That's quite unexpected.We can't change the whole code all in once.
Too bad you did close this PR.

@dragmz
Copy link
Contributor Author

dragmz commented Jul 26, 2018

@groud I have closed it because I can't reimplement it in a near future to have full batching.

@groud
Copy link
Member

groud commented Jul 26, 2018

I know, but it could have been merged until someone can.

@dragmz
Copy link
Contributor Author

dragmz commented Jul 26, 2018

@groud I can't (and don't want to) force @reduz to merge it 😄I'm sure Godot will get some proper 2D batching one day 🤞

@PtrMan
Copy link
Contributor

PtrMan commented Apr 30, 2019

The code doesn't seem to be hard to understand (hey it's just rendering code) but it still could be split into a few commits

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

Successfully merging this pull request may close these issues.

6 participants