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

"Reset" of Internal State for Framerate Calculation #5236

Closed
geometrian opened this issue Apr 23, 2022 · 4 comments
Closed

"Reset" of Internal State for Framerate Calculation #5236

geometrian opened this issue Apr 23, 2022 · 4 comments

Comments

@geometrian
Copy link

geometrian commented Apr 23, 2022

Version/Branch of Dear ImGui:

Version: Dear ImGui 1.88 WIP (18717)
Branch: master


Back-end/Renderer/Compiler/OS

Back-ends: Custom framework involving "opengl2", "opengl3", and "sdl2" backends.
Compiler: MSVC v143
Operating System: Windows 10 Pro 21H2


My Issue/Question:

(TL;DR summary: please expose a way to reset the framerate estimation, or more generally improve the overall calculation)

ImGui provides a nice convenience feature for getting a simple estimate of the framerate:

ImGui::GetIO().Framerate;

This is based on an average of the last 120 frames, and is calculated internally with some ring buffer:

    // Calculate frame-rate for the user, as a purely luxurious feature
    g.FramerateSecPerFrameAccum += g.IO.DeltaTime - g.FramerateSecPerFrame[g.FramerateSecPerFrameIdx];
    g.FramerateSecPerFrame[g.FramerateSecPerFrameIdx] = g.IO.DeltaTime;
    g.FramerateSecPerFrameIdx = (g.FramerateSecPerFrameIdx + 1) % IM_ARRAYSIZE(g.FramerateSecPerFrame);
    g.FramerateSecPerFrameCount = ImMin(g.FramerateSecPerFrameCount + 1, IM_ARRAYSIZE(g.FramerateSecPerFrame));
    g.IO.Framerate = (g.FramerateSecPerFrameAccum > 0.0f) ? (1.0f / (g.FramerateSecPerFrameAccum / (float)g.FramerateSecPerFrameCount)) : FLT_MAX;

It is often useful to explore realtime algorithms by switching between versions that are expected to have different performance (i.e. latency, calculated from framerate). However, after a switch, this framerate readout converges to the new correct average, only achieving it after 120ish frames. It is sometimes unclear, especially with very slow algorithms, whether the reported framerate has stabilized, or whether there are still frames from the previous algorithm lurking in the average.

It would be better to have more variance in the reported number, but not have it include "incorrect" values. That is, there should be a way to reset ImGuiContext::FramerateSecPerFrameCount (and ImGuiContext::FramerateSecPerFrameAccum). There is no built-in way to do this (indeed, ImGuiContext itself is semantically private), and I would like to suggest some command like this be exposed.

Of course, perhaps a more-general way to calculate/expose framerate is appropriate. The framerate calculation, as it is, is quite simple, and perhaps it could be improved in other ways.

Finally, while one can obviously calculate latency themself however they like, and the current framerate calculation is described variously as "purely luxurious", "[s]olely for convenience", and "rough", doing a correct framerate calculation is significantly annoying, and it does seem reasonable that the GUI should do it for us. Moreover, in practice, people seem to just use the existing framerate readout without thinking. I can't tell you how many ImGui-using graphics demos I've seen that suffer from the above issue of switching-leads-to-incorrect-perf—which suggests at the very least that the default behavior should be more controllable (if not removed entirely, to force people to do it "right" themselves).

@ocornut
Copy link
Owner

ocornut commented Apr 23, 2022

Hello,

I appreciate that you took the time to build up a case, but I disagree with your conclusion that implementing one yourself is annoying, considering you literally quoted the 5 lines that do it. There’s no single way to compute what “framerate” means, if you need anything else you can easily implement it on our side. The harm caused by users using this value in the situation you mentioned is merely an inconvenience as it is evident that the value takes times to converge.

Exposing anything more in public api is a maintenance weight for us and larger api surface which are exactly the sort of things we strive to avoid.
What we can do: add an internal inline function that clears those two values, reduce that buffer size from 120 to 60. If we start exposing even a helper ImMovingAverage<120> structure people are going to use that i’m not thrilled with that.

