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

Move hotkey definitions to mod yaml. #14379

Merged
merged 9 commits into from Dec 8, 2017

Conversation

Projects
None yet
6 participants
@pchote
Member

pchote commented Nov 18, 2017

This PR finishes the long-delayed (the first steps were made in #3991) migration of hotkeys out of the core engine and under the control of the individual mods.

This also removes the mod-specific hotkeys (naval, d2k queues, etc) from mods where they are not used.

@pchote pchote added this to the Next + 1 milestone Nov 18, 2017

@pchote pchote requested a review from penev92 Nov 18, 2017

@reaperrr reaperrr modified the milestones: Next + 1, Next release Nov 19, 2017

@jrb0001

This comment has been minimized.

Show comment
Hide comment
@jrb0001

jrb0001 Nov 22, 2017

Contributor

The upgrade rules for the utility seem to be missing.

Contributor

jrb0001 commented Nov 22, 2017

The upgrade rules for the utility seem to be missing.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 22, 2017

Contributor

There aren't any rule changes, so I doubt the mod updater will support this one.

Contributor

GraionDilach commented Nov 22, 2017

There aren't any rule changes, so I doubt the mod updater will support this one.

@jrb0001

This comment has been minimized.

Show comment
Hide comment
@jrb0001

jrb0001 Nov 22, 2017

Contributor

I would really prefer complete upgrade rules for the utility over spending a whole day to get all changes done correctly. But maybe that's just me.

Contributor

jrb0001 commented Nov 22, 2017

I would really prefer complete upgrade rules for the utility over spending a whole day to get all changes done correctly. But maybe that's just me.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 22, 2017

Member

This isn't something that can be automated, the hotkeys you need to define depend on your game design, which the upgrader doesn't know about.

I plan to include an upgrade rule for this as part of my planned upgrade rules rework, but that will be printing the steps that must be done by the modder.

Member

pchote commented Nov 22, 2017

This isn't something that can be automated, the hotkeys you need to define depend on your game design, which the upgrader doesn't know about.

I plan to include an upgrade rule for this as part of my planned upgrade rules rework, but that will be printing the steps that must be done by the modder.

Show outdated Hide outdated OpenRA.Game/HotkeyManager.cs
Show outdated Hide outdated OpenRA.Game/HotkeyManager.cs
@@ -132,8 +133,9 @@ public static IEnumerable<string> LinterHotkeyNames(MiniYamlNode widgetNode, Act
}
[ObjectCreator.UseCtor]
public ViewportControllerWidget(World world, WorldRenderer worldRenderer)
public ViewportControllerWidget(ModData modData, World world, WorldRenderer worldRenderer)

This comment has been minimized.

@penev92

penev92 Dec 4, 2017

Member

Isn't it a bit overkill to pass the entire ModData here (as well as 20 other places) for the HotkeyManager?

@penev92

penev92 Dec 4, 2017

Member

Isn't it a bit overkill to pass the entire ModData here (as well as 20 other places) for the HotkeyManager?

This comment has been minimized.

@pchote

pchote Dec 6, 2017

Member

We support dynamic arguments for only the logicArgs, World, WorldRenderer, ModData, and OrderManager. Adding a special case for the HotkeyManager (or anything else directly owned by one of these top-level types) IMO starts us down a slippery slope of people wanting us to support more and more unnecessary and possibly mod-specific state.

@pchote

pchote Dec 6, 2017

Member

We support dynamic arguments for only the logicArgs, World, WorldRenderer, ModData, and OrderManager. Adding a special case for the HotkeyManager (or anything else directly owned by one of these top-level types) IMO starts us down a slippery slope of people wanting us to support more and more unnecessary and possibly mod-specific state.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 6, 2017

Member

Fixed and rebased.

Member

pchote commented Dec 6, 2017

Fixed and rebased.

@penev92

penev92 approved these changes Dec 7, 2017

👍

@penev92 penev92 added the PR: Needs +2 label Dec 7, 2017

@abcdefg30 abcdefg30 merged commit 7ebea32 into OpenRA:bleed Dec 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Dec 8, 2017

@pchote pchote deleted the pchote:hotkey-settings branch Apr 28, 2018

@pchote pchote referenced this pull request Sep 9, 2018

Closed

Support per-mod hotkeys #7680

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment