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

citra-qt: Add customizable speed limit target #3353

Merged
merged 9 commits into from Jan 26, 2018

Conversation

Projects
None yet
7 participants
@jroweboy
Copy link
Member

commented Jan 8, 2018

Allows the user to customize the speed limit instead of a binary off/on frame limiter.

Things to know:

  • Adjusts the max frametime lag to 250ms instead to allow lower frame limits. If the max lag is too small, the clamp will prevent limiting to smaller values. In practice: at the original 25ms value, the limiter could clamp around values of 60%, so half speed play wasn't possible. 250ms was selected as it allows clamps around 6%. This can be changed if someone thinks that limiting to such values would be pointless.
  • Adds a hotkey (+ / - by default) to adjust the speed limit by +- 5%
  • Status bar changed to display the limit
    Status bar changed to display the limit
  • Using a spinbox lets the user choose a precise limit as its adjustable by +- 1%
    Using a spinbox lets the user choose a precise limit

This change is Reviewable

@neobrain

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-01-08T17:29:52Z: a66e458...jroweboy:0148ea9cdee281a0f378ba0defb96b6edbcc297c

@neobrain

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-01-08T17:39:18Z: a66e458...jroweboy:a6fbd8c56900a843163154ecac772d535d428f89

@neobrain

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-01-08T18:16:05Z: a66e458...jroweboy:8379072658d673902413ca86063a22cd310ee3ff

@ghost
Copy link

left a comment

Update SDL default ini

@neobrain

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-01-08T18:26:48Z: a66e458...jroweboy:22be430238ae98ac671eda51a8a1bf4fac903c83

<property name="text">
<string>Limit framerate</string>
</property>
</widget>

This comment has been minimized.

Copy link
@Dragios

Dragios Jan 9, 2018

Contributor

The checkbox for limit framerate should remain. What if I wanna do benchmark and make speed test comparison. With the current setting it is not possible to do as I can't make it run at the highest speed possible since it is user defined.

@jroweboy

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

@wwylele

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

@jroweboy in most UI practice, I see a check box + a number box(disabled if unckecked) for this case. This is way easier than to document a strange rule and potentially have to explain it over and over

@jroweboy

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

@wwylele i can't think of any place i've personally seen that, but sure, I'll add that in. Thanks!

