Allow Ports To Configure Default Button Mappings #724

MegaMech · 2024-11-22T00:19:55Z

Replaces hardcoded default button mappings with cmake variables

Replace keyboard button enums with default macros set in cvars.cmake. This allows ports to define their own default using CMake/lus-cvars.cmake file.

A PR to SOH and 2Ship should not be necessary because I believe they use the default button mappings.

briaguya-ai

overall the logic looks reasonable, but i'm not a huge fan of the LUS_BTN_* names

although verbose, i'd prefer something more descriptive such as LUS_DEFAULT_KB_MAPPING_*

edit: also, the name of this pr feels a little misleading. LUS already has default keyboard mappings, if i were to name the PR i'd call it "allow ports to configure default keyboard button mappings"

cmake/cvars.cmake

Kenix3 · 2024-11-22T15:59:48Z

overall the logic looks reasonable, but i'm not a huge fan of the LUS_BTN_* names

although verbose, i'd prefer something more descriptive such as LUS_DEFAULT_KB_MAPPING_*

edit: also, the name of this pr feels a little misleading. LUS already has default keyboard mappings, if i were to name the PR i'd call it "allow ports to configure default keyboard button mappings"

I agree with Bria's naming suggestion here. Can we get this in?

MegaMech · 2024-11-22T16:01:39Z

Done! (Albeit, I feel like I'm now in a sort of time distortion because I can't get in again what I've already gotten in)

MegaMech · 2024-11-22T16:03:00Z

Don't merge yet. Starship wants controller defaults

briaguya-ai · 2024-11-22T16:06:37Z

Don't merge yet. Starship wants controller defaults

that doesn't need to be the same PR though right?

briaguya-ai · 2024-11-22T16:11:24Z

Done! (Albeit, I feel like I'm now in a sort of time distortion because I can't get in again what I've already gotten in)

i suggested "allow ports to configure default keyboard button mappings" and the title is currently "Allow Keyboard Button Mapping Configuration In Games"

"Allow Keyboard Button Mapping Configuration In Games" is ambiguous, players can already configure keyboard button mappings. this PR is replacing hardcoded default keyboard button mappings with cmake variables. come to think of it that'd be another reasonable title - "replace hardcoded default keyboard button mappings with cmake variables"

briaguya-ai

the controller side of things feels a bit more complex because it uses both SDLButtonToButtonMapping and SDLAxisDirectionToButtonMapping

the current implementation allows ports to

set any SDL button to:
- any N64 button (including dpad and c buttons)
kind of configure SDL axes for the c buttons (axis vs axis direction issues)

the current implementation does not allow port to

set SDL buttons to analog stick axis directions
set SDL axis directions to any non-c-stick buttons

my opinion is the controller stuff requires quite a bit more architectural thinking than the keyboard stuff does, and it should be split out into a separate PR

briaguya-ai · 2024-11-22T19:42:50Z

src/controller/controldevice/controller/mapping/factories/ButtonMappingFactory.cpp

            mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_CLEFT,
-                                                                                 SDL_CONTROLLER_AXIS_RIGHTX, -1));
+                                                                                 LUS_DEFAULT_CONT_MAPPING_CLEFT, -1));
            if (isGameCube) {
                mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_CLEFT,
-                                                                              SDL_CONTROLLER_BUTTON_Y));
+                                                                              LUS_DEFAULT_CONT_MAPPING_CLEFT2));


these would benefit from more descriptive names. a consumer of LUS shouldn't have to know that CLEFT2 is for an SDLButton only on gamecube controllers and CLEFT is for an SDLAxisDirection

My bad, I didn't see the conditional gamecube statement.

briaguya-ai · 2024-11-22T19:45:15Z

cmake/cvars.cmake

+set(LUS_DEFAULT_KB_MAPPING_DLEFT KbScancode::LUS_KB_F CACHE STRING "")
+set(LUS_DEFAULT_KB_MAPPING_DRIGHT KbScancode::LUS_KB_H CACHE STRING "")
+
+set(LUS_DEFAULT_CONT_MAPPING_A SDL_CONTROLLER_BUTTON_A CACHE STRING "")


