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

Add foreground FPS limit #16

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

Conversation

mweber15
Copy link
Contributor

As reported in issue #13, PoB tends to use significant CPU and GPU resources
while in the foreground. This change exposes a foreground frame rate limit that
can be configured via a CVar (vid_fgfps) that defaults to a 30 FPS limit and
allows a 5-120 FPS range. A setting of 30 feels responsive and reduces the
resource usage by a modest amount. CPU usage hovers around 4% on my 16-core
system with this change, as opposed to 6% usage with the current build.
Similarly, GPU usage hovers around 18-20% with the 30 FPS limit and around
25-28% with the current build.

There may be a better place to situate the sleep that is introduced for this,
but this seemed like a reasonable place for someone who is not very familiar
with this code.

I also removed the reference to tiff.lib in the Debug configuration since that
is unused and not a vcpkg dependency.

As reported in issue PathOfBuildingCommunity#13, PoB tends to use significant CPU and GPU resources
while in the foreground. This change exposes a foreground frame rate limit that
can be configured via a CVar (vid_fgfps) that defaults to a 30 FPS limit and
allows a 5-120 FPS range. A setting of 30 feels responsive and reduces the
resource usage by a modest amount. CPU usage hovers around 4% on my 16-core
system with this change, as opposed to 6% usage with the current build.
Similarly, GPU usage hovers around 18-20% with the 30 FPS limit and around
25-28% with the current build.

There may be a better place to situate the sleep that is introduced for this,
but this seemed like a reasonable place for someone who is not very familiar
with this code.

I also removed the reference to tiff.lib in the Debug configuration since that
is unused and not a vcpkg dependency.
@Wires77
Copy link
Member

Wires77 commented Mar 16, 2021

Tested this initially thinking it was going to have an impact on the speed of our various sorting subroutines, but it seems they either already block the UI until the calculation is done, or they're put on hold when the new frame is called, but keep running otherwise, so the difference was negligible.

On my machine I saw 60 FPS on the old code (using Steam for a quick-and-dirty FPS counter), and 20 FPS with this new code.
CPU usage dropped from ~11% on the tree tab to ~4%. Looks pretty good to me!

@dclamage if this works for you do you think we might be able to remove or change the code that eliminates processing when PoB is in the background? We could severely limit FPS when in the background, but allow sorting to continue processing, for example.

@dclamage
Copy link
Contributor

Thanks for contributing!

Would you be able to make the following changes?

  1. Removing tiff.lib from the linker options is fine, but please make this a separate PR.

  2. Please combine your framerate limiter with the existing one in ui_main.cpp at the start of ui_main_c::Frame(). Even better would be to add a foreground and background framerate option, and use the existing foreground/background detection in that code to determine which limiter to use. Currently this code exits early from the frame when in the background. I think we can experiment with just severely limiting the framerate instead.

Thanks!

@mweber15
Copy link
Contributor Author

I'm not quite sure my interpretation of the bg/fg detection is correct or optimal, but the performance seems to be in line with what I would expect for both scenarios.

@Wires77 Wires77 mentioned this pull request Aug 8, 2021
1 task
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