@@ -77,22 +77,24 @@ double PerfStats::GetLastFrameTimeScale() {
void FrameLimiter::DoFrameLimiting(u64 current_system_time_us) {
// Max lag caused by slow frames. Can be adjusted to compensate for too many slow frames. Higher
// values increase the time needed to recover and limit framerate again after spikes.
constexpr microseconds MAX_LAG_TIME_US = 25ms;
constexpr microseconds MAX_LAG_TIME_US = 250ms;

This comment has been minimized.

Copy link
@wwylele

wwylele Jan 10, 2018

Member

I am assuming this number is changed for limiter value other than 100%. In this case should this number be a function of limiter value instead?


if (!Settings::values.toggle_framelimit) {
if (!Settings::values.frame_limit) {

This comment has been minimized.

Copy link
@wwylele

wwylele Jan 10, 2018

Member

if (Settings::values.frame_limit == 0)

@@ -20,7 +20,7 @@ std::unique_ptr<RendererBase> g_renderer; ///< Renderer plugin
std::atomic<bool> g_hw_renderer_enabled;
std::atomic<bool> g_shader_jit_enabled;
std::atomic<bool> g_vsync_enabled;
std::atomic<bool> g_toggle_framelimit_enabled;

This comment has been minimized.

Copy link
@wwylele

wwylele Jan 10, 2018

Member

Is this variable actually being used anywhere?

This comment has been minimized.

Copy link
@jroweboy

jroweboy Jan 23, 2018

Author Member

Nope. I think it was originally meant to have it there for a frame limit a long time ago. Didn't really look too much into it, just saw it wasn't used so I removed it.

return;
}

auto now = Clock::now();

frame_limiting_delta_err += microseconds(current_system_time_us - previous_system_time_us);
double sleep_scale = (Settings::values.frame_limit / 100.0);

This comment has been minimized.

Copy link
@wwylele

wwylele Jan 10, 2018

Member

No need for parenthese.

oops, wrong button. Didn't mean to lgtm

jroweboy added some commits Jan 23, 2018

UI: Address review comments
Made max lag time a function of target speed percent.
Added a checkbox to enable/disable frame limiter
@cluezbot

This comment has been minimized.

Copy link

commented Jan 23, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-01-23T07:34:34Z: a66e458...jroweboy:46ea71f0de858cfeec905f0c9376cb22be1713bc

@jroweboy

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2018

Thanks for the review! Changes since last update:

  • Addressed review comments
  • Made max lag time a function of target speed percent.
  • Added a checkbox to enable/disable frame limiter
  • Prevent frame_limit from under/overflowing
  • Hide target speed percent when frame limiter is off
<x>20</x>
<y>20</y>
</hint>
</hints>

This comment has been minimized.

Copy link
@wwylele

wwylele Jan 23, 2018

Member

what does this "hint" block do?

This comment has been minimized.

Copy link
@jroweboy

jroweboy Jan 24, 2018

Author Member

i... don't know actually. It was something added by qt designer, so i got curious and didn't find any information about it out there. i'll just remove it for now

@@ -20,7 +20,7 @@ std::unique_ptr<RendererBase> g_renderer; ///< Renderer plugin
std::atomic<bool> g_hw_renderer_enabled;
std::atomic<bool> g_shader_jit_enabled;
std::atomic<bool> g_vsync_enabled;
std::atomic<bool> g_toggle_framelimit_enabled;
std::atomic<u16> g_frame_limit;

This comment has been minimized.

Copy link
@wwylele

wwylele Jan 23, 2018

Member

I mean, this is not used anywhere, either, right?

@cluezbot

This comment has been minimized.

Copy link

commented Jan 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-01-24T06:14:53Z: a66e458...jroweboy:3106bfbe469e4833f3621df892744f63836bb65b

@jroweboy

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2018

Updated UI picture:
citra-qt_2018-01-23_23-16-18

When its disabled, it will hide the / TARGET% in the status bar as well

@B3n30 if you have a chance to review and merge this please :)

@Dragios

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

By right when uncheck "Limit speed percent" the spin box should be greyed out and changes are locked.

@B3n30

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

Reviewed 4 of 10 files at r1, 1 of 1 files at r2, 6 of 7 files at r5, 1 of 1 files at r7, 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


src/citra_qt/main.cpp, line 294 at r1 (raw file):

    connect(GetHotkey("Main Window", "Increase Speed Limit", this), &QShortcut::activated, this,
            [&] {
                Settings::values.frame_limit += 5;

Not sure about this: Should these hotkeys only be enabled if the framelimiter is enabled?


src/citra_qt/configuration/configure_graphics.cpp, line 99 at r8 (raw file):

        static_cast<int>(FromResolutionFactor(Settings::values.resolution_factor)));
    ui->toggle_vsync->setChecked(Settings::values.use_vsync);
    ui->toggle_frame_limit->setChecked(Settings::values.use_frame_limit);

Rename the check box to "use_frame_limit"?


src/core/telemetry_session.cpp, line 162 at r8 (raw file):

    AddField(Telemetry::FieldType::UserConfig, "Renderer_ResolutionFactor",
             Settings::values.resolution_factor);
    AddField(Telemetry::FieldType::UserConfig, "Renderer_FrameLimit", Settings::values.frame_limit);

I think the use_frame_limit should still stay in telemetry


Comments from Reviewable

@cluezbot

This comment has been minimized.

Copy link

commented Jan 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-01-24T18:49:39Z: a66e458...jroweboy:264170d8ff7a5a99819486b21bf31796bc06a9fe

@jroweboy

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2018

Disabled the spin box when the checkbox is false


Review status: 10 of 12 files reviewed at latest revision, 8 unresolved discussions.


src/citra_qt/main.cpp, line 294 at r1 (raw file):

Previously, B3n30 wrote…

Not sure about this: Should these hotkeys only be enabled if the framelimiter is enabled?

I honestly don't think it matters at all, as if the frame limit is off, no one will even see a difference.


src/citra_qt/configuration/configure_graphics.cpp, line 99 at r8 (raw file):

Previously, B3n30 wrote…

Rename the check box to "use_frame_limit"?

checkboxs in the ui are all prefixed with toggle so i'm keeping it that way for consistency\


src/core/perf_stats.cpp, line 80 at r4 (raw file):

Previously, wwylele (Weiyi Wang) wrote…

I am assuming this number is changed for limiter value other than 100%. In this case should this number be a function of limiter value instead?

Done. The function isn't anything special, and may need adjusting if someone


src/core/perf_stats.cpp, line 82 at r4 (raw file):

Previously, wwylele (Weiyi Wang) wrote…

if (Settings::values.frame_limit == 0)

Done.


src/core/perf_stats.cpp, line 88 at r4 (raw file):

Previously, wwylele (Weiyi Wang) wrote…

No need for parenthese.

Done.


src/core/telemetry_session.cpp, line 162 at r8 (raw file):

Previously, B3n30 wrote…

I think the use_frame_limit should still stay in telemetry

Done.


src/video_core/video_core.cpp, line 23 at r7 (raw file):

Previously, wwylele (Weiyi Wang) wrote…

I mean, this is not used anywhere, either, right?

Done.


Comments from Reviewable

@B3n30

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

:lgtm:


Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@B3n30

B3n30 approved these changes Jan 24, 2018

Copy link
Contributor

left a comment

Can get merged as soon as CIs finish (which will never happen :-/ )

@Dragios

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

Fix clang format 1st before merging.

@jroweboy

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

@jroweboy jroweboy merged commit b002511 into citra-emu:master Jan 26, 2018

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
code-review/reviewable 5 discussions left (wwylele)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@Leo626 Leo626 referenced this pull request Feb 7, 2018

Closed

Progress Report 2018 January #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.