curious as to what others think about the naming here. considering these are used specifically for SDL, i think i'd prefer LUS_DEFAULT_SDL_MAPPING_* over LUS_DEFAULT_CONT_MAPPING_*

Sure! But for me I didn't realize at first that SDL referred to a physical gamepad.

briaguya-ai · 2024-11-22T19:52:02Z

src/controller/controldevice/controller/mapping/factories/ButtonMappingFactory.cpp

            }
            break;
        case BTN_CLEFT:
            mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_CLEFT,
-                                                                                 SDL_CONTROLLER_AXIS_RIGHTX, -1));
+                                                                                 LUS_DEFAULT_CONT_MAPPING_CLEFT, -1));


the -1 is important here as it is what determines that we mean left on the x axis instead of right. LUS_DEFAULT_CONT_MAPPING_CLEFT is only the axis part of AxisDirection

I was copying the name of BTN_CLEFT

you replaced SDL_CONTROLLER_AXIS_RIGHTX with LUS_DEFAULT_CONT_MAPPING_CLEFT

SDL_CONTROLLER_AXIS_RIGHTX is used for both left and right

MegaMech · 2024-11-22T22:47:37Z

my opinion is the controller stuff requires quite a bit more architectural thinking

Would any port actually want to override where the analogue stick goes? I think usually with the different brands of controllers the main issue is the face buttons are different like A/B and X/Square or whatever.

At the very least, with starship's release coming up, I think the point is moreso about having the ability to set what they need, and not necessarily a full blown implementation.

briaguya-ai · 2024-11-23T03:36:20Z

I think the point is moreso about having the ability to set what they need, and not necessarily a full blown implementation.

i still think the controller part should be a separate PR. the axis direction vs button stuff is more complex than the keyboard stuff. let's land the keyboard stuff and figure out what an MVP for controller stuff looks like after.

briaguya-ai · 2024-12-02T18:20:46Z

this shouldn't land as-is. the controller stuff either needs to be reworked or removed. the keyboard stuff seems ok as-is, but if we want to do controller stuff moving away from cmake vars is a better architectural decision.

briaguya-ai

briaguya-ai · 2024-12-04T19:21:59Z

even if we decide to add more robust (non-cmake-based) default setting in the future, having this shouldn't get in the way.

copying my previous note from discord here so it's in a more permanent place

if we want to go that route moving away from cmake vars makes sense. i think the move would be to update InitControlDeck to optionally take in default mappings and store them somewhere

the stored default mappings can't be the same class as the saved mappings, for example

libultraship/src/controller/controldevice/controller/mapping/factories/AxisDirectionMappingFactory.cpp

Lines 81 to 90 in 80c2c5e

AxisDirectionMappingFactory::CreateDefaultKeyboardAxisDirectionMappings(uint8_t portIndex, Stick stick) {

std::vector<std::shared_ptr<ControllerAxisDirectionMapping>> mappings = {

std::make_shared<KeyboardKeyToAxisDirectionMapping>(portIndex, stick, LEFT, LUS_KB_A),

std::make_shared<KeyboardKeyToAxisDirectionMapping>(portIndex, stick, RIGHT, LUS_KB_D),

std::make_shared<KeyboardKeyToAxisDirectionMapping>(portIndex, stick, UP, LUS_KB_W),

std::make_shared<KeyboardKeyToAxisDirectionMapping>(portIndex, stick, DOWN, LUS_KB_S)

};

return mappings;

}

has
std::make_shared<KeyboardKeyToAxisDirectionMapping>(portIndex, stick, LEFT, LUS_KB_A),
portIndex and stick aren't part of the hardcoded defaults, so we'd need a way to store just the direction and the keyboard code in that case.

SDLAxisDirectionToAxisDirectionMapping is a little more complicated

libultraship/src/controller/controldevice/controller/mapping/factories/AxisDirectionMappingFactory.cpp

Lines 104 to 112 in 80c2c5e

