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

Simplify rendering pipeline, reduce gpu memory usage, remove duplicated data #86

Merged
merged 1 commit into from
May 27, 2024

Conversation

PPakalns
Copy link
Contributor

No description provided.

@grovesNL
Copy link
Owner

Thank you! I'd like to do some performance testing (especially WebGL) before merging this though. As far as I remember instancing small vertices used to be slower on older GPUs which is why I didn't do it this way from the start.

@PPakalns
Copy link
Contributor Author

Didn't know that, learned something new :)

@hecrj
Copy link
Contributor

hecrj commented Mar 30, 2024

I tested these changes in a relatively big app and I see a noticeable speedup on my M1 (from ~2ms frame time to ~1.7ms). I guess it's expected since the prepare loop does quite less. GPU usage seems unaffected.

src/text_render.rs Outdated Show resolved Hide resolved
@grovesNL grovesNL force-pushed the improve-pipeline branch from 9f0391d to fd9e38b Compare May 27, 2024 01:17
@grovesNL
Copy link
Owner

I just rebased this onto main. Let's go ahead with this for the CPU time improvements.

It's possible it could regress GPU time some specific older hardware that doesn't handle small instances well, but most modern hardware should be ok with this based on what I can tell. e.g., it's confirmed that AMD has optimizations for small instances now instead of doing the slow thing that used to happen.

If we see a noticeable performance problem with any hardware then we can always special case certain vendors and expand the vertices out in those cases.

@grovesNL grovesNL merged commit 0fdbd90 into grovesNL:main May 27, 2024
1 check passed
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.

3 participants