kodi-game / controller-topology-project

The Controller Topology Project models how controllers connect to and map to each other for all gaming history
Open Data Commons Open Database License v1.0
21 stars 15 forks source link

Sega Genesis controllers update #203

Closed KOPRajs closed 1 year ago

KOPRajs commented 1 year ago

The goal is to add a 3 button joypad, the Team Player and the 4 Way Play controllers for Sega Genesis.

garbear commented 1 year ago

Looks like this was fundamentally done right, and good catch on the other fixes. What should we do for the 3-button controller image?

garbear commented 1 year ago

Can you tag me as a reviewer in the future or CC me in the description? I'm gonna start checking my Kodi notifications a bit less and might miss a game-related PR or issue.

KOPRajs commented 1 year ago

What should we do for the 3-button controller image?

How about this? As I've said in the forum, the source picture can be found across the internet on more than 20 different sites, so I don't know who is the original author. I've removed the branding, added transparency, added the shadow and added the background to match the other controller.

Can you tag me as a reviewer in the future or CC me in the description?

I will.

I'm gonna start checking my Kodi notifications a bit less and might miss a game-related PR or issue.

Pitty to hear that you're gonna have less time for Retroplayer, but I can understand that very well.

You can merge this when you think it is ready, I will create another PR for the 2 multiplayer adapters when I get some time.

KOPRajs commented 1 year ago

While testing it, I've found out that I need to make a few more modifications.

KOPRajs commented 1 year ago

@garbear Can you please give me a hand here?

Is there some kind of mapping from the Default controller to every other controller? I mean, when you setup feature name="rightbumper" for the Default controller, the Sega emulator knows that it is feature name="c" for the 6 button controller.

I can't find anything like a translation from game.controller.default to game.controller.genesis.6button.

garbear commented 1 year ago

Is there some kind of mapping from the Default controller to every other controller?

Yes, at the very end, we fall back to C++.

Search https://github.com/kodi-game/game.libretro/tree/Nexus/src/input for the tables the define the default fallbacks for the known controllers.

I think that DefaultControllerTranslator.cpp uses the ergonomic mapping, is this true?

garbear commented 1 year ago

Picture looks great. There's no real copyright reason to attribute the image; all of this is clearly fair use. I attribute images as a favor to the creator. We do take a cost for image attribution, because it's a translatable string. So weight the costs/benefits, and I think it's better to not attribute the image here.

KOPRajs commented 1 year ago

I think that DefaultControllerTranslator.cpp uses the ergonomic mapping, is this true?

Yes, it seems so. One more reason for changing the buttonmaps of affected emulators (like NES and SNES) to match this.

Search https://github.com/kodi-game/game.libretro/tree/Nexus/src/input for the tables the define the default fallbacks for the known controllers.

I think that I'm still missing one piece from the input puzzle. I've installed the new 3 button controller and I've added it to the topology.xml and to the buttonmap.xml of an emulator for testing. I can now select the controller, I can map it and it works as expected, but I've had to manually assign the buttons in the Controls menu.

So my button map in _userdata/addon_data/peripheral.joystick/resources/buttonmaps/xml/linux/GamepadPlus_90b9a.xml now looks like this:

<?xml version="1.0" ?>
<buttonmap>
    <device name="GamepadPlus" provider="linux" buttoncount="90" axiscount="9">
        <configuration />
        <controller id="game.controller.default">
            <feature name="a" button="0" />
            <feature name="b" button="1" />
            <feature name="back" button="10" />
            <feature name="down" axis="+7" />
            <feature name="left" axis="-6" />
            <feature name="leftbumper" button="6" />
            <feature name="leftstick">
                <up axis="-1" />
                <down axis="+1" />
                <right axis="+0" />
                <left axis="-0" />
            </feature>
            <feature name="leftthumb" button="13" />
            <feature name="lefttrigger" button="8" />
            <feature name="right" axis="+6" />
            <feature name="rightbumper" button="7" />
            <feature name="rightstick">
                <up axis="-3" />
                <down axis="+3" />
                <right axis="+2" />
                <left axis="-2" />
            </feature>
            <feature name="rightthumb" button="14" />
            <feature name="righttrigger" button="9" />
            <feature name="start" button="11" />
            <feature name="up" axis="-7" />
            <feature name="x" button="3" />
            <feature name="y" button="4" />
        </controller>
        <controller id="game.controller.genesis.3button">
            <feature name="a" button="3" />
            <feature name="b" button="0" />
            <feature name="c" button="1" />
            <feature name="down" axis="+7" />
            <feature name="left" axis="-6" />
            <feature name="mode" button="10" />
            <feature name="right" axis="+6" />
            <feature name="start" button="11" />
            <feature name="up" axis="-7" />
        </controller>
    </device>
</buttonmap>

Note the game.controller.genesis.3button section.

The 6 button works without the need to manually assign the buttons but the 3 button does not. So there has to be some fallback mapping from game.controller.default to game.controller.genesis.6button, which I can't find. This fallback mapping somehow does work for the 6 button, but it doesn't work for the 3 button. I can't find the "known controllers" table.

garbear commented 1 year ago

I think your solution lies in https://github.com/xbmc/peripheral.joystick/tree/Nexus/src/buttonmapper