std::vector<std::shared_ptr<ControllerAxisDirectionMapping>> mappings = {

std::make_shared<SDLAxisDirectionToAxisDirectionMapping>(shipDeviceIndex, portIndex, stick, LEFT,

stick == LEFT_STICK ? 0 : 2, -1),

std::make_shared<SDLAxisDirectionToAxisDirectionMapping>(shipDeviceIndex, portIndex, stick, RIGHT,

stick == LEFT_STICK ? 0 : 2, 1),

std::make_shared<SDLAxisDirectionToAxisDirectionMapping>(shipDeviceIndex, portIndex, stick, UP,

stick == LEFT_STICK ? 1 : 3, -1),

std::make_shared<SDLAxisDirectionToAxisDirectionMapping>(shipDeviceIndex, portIndex, stick, DOWN,

stick == LEFT_STICK ? 1 : 3, 1)

we have some logic in there for setting the axis based on which stick it is, stick == LEFT_STICK ? 0 : 2 is saying "if this is the left stick, map axis 0, if it's the right stick, map axis 2" and likewise stick == LEFT_STICK ? 1 : 3 is saying ""if this is the left stick, map axis 1, if it's the right stick, use axis 3"

for button mappings we'd just need to make sure we pass in/store something that can be filtered by bitmask

for keyboard

libultraship/src/controller/controldevice/controller/mapping/factories/ButtonMappingFactory.cpp

Lines 73 to 130 in 80c2c5e

ButtonMappingFactory::CreateDefaultKeyboardButtonMappings(uint8_t portIndex, CONTROLLERBUTTONS_T bitmask) {

std::vector<std::shared_ptr<ControllerButtonMapping>> mappings;

switch (bitmask) {

case BTN_A:

mappings.push_back(std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_A, KbScancode::LUS_KB_X));

break;

case BTN_B:

mappings.push_back(std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_B, KbScancode::LUS_KB_C));

break;

case BTN_L:

mappings.push_back(std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_L, KbScancode::LUS_KB_E));

break;

case BTN_R:

mappings.push_back(std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_R, KbScancode::LUS_KB_R));

break;

case BTN_Z:

mappings.push_back(std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_Z, KbScancode::LUS_KB_Z));

break;

case BTN_START:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_START, KbScancode::LUS_KB_SPACE));

break;

case BTN_CUP:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_CUP, KbScancode::LUS_KB_ARROWKEY_UP));

break;

case BTN_CDOWN:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_CDOWN, KbScancode::LUS_KB_ARROWKEY_DOWN));

break;

case BTN_CLEFT:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_CLEFT, KbScancode::LUS_KB_ARROWKEY_LEFT));

break;

case BTN_CRIGHT:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_CRIGHT, KbScancode::LUS_KB_ARROWKEY_RIGHT));

break;

case BTN_DUP:

mappings.push_back(std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_DUP, KbScancode::LUS_KB_T));

break;

case BTN_DDOWN:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_DDOWN, KbScancode::LUS_KB_G));

break;

case BTN_DLEFT:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_DLEFT, KbScancode::LUS_KB_F));

break;

case BTN_DRIGHT:

mappings.push_back(

std::make_shared<KeyboardKeyToButtonMapping>(portIndex, BTN_DRIGHT, KbScancode::LUS_KB_H));

break;

}

return mappings;

}

we switch on bitmask, we'd want to just filter the stored defaults instead and then apply them to the appropriate portIndex

for SDL button mappings

libultraship/src/controller/controldevice/controller/mapping/factories/ButtonMappingFactory.cpp

Lines 132 to 222 in 80c2c5e

std::vector<std::shared_ptr<ControllerButtonMapping>>

