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

Rename "Speed up Disc Transfer Rate" to "Emulate Disc Speed" #11538

Merged
merged 2 commits into from Feb 13, 2023

Conversation

t895
Copy link
Contributor

@t895 t895 commented Feb 3, 2023

Following a suggestion from @mbc07, this renames the "Speed up Disc Transfer Rate" option to "Emulate Disc Speed" and inverts the setting. Since we have no guarantee as to whether or not the improved version of instant reads will ever be created, this allows us to keep the current implementation in a way that will hopefully reduce encourage reckless user behavior.

Additionally, this PR exposes the option in the Android GUI.

@MayImilae
Copy link
Contributor

MayImilae commented Feb 4, 2023

If the intention is to keep users from using the faster disc reading speed senselessly, I doubt this can prevent it. Users looking to make things faster will do dumb things sometimes. However, this new approach is more thoughtfully worded and switching the direction should at least stop the "see 'speed up', immediately click" action. So I think this is an improvement.

LGTM.

@JosJuice
Copy link
Member

JosJuice commented Feb 4, 2023

Changing the name in the GUI makes sense to me, but I'm unsure if we really should break existing configs by also renaming the INI name of the setting.

Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Feb 5, 2023

Yeah I'm with Jos, you should leave the backend setting alone and just have the GUI setting behave inverted.

@t895
Copy link
Contributor Author

t895 commented Feb 7, 2023

I've inverted the option in the UI but kept the key so we don't break configs. I haven't touched the Qt code before so please let me know if this is a bad way of doing this.

Copy link
Contributor

@mbc07 mbc07 left a comment

Choose a reason for hiding this comment

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

Code looks fine to me, untested though...

Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

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

The INI setting and reading code is really ugly tbqh, but making it nicer (transition the dialog to GraphicsBool and similar, maybe?) is really out of scope for this PR.

Likewise, does Android not have a wrapper for a GUI element that is inverted compared to the INI setting?

But yeah other than that seems fine.

@AdmiralCurtiss
Copy link
Contributor

JosJuice informed me that yes, we have InvertedSwitchSetting on Android. So maybe use that there.

@t895
Copy link
Contributor Author

t895 commented Feb 12, 2023

JosJuice informed me that yes, we have InvertedSwitchSetting on Android. So maybe use that there.

Don't know how I missed that. Fixed.

@phire phire merged commit 2c24d07 into dolphin-emu:master Feb 13, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants