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

Control profiles #15406

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Control profiles #15406

wants to merge 2 commits into from

Conversation

iota97
Copy link
Contributor

@iota97 iota97 commented Feb 17, 2022

Implement control profiles.

Use cases:

  • One may want to share the same controls preset in a few game and have other different setting per game, this make it easier.
  • Run time change as different game area/situation may map better to different controls layout (mostly on touch controls).
  • More binding on the controller: you may bind a key to change the profile and then have all key bind to action as fast forward, screen shot and so on in that profile. (This also change touch control so you can add a touch control to tell in which state you are).

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:

if (!file.HasSection("ControlMapping")) {

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.

@unknownbrackets
Copy link
Collaborator

unknownbrackets commented Feb 21, 2022

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:
ControlMapping"

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 [ControlMapping] it could be [ControlProfile1Mapping] etc. I feel like this could just go in controls.ini? SaveToIni could just take a section name.

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]

Core/Config.cpp Outdated Show resolved Hide resolved
@hrydgard hrydgard added this to the v1.13.0 milestone Feb 27, 2022
@hrydgard hrydgard added the Input/Controller Input and controller issues label Feb 27, 2022
@hrydgard
Copy link
Owner

will take a real look soon! sorry for the delay in reviewing.

Copy link
Owner

@hrydgard hrydgard left a 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
Copy link
Owner

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...

Copy link
Contributor Author

@iota97 iota97 Jul 16, 2022

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

Copy link
Owner

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.

@hrydgard hrydgard modified the milestones: v1.13.0, Future-Prio Jul 16, 2022
@iota97
Copy link
Contributor Author

iota97 commented Aug 14, 2022

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.

@hrydgard
Copy link
Owner

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.

@iota97
Copy link
Contributor Author

iota97 commented Nov 14, 2022

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?

@hrydgard
Copy link
Owner

What do you mean by manually writing down virtkey values? Giving them the string names?

@iota97
Copy link
Contributor Author

iota97 commented Nov 29, 2022

I mean we write VIRTKEY_CONTROLLER_PROFILE_1 = 0x40000027. We could omit the value and it will auto increase anyway, this seems just more affected by typo and git conflict to me. Not sure if there is any pro in this ;)

@unknownbrackets
Copy link
Collaborator

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]

@ghost
Copy link

ghost commented Feb 27, 2023

This fixes #16960?

@hrydgard
Copy link
Owner

hrydgard commented Apr 2, 2023

Tried this again. As before, i'm concerned with the UX.

While it's not too hard to understand what's going on:

image

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...

@iota97
Copy link
Contributor Author

iota97 commented Apr 2, 2023

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 :)

@hrydgard
Copy link
Owner

hrydgard commented Apr 2, 2023

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...).

@hrydgard hrydgard modified the milestones: v1.15.0, v1.16.0 Apr 26, 2023
@hrydgard hrydgard modified the milestones: v1.16.0, v1.17.0 Aug 29, 2023
@hrydgard hrydgard modified the milestones: v1.17.0, v1.18.0 Jan 18, 2024
@hrydgard hrydgard added the TODO after release Things to do after the next release label Sep 11, 2024
@hrydgard hrydgard modified the milestones: v1.18.0, v1.19.0 Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Input/Controller Input and controller issues TODO after release Things to do after the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control profiles. Feture request changeable controler profiles
3 participants