Add ability to edit game-specific GFX settings from game properties tab. #13063
Conversation
|
I voiced my opinion several times on discord, so this is probably unnecessary to state. This is more in case whoever is reviewing/merging doesn't see those comments and for some reason doesn't think of this. I don't think we should be using a button to open the Dolphin global windows. We should be using the widgets themselves as new tabbed entries in the game specific window. This is why I broke various pieces of code into their own widgets in the first place, see also my mocked out example window which uses those widgets. I haven't looked at other emulators but I'd assume this is how they are doing it (most seem to be using a unified window as well but baby steps..). This gives a consistent experience to the user (settings are in the same place, they behave the same as global settings, users have full control, etc). Regardless of approach, will we keep the ini editing capability? I think it was primarily for graphics... All that being said, happy to see this picked up. Thanks @TryTwo ! |
|
Not part of this PR You're thinking something like this is better? There needs to be a way to switch between three states. On, Off, and Not Local. I have this problem too in this PR. I'm also not sure if there's a simple way to remove global settings and only show local ones. |
|
@TryTwo - yeah, if we go that route, I'm sure on of the UI experts can provide ideas on what best to do. My thinking was to add a "Graphics" tab at top. Then have a "General", "Hacks", "Advanced" set of tabs underneath, just like we do today in the main window. But maybe there's a better visual look. (also depends on if we want to keep General/Editor under "Game Config"...but I don't want to blow out your PR lol)
I was thinking the global window would be the "Not Local" setting. But to your point, how do you unset a game specific setting and go back to global. I wonder how other emus do this. At least looking at a PCSX2 screenshot, seems their comboboxes have a "Use Global" option (that would be more work that I wasn't considering). But also not sure how their checkboxes work... 🫤 Maybe just a global "reset" button to just clear the layer? Definitely an interesting question! |
I can't speak on a technical level, but PCSX2 has three states for checkboxes when editing game-specific settings; on, off and use global. |
Thanks, that's a good solution for a more robust PR. Unfortunately it's too much work for a quick fix, since we're using existing widgets that don't have that. If we duplicate widgets into game config then a reset button per widget to clear sections would be fine. Otherwise I think a reset everything button will be needed. There's also the suggestion for right clicking to disable the User state for an individual option. It's possible to do. |
|
My conclusion is that the game config layer system needs to be expanded before this can be done correctly. imo the game specific settings window needs to operate on a separate instanced layer outside the global layer system (which isn't built to handle this). Game properties bypasses the layer system entirely, so that's why there's no problem currently. A few important settings could be added to the current game properties method as a quick fix, but any robust improvement will require more than I originally thought. I'll leave it to someone else to tackle those bigger tasks. The Android version works because its quite limited. One game at a time, not when a game is running , not when the global settings are up. I can duplicate those restrictions, but I think the end result won't feel good on PC. It also prevents @iwubcode 's idea of keeping the settings in the game properties window. |
TryTwo
force-pushed
the
PR_GameSettings
branch
from
d97233c
to
031a12d
Compare
|
I was able to add right-click to delete a local game change. For general config it's hard to tell what's appropriate to edit for a local game ini. My method just attempts to write any change to the game ini. Not sure the general config window should be added. |
TryTwo
force-pushed
the
PR_GameSettings
branch
from
031a12d
to
905391a
Compare
|
I tried doing what I said I wasn't going to do. We'll see if it worked out. I managed to fit all the stuff into game properties and make it work with multiple game properties open at once. Any custom-written game options haven't been converted to work with this. |
TryTwo
force-pushed
the
PR_GameSettings
branch
3 times, most recently
from
636099a
to
b290fb8
Compare
TryTwo
changed the title
|
Did some testing. Overall it works just fine for the editable fields. The only 'issue' is editing Backend or Adapter in GFX -> Basic changes it for Global.
Presumably this is already known based on your prior comment:
Calling it out just in case. The sub-views within views is... not pretty to look at, but this is perfectly functional as is. I already have two practical uses for this:
|
|
Thanks for looking at it. Do you think backend and adapter are important to have editable? Or should I just disable those? There's a small problem of options pulling from backend info to see how they should be filled, which is loaded globally. I think it makes sense to just bypass it and show all options, but it may be confusing if backend isn't disabled - as you might expect it to limit what options are being shown. |
I think it's fine to disable it for now. |
|
While they are important, let's not let perfect be the enemy of good. It's very rare that someone would want a specific backend for a specific game (MSAA on D3D breaking less games being a potential one) but it's a far better solution than having to change everything. |
|
Agreed, I would rather see this merged even with the "not great" UI because the functionality is worth having. |
|
I believe I have everything in Enhancements working now. If someone could make a ConfigComplexChoice that takes any two types of Config::Info and a vector of {QString Name, T1 setting1val, T2 setting2val} there wouldn't need to be so many edits to the enhancements widget. I haven't looked to see if that'd be useful beyond the two settings in Enhancements. |
TryTwo
force-pushed
the
PR_GameSettings
branch
from
754ec3d
to
98675eb
Compare
|
Realized the Backend combobox can be turned into a ConfigStringChoice, making it easy to include in these changes. I avoided most of the other problems. Leaving adapter disabled. Also, I manually set AA options to 2, 4, 8 (which is the option for most backends) unless OpenGL is in the game ini, then it shows 16 and 32 as well. I think that keeps things from being too confusing. Are there non-windows builds that use these Qt widgets? Not sure about their backends. |
TryTwo
force-pushed
the
PR_GameSettings
branch
from
586919d
to
f8c7a89
Compare
|
Could the commit history please be cleaned up? It would make this easier to review. |
TryTwo
force-pushed
the
PR_GameSettings
branch
from
f8c7a89
to
e0b3a89
Compare
Ok, I think I got it under control. |
There was a problem hiding this comment.
As I've mentioned on Discord, I'm not a fan of the tabs inside tabs inside tabs design that this PR opted for and would like to see less of it (instead of more), however, in its current state, this PR does provide new (and wanted) functionality, which I tested and seems to work as expected.
I won't try to block it over a complete redesign or something along those lines (even though I strongly think there has to be better ways to present this to the users), but I'll request you to at least make it consistent with the existing UI by padding the tab switcher and its contents. More specifically, I'd like to see the default Qt padding restored at these points:
For reference, here's the default padding as applied in the existing Editor tab, which also follows this tabs inside tabs design:
After the padding change, this LGTM.
|
I can remove the GFX tab and put everything on the same tab line, if that helps? I did a GFX tab to prepare for a possible General Settings or Audio tab, so it wouldn't overflow off the page like the other tabs at the very top. I can fix those margins either way. If the EnhancementsWidget refactor is too much, I think I figure out how to make the remaining options into ConfigChoices instead. |
TryTwo
force-pushed
the
PR_GameSettings
branch
from
e0b3a89
to
b6b4e45
Compare
|
Thanks, iwubcode. I hope my coding is good enough. I'm learning how to do some of this stuff still.
This would clean up the overflow, which would be an improvement imo. It would probably be doable to abandon the tabs entirely for another navigation style in the same window. Whatever it'd be, it'd still "page" through the existing widgets. Open to suggestions, as long as it doesn't involve re-writing everything. What's up with those build fails? Also, if the EnhancementsWidget changes are too much, I think I figured out how to template those options that take two Config::Info<> settings, and could place most changes into another ConfigComplexChoice.h. |
Most of what you said is beyond the scope of this PR, though, and in a hypothetical scenario where this PR also included a bigger redesign, nested horizontal tabs (like we have now) definitely wouldn't be my choice either, as I mentioned earlier. With that said, I strictly reviewed what this PR is bringing to the table and how it was added to the current UI, without considering any major redesigns. In that regard, moving the "GFX" tab to the top-most level instead of keeping it nested inside the existing "game config" tab is worse (IMHO)... |
|
@TryTwo - I gave it a skim, I liked the idea you came up with! I will give it a more in-depth look tomorrow if others haven't code reviewed it already. While I have my hopes and dreams, I think what you've done is set us up to expand this idea in the future. Most of what I said would be nice to add but starting small will have the best chance of getting approval. And I think many users would agree this is a much needed improvement! So I think you made the right choice 👍 |
This is the case for quite a few games currently. For example E.ini, F.ini, J.ini, L.ini, M.ini, N.ini, P.ini all will load for any gameINI with a GameID starting with that letter. So we are talking about over a hundred games affected. |
|
I think we should only load the per-game INI. If you open the game properties page, it should probably always be the 3 (or even 6?) character INI. So, if you load an NES game, it would be the one letter INI, but rather the full game ID. If the settings of the 6 character ini differ from the 1 character, i'm pretty sure the 6 character ini are higher priority. |
|
It's a combination of each of the INIs, sometimes even more than 2 layers for example PZLE01 (P.ini, PZL.ini, PZLE01.ini) or any game that has 2 layers plus a user config INI, with each setting from the INIs being applied, and yes each longer INI gets preference if it's the same setting that is altered. That's something that is currently handled correctly in master although the list of settings exposed in game config in master is minimal. EDIT: |
|
You're right, thanks for explaining that. It should definitely communicate the current behavior, then we can change the behavior if needed, later. |
|
Okay. I'm going to change it a little. Currently, if P.ini exists with no game_id.ini, then a game_id.ini tab will not be created when you open game properties. I'm going to make sure game_id.ini will always have a tab, then it will just recreate that one tab to update it. |
This is a bit silly, but the game properties window has |
The existing setting in the Game Config tab is supposed to be an offset on top of the depth value set in the graphics settings. I'm not sure in how to properly handle that either... |
TryTwo
force-pushed
the
PR_GameSettings
branch
from
55e5216
to
0996c55
Compare
Good catch. Explains why convergence isn't there. We may break people but they can just go and tweak their settings (using your new window!) to fix it. The only thing I could see is this might provide a finer level of control to depth values. But if that is the case, we should just make a more granular control. This is another relic of the old system imo. If you want to punt on this too, that's fine but I think this will need to be fixed. It's awkward. |
|
@iwubcode the default game INIs we ship in Dolphin has optimal depth values for a bunch of games (right now I recall Luigi's Mansion, but there are others), that are multiplied on top of the user-defined depth value to produce the expected 3D effect. It's not just a matter of yanking the existing slider out and directing people to the one available in the graphics settings, they are not equivalent (at the moment, at least)... |
I'm aware.
But it is. The percentage thing, was just that, a percentage of the optimal value. Spanning from 100-200 (so up to double). As I mentioned, it'd be a matter of updating the control to provide more flexibility (probably also be more explicit) and then removing the existing setting/control. If it needs more tweaking, we do so when we have a bug report or a dev has inspiration. It's probably already clear but I am very much a supporter of breaking things to move forward (if that is the cleanest solution). Obviously, I want to avoid breaking changes but I will use them if necessary. Avoiding removal of old ideas is how a product gets debt and this debt can bleed into the rest of the project. Breaking changes do have their own challenges and open source means new ideas can remain half finished.. but those sorts of problems are much easier to solve than generally trying to adapt the old idea in with the new system. That's just my general stance on breaking changes. An extra slider in a UI is not going to require much maintenance if we would decide to keep it. At the same time, if we want to remove it, I would prefer not to wait for years for a perfect solution that achieves backwards compatibility when we could make the drastic change and move on. I feel like I've gotten us on another tangent. I wanted to throw out other ideas for the team to think about but I will leave it at that. Thanks for reading 😅 |
|
It's a good time to throw ideas while it's being worked on. A month later and I'll have forgotten all the details I discovered the normal game properties checkboxes load the global/sys game value if it exists. It's unfortunate that there's no easy way to do that for GFX. If I convert the main game properties to the config system, and keep that system ini feature, I could make system game values Italic to point them out. |
|
My point here is that, in its current state, this PR already brings big QoL improvements, has an acceptable design (UI-wise) and is ready to go. All those things you want to do are mostly good, but can be discussed and implemented in follow up PRs with relatively ease. Keep trying to chew up too much in one go and this PR will likely end bloated, harder to review and held back due to disagreements on specific UI choices, what should go where, what should be dropped/added and so on, eventually falling into a limbo and never shipping to end users. It wouldn't be the first time this would happen, either... |
| // Bypass global backend_info when making game-specific changes. | ||
| if (m_game_layer != nullptr) | ||
| { | ||
| if (m_game_layer->Exists(Config::MAIN_GFX_BACKEND.GetLocation()) && |
There was a problem hiding this comment.
Multiline if statements need to be wrapped in {}.
I'm curious where you got that list. I'm not sure if we want to / should assume the AA modes...
There was a problem hiding this comment.
I just made it up based on DX11, DX12, and Vulkan all using those numbers. Not great, the only alternative I know is to hand code the numbers for each backend.
There was a problem hiding this comment.
I'm leaning toward just leaving the AA modes out for now? In the future, one possible solution is we could query the various backends at startup to see what is available. But even that is a bit klunky. It's possible #11531 could help with this. I'd be curious how other emulators handle it and open to comments from others too..
TryTwo
force-pushed
the
PR_GameSettings
branch
from
0996c55
to
8532988
Compare
|
@iwubcode I tried what you said with the ConfigWidgets. Can you give me your thoughts? I also moved the MouseEvents to GameConfigWidget, because that is the only place they apply. The code to do that is a little awkward. RenderToMainWindow and Auto-Adjust Window Size don't seem to work from a game config ini. Maybe those should not be editable? (note: to make them uneditable you just remove the layer pointer from being passed). |
I love it! Great work.
Yeah, I do find this a bit awkward. I think it'd be cleaner in |
…set to the user game ini. Additionally, refactor ConfigWidgets to reduce duplication. Creates a layer outside the game config layer system and passes it to the created gfx widows, so as to not interfere with the global config system. Supports multiple game properties being open at once. Supports editing while a game is playing, but the options only save and update the active game when the window is closed. Right-clicking will remove a property from the game ini.
Cleanup redundant calls. The argument was rarely being used, and could be replaced with checking the config. GeneralWidget was trying to use the signal to update the backend combobox, but this also looked redundant. The only time the combobox differs from the config should be during OnEmulationStateChanged, and there's a function to handle that.
TryTwo
force-pushed
the
PR_GameSettings
branch
from
8532988
to
1988739
Compare
Add method to bold slider/spin labels. Reduce ConfigSlider signal spam. Small bugfix for tooltipwidget.
TryTwo
force-pushed
the
PR_GameSettings
branch
from
bfcd02a
to
aeedaf5
Compare
|
Since no full review has started, I've been continually refining things. Someone might want to test the build again. I refactored the general config tab to use the config system, removing tri-state boxes for consistency. Added a note on what bold means and right-clicking to remove settings. Added a class to link and bold the slider/spinbox labels, since the sliders/spinboxes can't be bolded themselves. Added italics for the general tab when it detects global game settings. The only thing left on my list is possibly making the dual-option comboboxes in EnhancementsWidget work with the config system. I also figured out how to put a floating number on the QSlider to show the actual value when sliding, but I won't add that here. |
|
There is one minor issue with overriding the video backend, the title of the graphics configuration window no longer gets updated unless the video backend dropbox is changed manually. Master: This PR: |
edit Reworked and ready to be reviewed.
Follows iwubcode's suggestion below. Adds tabs to game properties window that allows editing game-specific GFX settings. Uses the existing GFX widgets as new tabs, but passes a game layer to them that's outside the global game layer system. This is the solution I came to after trying multiple things.
Features:
Notes:
This PR integrates with the ConfigBool, etc widgets that set most GFX settings. The GFX settings outside of this system require too much editing for this PR. They would need to be conditionally redirected to the custom layer, be bold-able if in the game ini, right clickable to remove them from the game ini, and save only when interacted with (they usually batch save). All of those are hard to do for the few options that require it.
Possible issue:
If multiple game ini's are somehow loaded into the editor widget as tabs (maybe from game modding?) it won't refresh the game ini tab correctly.