-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Control profiles #15406
base: master
Are you sure you want to change the base?
Control profiles #15406
Conversation
So this essentially implements hot-swappable controller profiles. Initially I was imagining you'd assign custom game configs to specific profiles, but this seems more versatile. The intent of that line was to allow for defaults on first startup, i.e. to avoid these lines: Otherwise, if I have no inis yet and start up, I would get no defaults. That said, I don't know of anything that would break if we allowed saving empty sections (though I'm not terribly sure how useful an empty controller profile is...?) Personally, I feel like it'd be nicer to put all controller profiles in the same ini, just with different section names. Instead of We could also "port" the settings in the separate section on first startup with those settings missing from the new place. Otherwise there will definitely be people confused or thinking we removed features. -[Unknown] |
will take a real look soon! sorry for the delay in reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay in this one, I really don't know what I want to do with control profiles, and since this has some potential backwards compatibility issues, I'm gonna bump this to after 1.13.
Having multiple control profiles has a lot of value of course so we're gonna get this in later in some form.
@@ -601,6 +586,7 @@ static ConfigSetting generalSettings[] = { | |||
ConfigSetting("EnablePlugins", &g_Config.bLoadPlugins, true, true, true), | |||
|
|||
ReportedConfigSetting("IgnoreCompatSettings", &g_Config.sIgnoreCompatSettings, "", true, true), | |||
ConfigSetting("SettingsVersion", &g_Config.uSettingsVersion, 0u, true, true), // Per game for game configs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We traditionally haven't had a version number for settings. I hope the addition of this will not lead to a proliferation of changes, because testing all kinds of settings upgrades will be very laborious...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have a bool specific for this port like bRightAnalogSettingPorted
that would reduce the temptations to add new settings version.
Anyway I see your concern with this PR, if/when you decide what's best for the project I'll change and rebase accordingly :)
Edit: another option would be to just leave the settings in the old section and have the profile code get them from there, a bit uglier code but the end user would not notice at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm taking back my comment here, SettingsVersion is the better option, otherwise if we make more corrections in the future, we'll just end up with more specific bools and the combinations get harder to understand.
I had some spare time at last, changed the logic to leave all setting where they are avoiding possible compatibility issues. There is simply a list of setting that get saved in the profile that are from general. |
This now has some conflicts due to the way too long review process, sorry :( Could you rebase those out? Then I'll play around with this myself to get a feel for it. I do want this feature in in some form. |
9b0ff06
to
8dbe935
Compare
Squashed the commit for my own sanity before rebasing, I have a local backup of the other versions if needed. PS: any reason we are manually writing down VIRTKEYS enum values? |
What do you mean by manually writing down virtkey values? Giving them the string names? |
I mean we write |
Since those values are saved externally, it's kind of a good thing that it causes conflicts when two people use the same number. I don't think it's bad to be explicit in "exposed" values. I think it makes sense to let auto-numbering happen when you don't care if i.e. two compiler versions auto-number the same way. -[Unknown] |
This fixes #16960? |
Tried this again. As before, i'm concerned with the UX. While it's not too hard to understand what's going on: it's gonna be hard to remember which is which. It's also not clear what happens when you have a game config - are all setups saved in the game specific config, or global? Also, it's unclear from the UI if this applies to bindings, touch screen UI, or both. I'm just afraid it's too confusing.. But like I said, I do like the functionality... |
We could add a string on top explaining what get saved. More strings to traslate tho'. About global vs game specific: this should be global to allow to "port" configs from one game to another. We could possibly add a simple text input to add a little description to help remember. I find this more useful than not, even if a bit confusing should be easy enough to understand if a few tries :) |
The confusion over what belongs to game and what is global does really go beyond this particular thing.. I've been thinking for a long time for how to highlight what's what in the UI, and how to make it understandable... I have a few concepts in mind, but to make it managable, need to make as much as possible of the settings menus dynamically generated from tables (like if we add a little color flash to settings that are being modified for the specific game, don't want to manually add that to all of them...). |
Implement control profiles.
Use cases:
Look like when I implemented right analog setting I didn't notice the section separation, this will reset those settings (and gesture one) :/
If the key mapping have no entry save will create no section, so load will trigger this:
ppsspp/Core/KeyMap.cpp
Line 676 in 9bc6b96
This will make default key map to be loaded instead of empty binding, not sure if there was any reason for that check to be there so I'm asking first if I could just remove it. Or I should make save create the section always?
Fixes #14404 and fixes #13303.