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 Frame Time Recording #4636

Closed
wants to merge 7 commits into from

Conversation

BreadFish64
Copy link
Contributor

@BreadFish64 BreadFish64 commented Feb 11, 2019

Frame times are recorded until the game stops or the user unchecks this option:
image
Then the data is dumped to a .csv file in the log directory and can be used to make fancy graphs like this:
image


This change is Reviewable

src/core/core.cpp Outdated Show resolved Hide resolved
@@ -175,6 +175,7 @@ struct Values {
// Debugging
bool use_gdbstub;
u16 gdbstub_port;
std::atomic_bool record_frame_times;
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this shouldn't be a part of the settings; I think it's more suitable for being a temporary state in PerfStats.

Copy link
Contributor Author

@BreadFish64 BreadFish64 Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like not saving it between runs? I don't see the advantage to that

Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying you shouldn't save it (though I doubt the necessity of this), but even if you do, you shouldn't use the settings as a state of PerfStats. (and if you look carefully no other fields here are atomic)

Instead make this just a bool and add an atomic_bool to PerfStats, and initialize that atomic bool from the settings on load and save it on exit.

src/core/perf_stats.cpp Outdated Show resolved Hide resolved
src/citra_qt/main.ui Outdated Show resolved Hide resolved
src/citra_qt/main.ui Outdated Show resolved Hide resolved
src/core/perf_stats.cpp Outdated Show resolved Hide resolved
@BreadFish64
Copy link
Contributor Author

BreadFish64 commented Feb 12, 2019

review comments addressed and bug squashed

@BreadFish64
Copy link
Contributor Author

BreadFish64 commented Feb 12, 2019

refactored to not use unique_ptr

lower deviation due to no reallocations
@JayFoxRox
Copy link
Member

JayFoxRox commented Feb 27, 2019

I'm currently strongly against this. It appears totally unnecessary to me.
The feature is also very underdocumented: how would I know the output will land in the log, just from that menu option?

Profiling will be done differently, using more powerful tools.
Apparently microprofile is also still part of Citra, so I assume this functionality exists already (to some extend).

There's just a ton of unanswered questions, especially considering this PR has been here for 14 days already:

  • Why would anyone use this feature? Fancy graphs for... what?
  • Why can't it be solved using the existing tools like microprofile?
  • Why does it have to be solved in Citra, not a third-party solution which most people will have to use anyway?
  • Why is this considered a "Tool" and not specifically marked as a debug feature?
  • ...

@BreadFish64
Copy link
Contributor Author

BreadFish64 commented Feb 27, 2019

There's just a ton of unanswered questions

Nobody asked any, now that there's questions I can answer them.

Documentation

Right now I have the tooltip set to tell the user to look in the log directory. I wanted to avoid popups. If you have a better suggestion I would be open to it.

Why can't it be solved using the existing tools like microprofile?

I wasn't aware that microprofile had this capability because it's poorly documented in the application; searching "microprofile" on google gave irrelevant results and nobody told me for the 14 days this was open. I don't see an option to actually save the data microprofile outputs but maybe that would be a better approach.

I also find it more convenient to start recording frametimes when the game starts, because I was using it with our input recording feature to get reproducible results.

Why does it have to be solved in Citra, not a third-party solution which most people will have to use anyway?

Most third party applications used for measuring frame data such as MSI afterburner don't work properly in conjunction with Citra as far as I can remember.

Why is this considered a "Tool" and not specifically marked as a debug feature?

Because I saw reasons for non-developers to use it. It was mentioned on the yuzu discord that it might be useful for youtubers to get hard performance data. (A single checkbox is also much less intimidating to these users than a separate UI full of seemingly redundant options, some of which have a non-trivial performance impact if enabled accidentally.)

...

If you have other feedback then you can say it.

@GXTX
Copy link

GXTX commented Feb 27, 2019

If the feature is only going to be used in very specific instances (YouTubers gathering hard performance data), shouldn't this be a debug only feature? Or is this something that needs to be mainlined in Citra instead of pointing the user in the right direction with 3rd party tools specifically created for these tasks?

Seems to be like this is just feature creep in action.

@BreadFish64
Copy link
Contributor Author

BreadFish64 commented Feb 27, 2019

I gave one possible use for it, personally I was using it to compare performance data with different compiler optimizations in Excel.

IMO feature creep isn't a real issue if it doesn't detract from user experience. If you want an example of bad feature creep try using gimp without searching anything on the internet.

@wwylele
Copy link
Member

wwylele commented Mar 14, 2019

I'll put one of my concern expressed on discord here to remind that this not resolved yet:

IMO feature creep isn't a real issue if it doesn't detract from user experience.

Feature creep is more of a dev issue rather than a user issue that might hinder the development from making essential process.

@BreadFish64
Copy link
Contributor Author

BreadFish64 commented Apr 3, 2019

I'm going to close this. It seems like the better approach would be to use microprofile to save the data but I didn't want to spend a lot time on this in the first place.

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

6 participants