When a controller is connected, peripheral.joystick loads all button maps. If any button map can provide a mapping between two controllers in either direction, it is used. If there are multiple mappings to chose from, they're sorted and the most popular map is chosen.

KOPRajs commented 1 year ago

When a controller is connected, peripheral.joystick loads all button maps. If any button map can provide a mapping between two controllers in either direction, it is used. If there are multiple mappings to chose from, they're sorted and the most popular map is chosen.

I can see, finally it makes sense.

So the new game.controller.genesis.3button doesn't work out-of-box, because there is no mapping for it in addons/peripheral.joystick/resources/buttonmaps/xml/.

In such case, this PR may be considered complete. Let's merge this, if you agree with the cosmetics (uppercase X lowercase etc.).

Also this is the answer to "Where should we fix the default mapping for NES and SNES?" We just need to make sure that the ergonomic mapping ("A"->"B", "X"->"Y") is "the most popular" one in peripheral.joystick. If we try to change it anywhere else, it can be always overridden by the maps in peripheral.joystick anyway. And even worse, it depends on how many times the non-ergonomic mapping is present.

garbear commented 1 year ago

The initial goal of the input manager was: "Wrong input is better than no input". I've seen people plug in a controller, have it do nothing, and give up. I've also seen people plug in a controller, have it be slightly different from what they expect, and happily learn to play with the new configuration.

However, I think we've now passed the critical point where most controllers work for most games, and it's time to get most controllers correct for most games.

I actually planned for this inflection point, and it can be achieved without code changes and only XML changes. Simply remove multiple controllers from all button maps but one. One vote wins in a popularity of one. The single button map becomes the codex that all other controllers will use.

However, it's probably best to change the code to hard-code the codex approach. If you can take any controller and map every joystick profile in the UI, then we put that map in a special directory, and modify the code to only load that single map instead of every map.

(It's worth mentioning that loading a single map instead of every map will cause controllers to start working much faster in the UI. Currently there's a 1 or 2 second delay while the button maps load and parse until the first controller is responsive in the UI. I accepted the small delay because I assumed we'd move toward a single-codex approach eventually.)

KOPRajs commented 1 year ago

However, it's probably best to change the code to hard-code the codex approach.

I agree. Even better if this approach makes the controllers work faster! It is no problem when you are connecting only one controller, but when I'm trying to connect 4 controllers to play the Bomberman, it sometimes takes long (the delay is sometimes longer than 1-2 seconds).

However I can only provide the default maps for the controllers that I use (NES, SNES, Genesis and PSX).

garbear commented 1 year ago

That's not a problem, it's easy to match the ergonomics of most controllers because a large picture is shown when you're mapping. For when we're unsure, just check the game add-on's buttonmap.xml and use whatever's there, as it's based on the libretro map which tends towards physically-based.

KOPRajs commented 1 year ago

Ok, I will create the default mapping for all controller profiles using the latest Retroplayer release (there should be all controller profiles included in the build).

I've discussed the "Sentence case" vs. "Capital" in the brackets problem and the conclusion is to keep the capital in the brackets. So I'm going to rebase this to solve the version conflict, I'll add one more fix for the typo in the "Sega Virtua Gun (Eurpoean)" and then lets merge this before another version bump will break it again.

Lets continue the discussion about the default buttonmaps here: https://github.com/xbmc/peripheral.joystick/issues/259

KOPRajs commented 1 year ago

@garbear I've fixed the conflict and added the typo fix, ready to go.

garbear commented 1 year ago

Done!

garbear commented 1 year ago

@KOPRajs The language CI workflow just sent some PRs to sync strings, can you see if everything looks right?

KOPRajs commented 1 year ago

@garbear I don't know how this automated thing works, but it seems that something is messed up. I'm looking into it, but I'm not sure what happened. It looks like there are some mixed newline characters in the description field? But the diff doesn't show any changes to those lines.

KOPRajs commented 1 year ago

@garbear It looks like this commit has added a line breaks: https://github.com/kodi-game/controller-topology-project/commit/d27e77eb58a5575457f2b1cdc7fd21d59071527d But I don't understand why or even if anything in my PR have triggered it. I've checked the git diff of this PR and I don't see anything wrong.

KOPRajs commented 1 year ago

@garbear I don't know what exactly is this supposed to do, but isn't this automated commit broken? https://github.com/kodi-game/controller-topology-project/commit/d27e77eb58a5575457f2b1cdc7fd21d59071527d

The same also happened last year here: https://github.com/kodi-game/controller-topology-project/commit/75e7018fe6fbce175f3a5cd303af8b1af6dca0e4

Long one line strings are being broken into separate strings.

garbear commented 1 year ago

Yup, something went wrong, I'm looking into it with @gade01

gade01 commented 1 year ago

@garbear Is something wrong here? It looks like Weblate is wrapping lines at 77 characters, which is fine. Weblate will change it again at some point, as I configured it to only wrap at new lines.

garbear commented 1 year ago

I think we're good. You can see I reverted a problematic commit in the history. I'll keep an eye on the repo just in case.

gade01 commented 1 year ago

Yeah, I think so too. Thanks for fixing it. It's a good idea to keep an eye on the repo.

Also, we don't want to lose any translations and cause frustrations for translators.