VideoCommon: Improve FPS/VPS Counting with Euler + Moving Averages and Colors #11212
Conversation
|
An optional request, but could we have a checkbox for just the data points? [OFF] [ON] And each column will appear/disappear respective to your other check boxes you added (Show FPS, Show VPS etc...)
|
I have very little experience adding / removing options so I think it would be best as a follow up PR.
It is based on VPS. |
|
Here is a video demonstrating the new FPS counter. New.FPS.Counter.mp4 |
|
This looks really, really nice. |
|
In the future because frame times are being stored, it is possible to get a min/max frame rate range if we were to really start testing frame pacing, but this should be good for now. |
|
I strongly disagree with placement of the options for this. They should not be in Graphics > General. This is a spot where we need less bloat, not more. |
I don't think I agree. These usually are not touched by most users, Enhancements on the other hand... I could agree having lots of items. I think it fits here. Putting it on advanced (which has more elements) makes less sense to me imo. |
|
OLD: PROPOSED: I put it there because, for most of dolphin's history, that is where the FPS counter was, and so any user who wishes to enable / disable it is going to look there first. I think if it is moved, it should be done in a different PR that reorganizes the settings. Also, people turn the FPS counter on and off a lot to test different video settings for performance, so putting it in advanced would just be extra clicks for everybody. |
|
The checkmarks in Graphics > General are not touched because users see a wall of tiny buttons in a prominent spot and ignore them because of the sudden density shift. This is a problem, one I've been moving to solve. This is actively making it worse. Advanced doesn't have this problem because it is Advanced - it is the last tab in the list and thus much lower real estate, and it's already a high density area where users are expected to properly look through it and understand the things there. It is ok to put options typical users will ignore there. Graphics > General on the other hand is precious premium real estate. It's literally one button click in the GUI away. Extreme care should be made regarding it. Nothing there should be ignorable. And historical reasons are not a good excuse for making things worse. Admittedly, I'm more annoyed by this than most since I'm the one that's going to have to clean this up. However, I do not want it to get worse before it gets better. Honestly, I'd prefer there just being no additional GUI options at all and just change "Show FPS" to "Show Performance Stats" and then have controls in INIs. That would resolve my problem fairly easily. (while you are at it you could also add a colour toggle and/or controls to the INIs, which would be nice. I won't block for that though) As I've said previously the Euler change is awesome, and I really want that to be in Dolphin. To the point where if your PR didn't make it I was thinking of adding that bit myself! It's the UX and visual bits that you added I have problems with, which is why I asked you to break this up into multiple PRs for easier managing and merging. You have addressed much of my concerns since, though there's a lot that could be made better. The GUI bits are the only ones I'll put my foot down for though. |
|
About to push a commit that changes the config to look like: |
Define "ignorable" because I really can't follow your argument at all, it's not he same problem as finding the option which is a bigger problem. But these settings do fit in the "General" category semantically; placing them in Advanced makes them harder to find at no benefit to the user. Users that "ignore" settings aren't likely to want to change the defaults anyway (otherwise it wouldn't be an issue to take the time to read them despite the density)
Why would you be against having option in the UI? INI settings are supposed to apply on a per game basis, but users will want this disabled or enabled regardless of what game they are playing My suggestion is: place these option on the "General" tab but under a new header called "Performance Indicators" (above "Other"). While you are at it, "Shader Compilation" should come after "Basic", so that "Other" is last.
|
It's a UX theory thing. We want the settings that are most important for users to configure to be front and center so they actually see and use them. Then, as importance lessons, we cascade down into different tabs, windows, etc as required. This allows users to dip in and change things quickly without having to scan through a bazillion options, while still giving them options for the times when they want to dive deeper. By having low priority options mixed with high priority ones, users are going to blank out the low priority options to keep themselves from being overloaded and distracted. That's bad, we do not want users getting into a habit of that. We want users to engage with our options, and if we're teaching them to ignore settings we are reducing our ability to guide users through the interface. It undermines the entire GUI. It's hard enough to guide users through an interface. This is something that needs attention, and I'm going to tackle it throughout our GUI in the coming months. |
|
I’m not even sure this is needed. I see it as redundant with the title bar displaying the same information. I’m with MayImilae about the UX. It all becomes noise and contributes to more confusion and support being burdened. Piling on options turns to debt so it should be worth it, and I don’t see this being worth it IMO. |
|
|
|
I'd definitely recommend leaving the frame time counter for a follow up PR... |
|
Are you sure? I really want to just complete this performance tool so that other developers / users will be better able to gauge performance and frame pacing without sorting through the render times themselves. |
|
I really like the color changes. Tracking numbers changes each frame is difficult. Seeing color shifts is easy since it works on another aspect (or broader indicator) on performance. What I don’t agree with is having a checkbox breaking down each piece of what’s being displayed. IMO, it should be one checkbox to toggle FPS only. While displaying the frame time could be useful, if it doesn’t hold for a certain window of time, it’ll come and go without notice. I think it’d be a nice addition and the single checkbox should toggle it and the fps. |
|
Obviously I'm not your boss and it's up to you how you want to proceed, but doing work in multiple smaller PRs has a lot of advantages. It's way easier to review smaller PRs and get them merged, so you could be getting features into users hands way faster. And "older" features in a PR can be merged into Dolphin earlier so that users can benefit from them without having to wait for other cool features to be finished. And of course, nothing is stopping you from changing things you've pushed into master already in subsequent PRs. I am mainly saying this because I really like the Eular change and once we get sign off from our Android maintainers, I was going to push to get this PR merged. If you keep broadening your scope for this specific PR then getting that change into Dolphin will continue to be put off. Still, that's just my feelings though. |
| GFX_SHOW_VPS(Settings.FILE_GFX, Settings.SECTION_DEBUG, "ShowVPS", false), | ||
| GFX_SHOW_SPEED(Settings.FILE_GFX, Settings.SECTION_DEBUG, "ShowSpeed", false), | ||
| GFX_SHOW_FRAME_TIMES(Settings.FILE_GFX, Settings.SECTION_DEBUG, "ShowFrame_times", false), | ||
| GFX_SHOW_SPEED_COLORS(Settings.FILE_GFX, Settings.SECTION_DEBUG, "ShowSpeedColors", true), |
There was a problem hiding this comment.
These all should be in Settings.SECTION_GFX_SETTINGS and the frame time setting key needs to be changed to "ShowFrameTimes"
| sl.add(new CheckBoxSetting(mContext, BooleanSetting.GFX_SHOW_SPEED, R.string.show_speed, | ||
| R.string.show_speed_description)); | ||
| sl.add(new CheckBoxSetting(mContext, BooleanSetting.GFX_SHOW_FRAME_TIMES, R.string.show_frame_times, | ||
| R.string.show_frame_times_description)); |
There was a problem hiding this comment.
You forgot to change the string R.string.show_frame_times to R.string.show_speed_frame_times
R.string.show_frame_times_description also needs to be R.string.show_speed_frame_times_description
Maybe remove "speed_" in the string name?
Ah I see, I'm still new to contributing to FOSS, but I think this frame time thing is the last feature I wanted to add and since it got reviewed by an android maintainer I think i might just finish it and push. |
Just to clarify, I hardly have final say on this. I just wanted to make sure your Android code is functional. If other maintainers think it's a better idea to split things, it might be a better idea to take their advice. |
|
|
First of all, you can't see the titlebar in fullscreen, second, the titlebar isn't visible with the built in screenshots, third, jamming all this info in the titlebar is not pretty and can be hard to read. |
Let me ask, who does this serve? Why do you need to see the VPS while in full screen? Is the fps not good enough in determining if a game is running full speed or not? This is an honest question. What is the point in it? The “noise” I mentioned is referring to the settings. You will have 3 checkboxes on top of the countless other options crowding the window. This is not about anyone’s intelligence. Ask a random doctor to setup Dolphin. Do you think they’d know what these options are? It’s outside their domain of knowledge so they’d be ignorant to it. It doesn’t make them stupid but you certainly are wasting their time and causing confusion. I’m looking at it from a usability perspective from someone who isn’t neck deep in how Dolphin works. If that’s not a concern, then that’s fine. Everyone in here knows what the options do and maybe that’s all that matters. Different projects have a different focus. You should hang out in the Discord support though. It’s not uncommon for people to select options not knowing what it does. People just want it to run well but given too many choices, mistakes will be made. BTW, the color coding is a great idea but I think it should have stopped there. |
|
Sometimes I like seeing the FPS/VPS/speed% in full screen when testing. A lot of games on Wii require complex motion controls, so using full screen helps me see the game if I have to stay far away from my computer screen. I'm getting old, I have bad eyes. Seeing the color change makes it way easier to see it's lagging from far away, and allows me to differentiate ingame lag from dolphin lag. |
My personal iOS fork and Android (as far as I'm aware) have no title bar, so there is no way to see VPS and the current speed on these platforms. This is a great replacement. (To be honest: the title bar shouldn't even contain this information in the first place, imo.) |
|
Those are fair points. I just wasn’t sure with all the direction changes this was taking. I’m making it sound like a big deal but it really doesn’t bother me and I know I don’t hold much sway in here. So, I’m sorry about that. Don’t mean to derail. Those checkboxes though. I don’t think has to be split up like that. IMO, it should be one to toggle for all of it. Just my 2¢. |
I agree. Instead of just a FPS counter, improve the design so it’s obvious where the slow down is coming from, emulator slowdown (VPS) vs console slowdown (FPS). The title bar is limited in how you can convey that information. |
|
I think that's a fair thing to add, plus it'd make it easier for OBS to detect Dolphin if we're not changing the title bar all the time. |
Users like to experiment, "if I do this and that will the performance suffer or improve?", these indicators allow these experimentations and have an objective measure.
Why would having less options help though? They would still be as ignorant just not able to mess with some stuff but under that logic, no consequential setting should be in the UI (way more consequential stuff, like the Shader Compilation is not up debate for exclusion). People that are ignorant about Dolphin in general, would not feel as inclined to mess with the defaults, unless they were sure the setting won't make a difference on gameplay.
I mean, they could very clearly see the effect of the options regardless of what game they are playing. It's not like enabling MMU or the like, this doesn't affect the playability or performance in any meaningful way.
That's bound to happen no matter how you present the options though, with more users, you will get more bogus reports. If the argument is that moving these settings to advanced will reduce these reports, then my answer is no. The worst case is that somebody enables the indicators but can't figure out how to disable them. Having them under a new header makes this trivial "just uncheck everything under "Performance Indicators"" |
|
Just so we're clear, I don't think putting all these controls into Advanced is the -final- solution. It's just a good solution for now, keeping our user UX up while still allowing for sam-belliveou to add new features. There's a lot of great new features to be had here, and taking off the burden of being in General has already opened this up with lots of cool new features. Just remember that we don't have to perfect UX on the first try. Care needs to be taken of course, but iteration is ok! I'm going to go over the whole GUI in a big cleanup in a couple months. Any immediate UX problems are being avoided by having it in Advanced, so let's see what happens. I'm in particular curious whether or not users will ask for a global show performance statistics toggle, ala how "Show FPS" worked. It's something I've been debating, and having in-the-wild feedback will be very useful.
That is totally something you can do! But splitting it up retroactively could cause you to lose momentum, I don't know how you code so I don't know how that would affect you. Do what you feel is best? |
|
I do think that all the features of the PR are complete, however I could split up the PR before / after the frame time additions. The only issue is that at the moment I have school work and it might be awhile until I have the time to do the git trickery to split up the PR, especially since I added a few fixes noticed t895 after I added frame times. I think there are a few things I want to add to this performance window that I will do in a separate PR. Hopefully I can make Personally if it were up to me I would like to have everything here just reviewed and merged so I could move onto other things, but I understand that adding the frame time data threw a wrench into things. I'd like to be able to use the frame time standard deviation to test frame rate inconsistencies in new pull requests, but it is possible there are a few things to be ironed out with it. (like using the FPS or VPS for frame times or how to order the items in the stats menu. |
|
I have reverted the "Frame Time" commit and kept all the fixes to the android side of the PR. Sorry for the really long delay, I had kinda built up a bit of college work making this PR, and I was a little bit scared about reverting the commit without breaking something else. I apologize for being a little gung ho about my PR, I realize I just delayed this for everybody and I'll try to be a bit better about it in future commits. If you need me to do anything else I'd be more than happy to. Here are some screenshots of the important bits to show that it still works.
|
LGTM |
There was a problem hiding this comment.
Design LGTM. This is unlikely to be the final UX for this feature, but for now the UX is pretty good and it gives samb some leeway to expand this feature further. Functionality wise, the Euler change is awesome, and the addition of colours and the ability to customize the millisecond range are all great functionality improvements.
Also I'm really curious if users will comment about the lack of "Show FPS" (cause it's moved). I really want to hear user feedback on it to help direct future UX changes.
AdmiralCurtiss
force-pushed
the
realtime-fps-and-vps-counter
branch
from
3a6e81a
to
edb2c90
Compare
|
tbqh I'm not quite sure how much of an improvement this actually is in terms of accurate numbers but a few people have approved this so sure, whatever. |
|
If |
Can confirm. For some games like GUPE8P / Shadow The Hedgehog cutscenes are 30FPS and gameplay is 60FPS. If |
|
I tested it with skip presenting XFB but not with Immediately Present XFP, I'll see if I can fix this in a PR, but as it is finals week I don't know if I'll be able to resolve this until exams are over. |
Fixed in #11326 |
|
@Sam-Belliveau I'm working on the Dolphin Progress Report, and I'm having some difficulty with the Euler aspect of this change. I can see that it's better visually and I can read the code, but the Euler change is just all math and it's flying over my head. Can you explain to me what the Euler method is and why it makes a better FPS display? |
Sorry I didn't see this, the math looks very complex, but its actually a lot simpler than it looks. But imagine that you have a series of FPS values. like 0, 30, 30, 30, 30, 30, 30... When filtered by a euler average the sequence of values would look more like: 0, 15, 22.5, 26.25, 28.125, 29.0625... Basically imagine it taking the average of the previous value and the new one. However this weighting is not great, because its hard to tell how fast it is, and if you get FPS values faster, it will change value. So if you calculate a weight, call it When this is combined on top of a moving average, it ends up smoothing out the data and giving you a "summary" of the fps over the last second, and that summary is continous as it update. Here is a visual of a similar euler + moving average filter when applied to noisy data: You can see that despite the noisy input data, it is able to give you a straight line, because the trend is straight. So the TLDR, instead of calculating a new FPS value every 0.25 seconds, it does a lot of math to give you the fps value over the past x second, every single frame, so your frame rate is continuous. |
|
@Sam-Belliveau I have a few more question and would like a review, so could you please come to the IRC sometime? Ping me whenever you are around. |
Cleaned Up PR for: #10988
This FPS Counter has 4 new features:
The FPS is calculated using a 1 second Moving Average AND a Euler Average (0.25 RC Single IIR LowPassFilter) to smooth out the FPS Readings. Not only does this make the FPS counter more readable, it improves its accuracy, allowing the user to see if they are hitting a consistent 59.94fps and if their frame pacing is correct.
The FPS is now color coded, allowing users to tell, at a glance, the performance of their game without having to stare at the number.
VPS / %Speed have been added to the performance stats allowing the user to track more that just FPS.
Frame Time / Standard Deviation. This allows you to get a quick idea of how good the frame pacing is without staring at the FPS counter.
Here is the FPS Counter Running at Different Speeds:
Settings:
Frame Times:
