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

Create render-to-texture renderpool to render terrain in paralell #1671

Merged
merged 23 commits into from
Oct 3, 2022

Conversation

prozessor13
Copy link
Collaborator

@prozessor13 prozessor13 commented Sep 22, 2022

This branch creates a Render-To-Texture render-pool, instead of caching textures in RTT Tiles.

Old logic:
The Terrain class had one framebuffer, and a list of tiles which contained a list of textures. This had two problems:

  • for complicated stylesheets a lot of textures were created, for mobile-phones too much, and they crashed
  • render the textures with only one framebuffer was slow, because serial rendering one by one

New logic:
There exist a Framebuffer/Texture pool that would be reused during rendering, and the tiles only contains indexes to this pool-objects.

  • Now there is a single place to define the amount of created framebuffer/textures. Currently the pool contains 30 objects, not tested if this is too much for mobile phones
  • because of 30 paralell framebuffer objects, and no need of heavily switching textures during rendering the rendering is much faster.

Also this PR makes my other PR #1301 obsolete.

Need help with:

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2022

Bundle size report:

Size Change: +828 B
Total Size Before: 204 kB
Total Size After: 205 kB

Output file Before After Change
maplibre-gl.js 195 kB 196 kB +828 B
maplibre-gl.css 9.09 kB 9.09 kB 0 B
ℹ️ View Details No major changes

@HarelM
Copy link
Member

HarelM commented Sep 25, 2022

I've fixes most of the tests.
I've left one failing related to the changes in render to texture which I wasn't sure how to fix.
Let me know if you need more help in writing or or fixing the tests.

@prozessor13
Copy link
Collaborator Author

Hello Harel,

thank you for fixing the unit-tests. I will do the last missing tests. Additionally i can create tests with different stylesheets and check if the merging of layers works correct in render_to_texture.ts.

It would be great if someone else can:

  • Create a render-test, may the existing one are sufficient?
  • Test performance, e.g run, and my add, a benchmark test.

@HarelM
Copy link
Member

HarelM commented Sep 29, 2022

I think the render test that will be added as part of #1666 will be enough for this as well.
Once this is merged I can create a pre-release to test the performance in my app.
Did you try it with the iPhone 6 device you have?

@prozessor13
Copy link
Collaborator Author

Yes, tried on my iphone, and works much more performant without a crash.

src/ui/map.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Member

HarelM commented Sep 30, 2022

Overall this looks good.
I have added some last comments.
Let me know if you need help working on either of them.

@prozessor13
Copy link
Collaborator Author

@HarelM thanks for review! Are there more tests needed for testing performance? I am sure this branch increases performance, but i have no idea how to create a performance-test. Help with this would be great!

@HarelM
Copy link
Member

HarelM commented Oct 1, 2022

I've made the changes myself to reduce cycle time.
A few notes related to the pool class:

  1. There's no actual check for the size of the pool when creating new objects so basically the pool can be bigger than the size you initialize it with.
  2. The recently used is never bigger than objects array so there's a check that is always false and code that never gets executed.

I found out about both issues when I ran the tests and looked at the code coverage which is a great things when you create new classes that should be fully tested. :-)
The line I used is:
npx jest ./src/gl/render_pool.test.ts --coverage
and check the line coverage of render_pool under gl.

This is not holding this PR from my point of view and can be fixed in a later PR.

@prozessor13
Copy link
Collaborator Author

@HarelM thanks for your changes, and interesting result from the coverage tool! Very helpful! This line is from the old, not working logic. Missed to think about this line, and delete it.
I can fix the issues tomorrow, if PR is already merged i will create a new PR.

@HarelM
Copy link
Member

HarelM commented Oct 2, 2022

This can wait another day, I prefer to merge it with the relevant fix and not create a new PR for this.
Thanks a lot for all the hard work! Truly appreciated! :-)

@HarelM
Copy link
Member

HarelM commented Oct 3, 2022

render_pool.ts | 100 | 100 | 100 | 100 Nice! 100% coverage!

@HarelM HarelM merged commit 67cdbcb into main Oct 3, 2022
@HarelM HarelM deleted the paralell_rtt2 branch October 3, 2022 07:47
@klokan klokan added this to the 3.0.0 milestone Dec 10, 2022
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.

None yet

3 participants