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

WINTERMUTE: Add keymappings #2051

Merged
merged 5 commits into from Feb 19, 2020
Merged

WINTERMUTE: Add keymappings #2051

merged 5 commits into from Feb 19, 2020

Conversation

@lolbot-iichan
Copy link
Contributor

lolbot-iichan commented Feb 9, 2020

No description provided.

@lolbot-iichan lolbot-iichan requested a review from bgK Feb 9, 2020
@lolbot-iichan lolbot-iichan changed the title WINTERMUTE: Support keymapping (WIP) WINTERMUTE: Add keymapping (WIP) Feb 9, 2020
@lolbot-iichan lolbot-iichan changed the title WINTERMUTE: Add keymapping (WIP) WINTERMUTE: Add keymappings (WIP) Feb 9, 2020

Common::KeymapArray result = Keymap::arrayOf(engineKeyMap);

Common::String targetString = target;

This comment has been minimized.

Copy link
@bgK

bgK Feb 9, 2020

Member

The target is user-controlled. It's possible to edit it when adding a game. Is fetching the gameid from the configuration manager not working for you? Common::String gameId = ConfMan.get("gameid", target);

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Feb 9, 2020

Author Contributor

Thanks! Never noticed "gameid" property before.

@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_mapping branch from 9a02972 to 97b71a3 Feb 9, 2020
@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_mapping branch from c9f982a to 0586e42 Feb 13, 2020
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Feb 13, 2020