ButtonMappingFactory::CreateDefaultSDLButtonMappings(ShipDeviceIndex shipDeviceIndex, uint8_t portIndex,

CONTROLLERBUTTONS_T bitmask) {

std::vector<std::shared_ptr<ControllerButtonMapping>> mappings;

auto sdlIndexMapping = std::dynamic_pointer_cast<ShipDeviceIndexToSDLDeviceIndexMapping>(

Context::GetInstance()

->GetControlDeck()

->GetDeviceIndexMappingManager()

->GetDeviceIndexMappingFromShipDeviceIndex(shipDeviceIndex));

if (sdlIndexMapping == nullptr) {

return std::vector<std::shared_ptr<ControllerButtonMapping>>();

}

bool isGameCube = sdlIndexMapping->GetSDLControllerName() == "Nintendo GameCube Controller";

switch (bitmask) {

case BTN_A:

mappings.push_back(

std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_A, SDL_CONTROLLER_BUTTON_A));

break;

case BTN_B:

mappings.push_back(

std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_B, SDL_CONTROLLER_BUTTON_B));

break;

case BTN_L:

if (!isGameCube) {

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_L,

SDL_CONTROLLER_BUTTON_LEFTSHOULDER));

}

break;

case BTN_R:

mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_R,

SDL_CONTROLLER_AXIS_TRIGGERRIGHT, 1));

break;

case BTN_Z:

mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_Z,

SDL_CONTROLLER_AXIS_TRIGGERLEFT, 1));

break;

case BTN_START:

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_START,

SDL_CONTROLLER_BUTTON_START));

break;

case BTN_CUP:

mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_CUP,

SDL_CONTROLLER_AXIS_RIGHTY, -1));

break;

case BTN_CDOWN:

mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_CDOWN,

SDL_CONTROLLER_AXIS_RIGHTY, 1));

if (isGameCube) {

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_CDOWN,

SDL_CONTROLLER_BUTTON_RIGHTSHOULDER));

}

break;

case BTN_CLEFT:

mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_CLEFT,

SDL_CONTROLLER_AXIS_RIGHTX, -1));

if (isGameCube) {

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_CLEFT,

SDL_CONTROLLER_BUTTON_Y));

}

break;

case BTN_CRIGHT:

mappings.push_back(std::make_shared<SDLAxisDirectionToButtonMapping>(shipDeviceIndex, portIndex, BTN_CRIGHT,

SDL_CONTROLLER_AXIS_RIGHTX, 1));

if (isGameCube) {

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_CRIGHT,

SDL_CONTROLLER_BUTTON_X));

}

break;

case BTN_DUP:

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_DUP,

SDL_CONTROLLER_BUTTON_DPAD_UP));

break;

case BTN_DDOWN:

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_DDOWN,

SDL_CONTROLLER_BUTTON_DPAD_DOWN));

break;

case BTN_DLEFT:

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_DLEFT,

SDL_CONTROLLER_BUTTON_DPAD_LEFT));

break;

case BTN_DRIGHT:

mappings.push_back(std::make_shared<SDLButtonToButtonMapping>(shipDeviceIndex, portIndex, BTN_DRIGHT,

SDL_CONTROLLER_BUTTON_DPAD_RIGHT));

break;

}

return mappings;

}

this actually opens up quite a bit of room for improvement

we currently switch on bitmask and then also have some logic to map x and y on gamecube controllers to c buttons like vc/gc oot/mm have. we could instead store defaults in a way that they can be generic and apply to any SDL controller, and also store specific defaults for known controllers

i feel like trying to do all of this in cmake would be a nightmare

MegaMech added 3 commits November 21, 2024 17:18

Update ButtonMappingFactory.cpp

661d915

Update AxisDirectionMappingFactory.cpp

fe0b77c

Update cvars.cmake

1af736c

MegaMech changed the title ~~Allow Default Keyboard Button Mappings~~ Implement Default Keyboard Button Mappings Nov 22, 2024

MegaMech mentioned this pull request Nov 22, 2024

Define Default Keyboard Button Mappings HarbourMasters/Starship#13

Closed

Update ButtonMappingFactory.cpp

f1e95c7

briaguya-ai reviewed Nov 22, 2024

View reviewed changes

cmake/cvars.cmake Outdated Show resolved Hide resolved