@geometrian
Copy link
Author

Hi ( sorry for text wall :S ),

If I can argue the point some . . . the harm isn't merely that programmers have to calculate better estimates of the framerate themselves. (That's true, and it is a problem—solving these problems in a manner both reasonable and robust is surprisingly nontrivial; as you see, the 5 lines here may be short, but they are deficient—but it's not the main issue IMO.)

It is also not always evident that the value hasn't converged. In fact, in the cases where one cares in the first place about the framerate readout—that is, when the performance difference isn't obvious and you're comparing or reporting it quantitatively—the performance difference (and therefore, convergence from one to the other) is difficult to notice. Additionally, if the frame time is very long (seconds, even minutes), then you don't intuitively know how long you have to wait until 120 frames have elapsed (to say nothing of the extra time spent pointlessly waiting for the old values to clear out . . .).

The main issue (for me) is that people in practice seem to be misleading themselves and others about the actual performance of things. As I mentioned above, I can't tell you how many ImGui-using graphics demos suffer from this problem. I can't tell you how many times I'm seeing some heavy raytracing demo running at 10 fps or whatever and then they switch it to a new, even fancier model that looks nicer, and say it's running at 9 fps, when in fact if you leave it running it gets down to 5 fps. It's a pervasive problem in my field, and it's caused primarily by poor scholarship but also indirectly by this exact piece of code!


All that said, I definitely feel the maintenance weight counterargument. One major reason why I personally have avoided ImGui in the past is that, despite its claim of being "bloat-free", the code is immense and I feel like there's a lot that can be cut. This has got to be even worse on the maintenance side. Adding more framerate features would make that problem worse (regardless of whether other features are(n't) also bloaty).

Is there are way to both have and eat the cake? If adding a better framerate-calculation feature is deemed (by you) to be too bloated (I sortof agree) or out-of-scope (I disagree), is there at least a way to prevent people from misusing it (or at least making it harder?).

Offhand, a possible solution (keeping in the theme of reducing complexity) would be to remove it entirely. We already have ImGui::GetIO().DeltaTime I think, which is sufficient information for the programmer to implement the calculation themselves. Of course, that still leaves the possibility of error, but it at least encourages the programmer to think about such issues.

Alternately, perhaps one could add a new module that plugs into that value and uses best-practices, but keep it optional so it doesn't add unwanted code weight, at least to the user's codebase? I appreciate the convenience of fewer header/source file plug-and-play libraries, but perhaps this gui-relevant-but-also-sortof-gui-adjacent stuff should have been refactored into a separate module in the first place?

@AlexvZyl
Copy link

AlexvZyl commented Apr 26, 2022

One could rename ImGui::GetIO().Framerate to ImGui::GetIO().AverageFramerate to be more explicit, which might make the developer think about the problem as @geometrian suggested.

@ocornut
Copy link
Owner

ocornut commented Jul 1, 2022

One could rename ImGui::GetIO().Framerate to ImGui::GetIO().AverageFramerate to be more explicit, which might make the developer think about the problem as @geometrian suggested.

Not going to break build of thousands of codebase for this.

As a slight mitigation measure I made a small change of changing the moving average from 120 to 60 samples and tweaked comments. If you need to reset it you can use this function:

// Reset data used to compute io.Framerate moving average. Useful for faster convergence in case of drastic changes
void ResetImGuiFramerateMovingAverage()
{
    ImGuiContext& g = *ImGui::GetCurrentContext();
    memset(g.FramerateSecPerFrame, 0, sizeof(g.FramerateSecPerFrame));
    g.FramerateSecPerFrameIdx = g.FramerateSecPerFrameCount = 0;
    g.FramerateSecPerFrameAccum = 0.0f;
}

The problem you highlight can only be fixed by education. We can't be adding and maintaining public API and add copious comments in our examples app for the sake of people writing raytracers or application at very slow framerate and using a convenience function wrongly. It's not even a major problem for them. I agree it's annoying but its not our role here.

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

No branches or pull requests

3 participants