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

Jump to bottom

FPSCounter: Add "Log render time to file" feature. #591

Merged
merged 3 commits into from Jul 9, 2014
Merged

FPSCounter: Add "Log render time to file" feature. #591

merged 3 commits into from Jul 9, 2014

Conversation

CrossVR
Copy link
Contributor

@CrossVR CrossVR commented Jul 9, 2014

Allows for a more accurate performance measurement than the FPS log feature.

degasus
degasus reviewed Jul 9, 2014
View reviewed changes
Source/Core/VideoCommon/FPSCounter.cpp
@@ -17,20 +17,27 @@ namespace FPSCounter
static unsigned int s_counter = 0;
static unsigned int s_fps = 0;
static unsigned int s_fps_last_counter = 0;
static unsigned long s_last_update_time = 0;
Common::Timer s_update_time;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@degasus
Copy link
Member

degasus commented Jul 9, 2014

imo this feature is better than LogFPSToFile, but I don't think we need both. Lots of guys are used to FPS, but ms per frame is the easier one to calculate.

I also like to have this dumped for every frame, so we'll likely see the impact of texture decoding and/or shader generator better.

@CrossVR
Copy link
Contributor Author

CrossVR commented Jul 9, 2014

In this PR it is already dumped for every frame.

@delroth
Copy link
Member

delroth commented Jul 9, 2014

Agreed, we don't need both, and render time is objectively better.

@CrossVR
Copy link
Contributor Author

CrossVR commented Jul 9, 2014

I will replace it, should I account for the value of LogFPSToFile already saved in the config?

@delroth
Copy link
Member

delroth commented Jul 9, 2014

I would change the setting name, this option is not important enough to
care about backwards compat.

On Wed, Jul 9, 2014 at 5:43 PM, Jules Blok notifications@github.com wrote:

I will replace it, should I account for the value of LogFPSToFile already
saved in the config?

—
Reply to this email directly or view it on GitHub
#591 (comment).

Pierre "delroth" Bourdon delroth@gmail.com
Software Engineer @ Zürich, Switzerland
http://code.delroth.net/

@CrossVR
Copy link
Contributor Author

CrossVR commented Jul 9, 2014

I've made the necessary changes based on the feedback.

delroth
delroth reviewed Jul 9, 2014
View reviewed changes
Source/Core/VideoCommon/FPSCounter.cpp
@@ -17,36 +17,44 @@ namespace FPSCounter
static unsigned int s_counter = 0;
static unsigned int s_fps = 0;
static unsigned int s_fps_last_counter = 0;
static unsigned long s_last_update_time = 0;
static Common::Timer s_update_time;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@delroth
Copy link
Member

delroth commented Jul 9, 2014

@Armada651: This comment grants you the permission to merge this pull request whenever you think it is ready. After addressing the remaining comments, click this link to merge.

@dolphin-emu-bot allowmerge

dolphin-emu-bot added a commit that referenced this pull request Jul 9, 2014
@dolphin-emu-bot
Merge pull request #591 from Armada651/pref_log
7b754d6 
FPSCounter: Add "Log render time to file" feature.
@dolphin-emu-bot dolphin-emu-bot merged commit 7b754d6 into dolphin-emu:master Jul 9, 2014
neobrain
neobrain reviewed Jul 9, 2014
View reviewed changes
Source/Core/DolphinWX/VideoConfigDiag.cpp
@@ -322,7 +322,7 @@ VideoConfigDiag::VideoConfigDiag(wxWindow* parent, const std::string &title, con
{
SettingCheckBox* render_to_main_cb;
szr_other->Add(CreateCheckBox(page_general, _("Show FPS"), wxGetTranslation(show_fps_desc), vconfig.bShowFPS));
szr_other->Add(CreateCheckBox(page_general, _("Log FPS to file"), wxGetTranslation(log_fps_to_file_desc), vconfig.bLogFPSToFile));
szr_other->Add(CreateCheckBox(page_general, _("Log render time to file"), wxGetTranslation(log_render_time_to_file_desc), vconfig.bLogRenderTimeToFile));

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
5 participants
@CrossVR @degasus @delroth @neobrain @dolphin-emu-bot