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

Android: Update "Speed Limit" for "Unlimited" value #8928

Merged
merged 1 commit into from Jul 8, 2020

Conversation

Ebola16
Copy link
Member

@Ebola16 Ebola16 commented Jul 4, 2020

Changing this setting to 0% is how Dolphin interprets "Unlimited" speed limit.

@JMC47

@Ebola16 Ebola16 marked this pull request as draft July 4, 2020 07:58
@Ebola16
Copy link
Member Author

Ebola16 commented Jul 4, 2020

There are too many instances of conflicts with #8894 for this PR so I'll wait until after it's merged.

@JMC47
Copy link
Contributor

JMC47 commented Jul 5, 2020

That's fair. I'll get to testing this soon. It's been a bit hectic around here.

@Ebola16
Copy link
Member Author

Ebola16 commented Jul 5, 2020

Thanks! Instead of a "Disable Speed Limit" option, I think I'll implement the full "Speed Limit" slider as an in-game setting. This means that we're modifying a core setting during emulation and our current "modify all core settings" option won't work. I'll do something similar to #8894's SConfig::LoadSIDevice0Setting(), which will be yet another merge conflict if I try implementing that here now.

@JosJuice
Copy link
Member

JosJuice commented Jul 5, 2020

Writing to the INI during emulation and then asking the core to read from the INI is a hack that I would rather not extend to arbitrary settings. We should wait with making settings like this modifiable during gameplay until we have a comprehensive system for modifying in-memory settings.

@Ebola16
Copy link
Member Author

Ebola16 commented Jul 5, 2020

Is there a downside in writing to the INI during emulation and then reading from it? I can convert SConfig::LoadSIDevice0Setting() to SConfig::ReloadInGameCoreSettings so the relevant in-game core settings will be contained in one place. And I'm trying to avoid SharedPreferences since they're not retained across Dolphin builds.

Alternatively, should I use a SharedPreferences intermediate setting and save the INI setting on emulation close?

@JosJuice
Copy link
Member

JosJuice commented Jul 5, 2020

Is there a downside in writing to the INI during emulation and then reading from it?

The main reason I don't like it is because you are bypassing the layered config system (which to be fair isn't used by the Speed Limit setting yet, but is used by many other settings, and using the INI reloading approach for Speed Limit will make things harder for us when Speed Limit does get switched over to the new layered config system). Writing and reloading INI files also seems more cumbersome and slow.

Using SharedPreferences would not improve anything. I'm talking about using JNI functions to modify the values in SConfig and/or Config directly.

@Ebola16
Copy link
Member Author

Ebola16 commented Jul 5, 2020

Oh ok, if there's plans to convert Speed Limit to a different config system I agree with postponing https://bugs.dolphin-emu.org/issues/12174.

This PR in it's current state would probably be a useful standalone PR since users may not expect 0% to be unlimited speed.

And to make sure I'm understanding you correctly, is #8894's NativeLibrary.SetOverlaySIDevice() acceptable since it handles changes through SConfig?

@Ebola16 Ebola16 marked this pull request as ready for review July 5, 2020 09:41
Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

Yes, I also think it would make sense to merge this PR in its current state.

And to make sure I'm understanding you correctly, is #8894's NativeLibrary.SetOverlaySIDevice() acceptable since it handles changes through SConfig?

I don't really like the LoadSIDevice0Setting part of it. But let's leave that to when I get around to leaving review comments on that PR.

@lioncash lioncash merged commit 25118e2 into dolphin-emu:master Jul 8, 2020
@Ebola16 Ebola16 deleted the ADSL branch July 8, 2020 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants