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

Fix path-loading of GBA map configurations #12569

Merged

Conversation

GregoireLD
Copy link
Contributor

After digging into configuration loading, I wanted to give a try at tackling a bug I discovered a while ago
( https://bugs.dolphin-emu.org/issues/13096 )

Currently, Dolphin only recognize PadProfileX (pointing to the "GCPad" folder) and WiimoteProfileX (pointing to the "Wiimote" folder) for loading controller mapping parameters.

As such, it's not currently possible to specify a GBA control map in a game-specific INI (unless you step in relative paths territory).

Since GBA profiles are stored in their own third folder, I propose to add a third parameter "GBAProfileX", using the very same loading process and naming convention as the other two.

Since this wasn't supported before, this shouldn't have adverse effect (even for people like me using relative paths, the relative path would still work) but as I'm still pretty new here, I'll welcome any remarks and even if I haven't noticed any issue, some more testing might be in order.

@GregoireLD GregoireLD marked this pull request as ready for review February 7, 2024 18:59
@JosJuice
Copy link
Member

JosJuice commented Feb 7, 2024

"GBAProfileX" is already implemented in InputConfig.cpp, so it makes sense to me to use the same approach here.

@GregoireLD
Copy link
Contributor Author

Hi, and thanks for your input.

Just to be sure I haven't forgotten something, should I do something else on this PR ? or is it all good
(no hurry, I just want to be sure I'm not supposed to do some action, that I might've forgot about)

@JosJuice
Copy link
Member

There's nothing you need to do right now. I'm waiting for someone else to also review the PR.

@AdmiralCurtiss
Copy link
Contributor

I'm... a bit confused here. Even without this PR, if I put this in my game ini:

[Controls]
GBAProfile1 = xinput

It works just fine, because Core::EmuThread() calls Pad::LoadGBAConfig() which loads the profile. So what exactly is this solving?

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Feb 21, 2024

(For that matter, what is this code doing anyway? It's injecting the selected profile into the LocalGame config layer, but why? What code is searching for it there?)

e: Ah that's the default location for the 'not profile' mappings, that makes sense then. Still a bit confused why we have two pieces of code that seem redundant then?

@GregoireLD
Copy link
Contributor Author

I must admit that when I tried to track down the game-specific INI loading function, I found this, but I did not realize that the LoadGBAConfig() was loaded form elsewhere (I did not even realize this very function existed at all).

I just built a version with
Pad::LoadConfig(); Pad::LoadGBAConfig(); Keyboard::LoadConfig();
(lines 514 to 516) commented out from Core::EmuThread(), and all controller-related parameters seems to load and work as usual (from the default file as well as game-specific INIs), but I don't feel confident enough to declare we could definitively remove these references (or maybe it's the other way around and it's the function I modified that should go unused).

If requested, I can push the version with the commented out lines for others to test it

@GregoireLD
Copy link
Contributor Author

I'm still trying to figure out how controller settings are loaded, and why it seems we have two snippet of code performing pretty much the same tasks.

If it makes sense, maybe this PR could be validated so that both snippets are technically equivalent (specifically, loading GBA related controller settings), then, in a separate PR, I will check (and maybe remove) one of the two loading code, knowing they both would then be equivalent. Or do you think it's preferable to do it all in this PR ?

@AdmiralCurtiss what do you mean by 'not profile' mappings ?

@AdmiralCurtiss
Copy link
Contributor

Yeah, I think making the two equivalent and then dropping one in a separate PR is a good way to go.

By 'not profile' I mean the current/global mappings. The ones that you modify directly, that you actively see in the config window.

@AdmiralCurtiss
Copy link
Contributor

So I still don't fully understand why there's two separate loaders but I think this change is technically correct from what I can tell. I'm almost positive one of the two can be removed and replaced with a call to the other though.

@AdmiralCurtiss AdmiralCurtiss merged commit 107379b into dolphin-emu:master Apr 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants