-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Game specific configs #3376
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
Game specific configs #3376
Conversation
Fix #564 |
The issue with your suggestion that a custom config with all settings in it just disabled would face the issue of overriding the base config entirely, as the way it currently works is basically the same as variable shadowing, so if something exists in the custom config, then that value will be used, otherwise it'll be the value from the base config. This issue could be solved by adding all options in the file, but have all of them commented out, and then the user can uncomment whatever they need overridden, although that'd make it harder to write a GUI frontend for it
I believe that should be an easy thing to do
This is not the way the feature currently works (see above), and while this is mostly subjective, I prefer having only the changed config values in the custom configs, instead of full copies of the same file with one or two lines changed in some of them. Also, this PR is only for providing the base feature, and I don't plan to add a GUI to this myself |
a02f417
to
32cdb8e
Compare
32cdb8e
to
7cfc49c
Compare
I may write something stupid below, but I will still express my thoughts, so please don't get angry and don't swear)) My idea was (if it is possible to do this) that the settings would be split into two parts, i.e. the emulator would read 2 different files to have a general picture so to speak. The first part is config.toml (contains general settings - tabs and selections made on them),
When the emulator is launched, the files config.toml + config_default.toml are read, but when a specific game is launched, a second check will occur; if the game uses a custom configuration, the selection will change to config.toml + config_CUSAXXXXX.toml In the interface, it is the flag I remember that you are not so friendly with the graphical interface, but @rainmakerv3 can help here if he has time, provided that everything described above is possible. In Help there will be a description of the functions, for example: P.S. These are just my thoughts and I am not saying that this is right or anything like that, this information is for you to think about (if it is not stupidity) and maybe it will prompt you to improve this PR in some way. |
I tested the latest commit and everything seems to work fine, the only thing I didn't like was that I can't quickly create a custom configuration file. If you don't want to make a GUI because you think it's unnecessary, then I can even understand that, but there should at least be an option for the average user to quickly create such a file via the right-click menu. I think that it is possible to even create a copy of file I also have an idea, which is that in config.toml we can separate all these settings into a separate section, let it be called
Therefore, if there is no GUI, then we need comments directly in the file describing these options. |
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 don't think it's a good approach to duplicate all the settings fields like this, it's more maintenance burden whenever something changes. Would be cleaner if instead you just had one set of config fields that used a wrapper type to be able to pull from the right config.
I updated the PR to main, fixed/changed the two things mentioned in the review comments, and rewrote the code to get rid of duplication. |
src/common/config.cpp
Outdated
data["General"]["volumeSlider"] = volumeSlider.base_value; | ||
data["General"]["isPS4Pro"] = isNeo; |
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.
Why are only some of these using base_value
? Maybe I'm missing something
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.
It's a stupid thing but for integer types not having that there results in the below error, and I couldn't figure out what to do with it, so I just manually specified base_value to force it to build
/media/kalaposfos/Shared/shadps4/repos/shadps4-dev/src/common/config.cpp:959:37: error: use of overloaded operator '=' is ambiguous (with operand types 'value_type' (aka 'basic_value<toml::ordered_type_config>') and 'ConfigEntry<int>')
959 | data["General"]["volumeSlider"] = volumeSlider;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
/media/kalaposfos/Shared/shadps4/repos/shadps4-dev/externals/toml11/include/toml11/value.hpp:403:18: note: candidate function
403 | basic_value& operator=(boolean_type x)
| ^
/media/kalaposfos/Shared/shadps4/repos/shadps4-dev/externals/toml11/include/toml11/value.hpp:443:18: note: candidate function
443 | basic_value& operator=(integer_type x)
| ^
/media/kalaposfos/Shared/shadps4/repos/shadps4-dev/externals/toml11/include/toml11/value.hpp:536:18: note: candidate function
536 | basic_value& operator=(floating_type x)
| ^
1 error generated.
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.
It does seem a bit unintuitive to me for the default conversion operator to be the base value. For example if I write some code that accesses a ConfigEntry
typed field it will compile without any indication you are ignoring the game-specific config value. It would make more sense to me if the default access would convert to the correct config value, and if you need specifically the base value or override value you would do that explicitly.
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 chose to make accessing the base value as the default behaviour so that I only had to modify parts that needed the optional game specific value, and didn't have to rewrite the setters that should only set the base value, but if you think it's better to rewrite that and make accessing a config entry give the game specific value as well, I can do that
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 am thinking maybe we should be more explicit with handling the ConfigEntry values, so that it’s clear what a given piece of code is accessing or setting, whether its base value, game-specific value, or either
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 am thinking maybe we should be more explicit with handling the ConfigEntry values, so that it’s clear what a given piece of code is accessing or setting, whether its base value, game-specific value, or either
I removed the automatic type conversion operator, and made everything explicit in my latest commit
Tested the latest commit, everything is fine. As a regular user I would like to see improvements, not necessarily in this PR and maybe it will be done by someone else, but I will repeat myself a little:
Next 2 options: a) Opens a graphical window similar to what I showed above with help, which of course would be great for a regular user b) Opens the configuration file itself as is (but then it is worth doing point 2 )
P.S. It is necessary to edit the first comment after making the last changes. |
With this PR, you can create a new config file in userdir/configs/ named CUSAXXXXX.toml, where CUSAXXXXX is the ID of the game you're making the config for, and you can put any option in it that you'd like overwritten for that game only. For example, if you want to have readbacks turned on for every game, but have DMA only for SotC, you can turn on readbacks in the main config (which is still userdir/config.toml), have DMA turned off there, then make the userdir/custom_configs/CUSA08809.toml file and put the following in it:
If you have a game running, and update your settings from the GUI by opening settings and clicking save for example, these game-specific changes won't override your main config, however you can't reload them either by doing that.