MegaMech changed the title ~~Implement Default Keyboard Button Mappings~~ Allow Keyboard Button Mapping Configuration In Games Nov 22, 2024

As per review

b5ed20a

briaguya-ai reviewed Nov 22, 2024

View reviewed changes

cmake/cvars.cmake Show resolved Hide resolved

briaguya-ai approved these changes Nov 22, 2024

View reviewed changes

Add sdl controller mapping

69c9378

MegaMech changed the title ~~Allow Keyboard Button Mapping Configuration In Games~~ Allow Ports To Configure Default Keyboard Button Mappings Nov 22, 2024

MegaMech changed the title ~~Allow Ports To Configure Default Keyboard Button Mappings~~ Allow Ports To Configure Default Button Mappings Nov 22, 2024

tidy

b91a592

MegaMech requested a review from briaguya-ai November 22, 2024 19:30

briaguya-ai requested changes Nov 22, 2024

View reviewed changes

Kenix3 approved these changes Dec 2, 2024

View reviewed changes

MegaMech added 3 commits December 4, 2024 11:28

Update cvars.cmake

d2c9852

Update ButtonMappingFactory.cpp

3324b1b

tidy

20e0d72

briaguya-ai approved these changes Dec 4, 2024

View reviewed changes

Kenix3 merged commit 6351314 into Kenix3:main Dec 9, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Ports To Configure Default Button Mappings #724

Allow Ports To Configure Default Button Mappings #724

MegaMech commented Nov 22, 2024 •

edited

Loading

briaguya-ai left a comment •

edited

Loading

Kenix3 commented Nov 22, 2024

MegaMech commented Nov 22, 2024

MegaMech commented Nov 22, 2024 •

edited

Loading

briaguya-ai commented Nov 22, 2024

briaguya-ai commented Nov 22, 2024

briaguya-ai left a comment

briaguya-ai Nov 22, 2024

MegaMech Nov 22, 2024 •

edited

Loading

briaguya-ai Nov 22, 2024

MegaMech Nov 22, 2024 •

edited

Loading

briaguya-ai Nov 22, 2024

MegaMech Nov 22, 2024

briaguya-ai Nov 23, 2024

MegaMech commented Nov 22, 2024 •

edited

Loading

briaguya-ai commented Nov 23, 2024

briaguya-ai commented Dec 2, 2024

briaguya-ai left a comment

briaguya-ai commented Dec 4, 2024

Allow Ports To Configure Default Button Mappings #724

Allow Ports To Configure Default Button Mappings #724

Conversation

MegaMech commented Nov 22, 2024 • edited Loading

Replaces hardcoded default button mappings with cmake variables

briaguya-ai left a comment • edited Loading

Choose a reason for hiding this comment

Kenix3 commented Nov 22, 2024

MegaMech commented Nov 22, 2024

MegaMech commented Nov 22, 2024 • edited Loading

briaguya-ai commented Nov 22, 2024

briaguya-ai commented Nov 22, 2024

briaguya-ai left a comment

Choose a reason for hiding this comment

briaguya-ai Nov 22, 2024

Choose a reason for hiding this comment

MegaMech Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

briaguya-ai Nov 22, 2024

Choose a reason for hiding this comment

MegaMech Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

briaguya-ai Nov 22, 2024

Choose a reason for hiding this comment

MegaMech Nov 22, 2024

Choose a reason for hiding this comment

briaguya-ai Nov 23, 2024

Choose a reason for hiding this comment

MegaMech commented Nov 22, 2024 • edited Loading

briaguya-ai commented Nov 23, 2024

briaguya-ai commented Dec 2, 2024

briaguya-ai left a comment

Choose a reason for hiding this comment

briaguya-ai commented Dec 4, 2024

MegaMech commented Nov 22, 2024 •

edited

Loading

briaguya-ai left a comment •

edited

Loading

MegaMech commented Nov 22, 2024 •

edited

Loading

MegaMech Nov 22, 2024 •

edited

Loading

MegaMech Nov 22, 2024 •

edited

Loading

MegaMech commented Nov 22, 2024 •

edited

Loading