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

SKY: Keymapper integration #2035

Merged
merged 1 commit into from Feb 20, 2020
Merged

SKY: Keymapper integration #2035

merged 1 commit into from Feb 20, 2020

Conversation

@bgK
Copy link
Member

bgK commented Feb 2, 2020

scummvm-00008

@bgK bgK changed the title SKY: Implement keymapper integration SKY: Keymapper integration Feb 2, 2020
act->addDefaultInputMapping("C+g");
shortcutsKeymap->addAction(act);

act = new Action(kStandardActionOpenDebugger, _("Open debugger"));

This comment has been minimized.

Copy link
@sev-

sev- Feb 2, 2020

Member

After looking at this. Could it possibly make sense to create a straightforward helper method, which would process the following:

keymap = {
  { kStandardActionOpenMainMenu,  _s("Open control panel"), kSkyActionOpenControlPanel, { "F5", "JOY_X", NULL } },
  { "RFAST", _s("Toggle really fast mode"), kSkyActionToggleReallyFastMode, { "C+g", NULL } },
 ...
};

This comment has been minimized.

Copy link
@bgK

bgK Feb 2, 2020

Author Member

It would certainly be possible to make such a helper. However I did not choose this approach because:

  • It makes the code harder to read. In the struct literal the names of the fields are not immediately visible.
  • It makes the code harder to edit. If you want to change one of the action in a way that does not fit the struct / helper to experiment, you have to rewrite in to use the Action class directly anyway.
@rsn8887

This comment has been minimized.

Copy link
Contributor

rsn8887 commented Feb 7, 2020

Why not map skip line to a joy button by default? I find it essential. Same goes for global default mapping in metaengine.

@rsn8887

This comment has been minimized.

Copy link
Contributor

rsn8887 commented Feb 8, 2020

These keymaps should include default game controller mappings too. Otherwise users will find their controllers not working in all the games that have custom mappings.

@bgK

This comment has been minimized.

Copy link
Member Author

bgK commented Feb 8, 2020

Why not map skip line to a joy button by default? I find it essential. Same goes for global default mapping in metaengine.

Can you suggest a button?

These keymaps should include default game controller mappings too. Otherwise users will find their controllers not working in all the games that have custom mappings.

What do you mean? That keymap is including game controller bindings, is it not?

@rsn8887

This comment has been minimized.

Copy link
Contributor

rsn8887 commented Feb 8, 2020

Why not map skip line to a joy button by default? I find it essential. Same goes for global default mapping in metaengine.

Can you suggest a button?

Globally I would suggest "JOY_X". But you are right here in this game that doesn't work because X opens the menu.

These keymaps should include default game controller mappings too. Otherwise users will find their controllers not working in all the games that have custom mappings.

What do you mean? That keymap is including game controller bindings, is it not?

I was going by the screenshot. Yes it is there.

@bgK bgK force-pushed the bgK:sky-keymap branch from 0f6de62 to 3e58ba9 Feb 16, 2020
@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 20, 2020

This looks to be in a good state to be merged. Are there any pending TODOs?

@bgK

This comment has been minimized.

Copy link
Member Author

bgK commented Feb 20, 2020

Ideally we could map skip and skip line on the same game controller button. However someone with a good knowledge on the engine would need to do the required changes. For now, users will have to remap skip line to the button of their choice.

@bgK bgK merged commit eba793f into scummvm:master Feb 20, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.