Current state of this PR:

  1. Standard in-game controls & game-specific controls for all 90+ games listed at https://wiki.scummvm.org/index.php?title=Wintermute/Controls are all added to detection.cpp, except for "onehelluvaday" demo, which features an action binded to Shift keypress. It seems that currently it's impossible to either add default input mapping for Shift, or assign it manually with GUI. Original inputs are all marked with "//original [keyboard|mouse|joy]" comments.

  2. Text input engine-hardcoded controls are still hardcoded, I don't plan to change it in this PR.

  3. Extra keyboard input mappings (UP+DOWN or PGUP+PGDN) are added for a couple of games. They are marked with "// extra keyboard". Ideas begind assigning extra keyboard keys:

  • original keymaps are never changed, if both UP+DOWN and PGUP+PGDN is already used, then nothing is added.
  • if game is using mouse wheel and does not use UP+DOWN or PGUP+PGDN pair -> map one of them to the mouse wheel
  1. Extra joystick mappings are added for all games. They are marked with "// extra joy". Ideas behind assigning JOY buttons:
  • All games are using LMB=JOY_A, RMB=JOY_B, ESC=JOY_X.
  • Game's most useful action (usually, HINTS or INV) is binded to JOY_Y.
  • If game has paired controls like scrolling, changing pages, walking, switching between 2 GUI types -> assign those to JOY arrows.
  • A few games features unpaired actions binded to JOY_LEFT and/or JOY_RIGHT (like, Face Noir's INV on JOY_LEFT and HINTS on JOY_Y). I'm not sure if it's ok or not to call commands with DPAD keys. Maybe JOY_R+Y would be nice to have, maybe not - not use...
  • Enter is curretly unmapped to JOY. It would be really nice if JOY_R+JOY_X would be allowed for Enter. Almost all of listed games are using Enter at windows like "Are you sure you want %s", but I'm not sure if it's a shortcut for OK button in all the games or some games are not fully playable without ability to press Enter. I'd better have a JOY mapped combo for it, just in case some game absolutely require it.
  • All cheats are unmapped to JOY. It would be really nice if JOY_R+JOY_B combo would be allowed for cheats & secrets. FoxTail has 2 cheats, for JOY_R+JOY_A would be useful as well.
  1. Extra mouse input mappings (MOUSE_WHEELUP+MOUSE_WHEELDOWN, MOUSE_MIDDLE) are added. They are marked with "// extra mouse". Ideas behind assigning extra MOUSE buttons:
  • original keymaps are never changed, if MOUSE_MIDDLE and/or MOUSE_WHEELUP+MOUSE_WHEELDOWN are already used, rules below does not apply.
  • If MOUSE_RIGHT is not actually used, it may be remapped (used only for "Wilma Tetris", which does not use RightClick event).
  • JOY_Y has some action -> assign MOUSE_ MIDDLE to the same action (except for Face Noir, where INV action is mapped to MOUSE_MIDDLE in original keymap, while I chose HINTS to be assigned to JOY_Y).
  • If game has paired controls like scrolling, changing pages, switching between 2 GUI types -> assign those action assigned to MOUSE_WHEELUP/MOUSE_WHEELDOWN arrows
@lolbot-iichan lolbot-iichan changed the title WINTERMUTE: Add keymappings (WIP) WINTERMUTE: Add keymappings Feb 13, 2020
@bgK

This comment has been minimized.

Copy link
Member

bgK commented Feb 13, 2020

Humongous work! We can merge this whenever you feel it's ready.

It seems that currently it's impossible to either add default input mapping for Shift, or assign it manually with GUI.

I'll look into fixing that, there is no reason not to be able to remap the modifier keys.

@bgK
bgK approved these changes Feb 13, 2020
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Feb 13, 2020

Humongous work! We can merge this whenever you feel it's ready.

Thanks! I guess I'll wait for #2052 to complete and will do a rebase, since my branch conflicts with that PR.

Meanwhile, current open questions:

  • Should I add JOY_R+JOY_X for Enter or leave it unmapped?
  • Should I add JOY_R+JOY_B for secrets or leave them unmapped?
    If at least one of those is yes, then what is the syntax? act->addDefaultInputMapping("JOY_R+JOY_X") does not seem to work for me (GUI does not list this combo).
@bgK

This comment has been minimized.

Copy link
Member

bgK commented Feb 14, 2020

Should I add JOY_R+JOY_X for Enter or leave it unmapped?
Should I add JOY_R+JOY_B for secrets or leave them unmapped?

At the moment, joystick button combos are not expressible using the keymapper. I suggest leaving those actions unmapped with a comment describing the ideal bindings. Users can map them to device specific buttons (triggers, stick press, right stick axes) if needed.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Feb 14, 2020

It seems that currently it's impossible to either add default input mapping for Shift, or assign it manually with GUI.

I'll look into fixing that, there is no reason not to be able to remap the modifier keys.

Done in 29dd15a.

act = new Action("RETURN", _("Confirm"));
act->setKeyEvent(KeyState(KEYCODE_RETURN, ASCII_RETURN));
act->addDefaultInputMapping("RETURN"); // original keyboard
//TODO: extra joy control, e.g. "JOY_R+JOY_X"

This comment has been minimized.

Copy link
@bluegr

bluegr Feb 16, 2020

Member

Please, indent all of these TODO comments properly (more follow below, no reason to repeat this comment)

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Feb 16, 2020

Author Contributor

Intended in line with code with tabs.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 16, 2020

Extraordinary work! Well done!

Could you please fix the indentation of the TODO comments? Other than that, it's an amazing effort, kudos!

@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_mapping branch from 0586e42 to 9e267aa Feb 16, 2020
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Feb 16, 2020

Rebased, removed debugger handling (it's already done at #2052), added Shift hotkey for One Helluva Day demo, moved TODOs to the right.

Done in 29dd15a.

Thanks, supported that at 9e267aa.

Could you please fix the indentation of the TODO comments?

Fixed at 9e267aa.

We can merge this whenever you feel it's ready.

I feel it's ready.

gameId == "rhiannon" ||
gameId == "shinestar"
) {
act = new Action("SKIP", _("Skip"));

This comment has been minimized.

Copy link
@ccawley2011

ccawley2011 Feb 16, 2020

Member
Suggested change
act = new Action("SKIP", _("Skip"));
act = new Action(kStandardActionSkip, _("Skip"));

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Feb 17, 2020

Author Contributor

Thanks, fixed! Maybe we should also introduce more Standard Actions for things like "HINT", "WTF", "ACTNXT", "PAGEDN", "SCRLUP", etc?

This comment has been minimized.

Copy link
@bgK

bgK Feb 18, 2020

Member

Yes, feel free to add some. I'm not convinced standard actions are very useful at the moment. We set the default bindings for mouse+keyboard and gamepad when we define the actions. The backend changing the defaults for the standard actions is very likely to result in nonsensical / conflicting keymaps.

One possible future change is the evolution of the virtual keyboard into some sort of more general touch oriented on-screen input method. Having standard actions in that context would be nice as we could bundle a set of corresponding icons.

engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
//TODO: extra joy control, e.g. "JOY_R+JOY_B"
gameKeyMap->addAction(act);

act = new Action("DBG", _("Debug print"));

This comment has been minimized.

Copy link
@ccawley2011

ccawley2011 Feb 16, 2020

Member

This has the same ID as the action for opening the debugger. I suggest renaming it to something else.

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Feb 18, 2020

Author Contributor

Renamed to "DBGTXT", thanks for pointing this.
@bluegr @ccawley2011 @bgK Should we introduce some convention to avoid things like this in the future? Like, "non-standard actions IDs should always be 6+ characters long"?

This comment has been minimized.

Copy link
@bgK

bgK Feb 18, 2020

Member

Well the action ids are namespaced by the keymap names. There was no real conflict here. The only issue is if a backend sets a default for a standard action that happens to have the same name as game action. I'm not sure it's worth the effort of enforcing a naming convention just for that.

engines/wintermute/detection.cpp Show resolved Hide resolved
engines/wintermute/detection.cpp Outdated Show resolved Hide resolved
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Feb 18, 2020

Fixed errors from above + splited "5ma||5ld||dirtysplit||projectjoe" keymap into two, since they slightly different original controls + fixed action ID collision at Alpha Polaris.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 19, 2020

Great work, once again! Thanks for fixing the pending issues. Merging

@bluegr bluegr merged commit 887db8a into scummvm:master Feb 19, 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.