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

Allow hotkey selection for actors owned by multiple players #15813

Conversation

@dragunoff
Copy link
Contributor

commented Nov 11, 2018

The first commit adds the ability to select actors of the same type(class) owned by your allies (and by any player in the case of spectators). It works by double clicking on an actor or by using the hotkey with selected actor(s).

The second commit PR adds the ability to use selection hotkeys for multiple players. This feature is only relevant in spectator mode and can be useful for keeping track of the action (now that we have target lines for spectators).

Pressing q OR w will select units of the current render player or for everyone if we are spectating as "Everyone" or with shroud off.

Pressing w will select all units of the same type based on the currently selected units but it will work for multiple owners.

Closes #16044

@dragunoff dragunoff changed the title Allow selection by hotkeys for multiple players Allow hotkey selection for actors owned by multiple players Nov 12, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

Something is off with the q key. I did some testing in a team game with some ai players. After a while surrender to test as a spectator, when pressing q as a spectator it only selected units from one of my previous allies.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2018

@teinarss I can't test that now but I think it's because selection priority is given to the render player. So if you are spectating from the POV of one of your former allies then the q hotkey will select only their units. If you switch to the vision of another player then selection priority goes there.

I wonder if that should be changed in the case of team game spectators as there is actually no real "team view" - it's always from the perspective of one of the team members. I'd say we should leave it like it is as that's how the game works though I've already introduced a kind of exception here for "Everyone/Shroud off".

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2018

The problem was that i was spectating with shroud off/everyone.

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from 6e7e503 to 89a2201 Nov 27, 2018

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@teinarss That was good catch. I have amended the code to account for that case.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

OpenRA.Mods.Common/Widgets/WorldInteractionControllerWidget.cs:L251: [SA1408] Insert parenthesis within the conditional AND and OR expressions to declare the operator precedence.

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from 89a2201 to b215b06 Nov 27, 2018

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@abcdefg30 Fixed. I gotta get used to running make check.

@obrakmann obrakmann added this to the Next Release milestone Feb 2, 2019

@pchote pchote modified the milestones: Next Release, Next + 1 Feb 6, 2019

@pchote pchote removed the PR: For stable label Feb 6, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Changing things related to player visibility has always been risky, so considering that this is fixing a long-standing limitation rather than a recent regression its better to leave this to Next + 1. I don't want to risk delaying the final release this cycle considering the tight timing of the RAGL.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

@pchote I agree, let's defer this to Next+1.

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch 2 times, most recently from 0ea75d2 to 8a56e23 Feb 7, 2019

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from 8a56e23 to 53ed469 May 24, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Rebased.

@pchote pchote dismissed their stale review Jun 20, 2019

Code changed

@teinarss
Copy link
Contributor

left a comment

Some suggestions to prevent multiple enumerations.

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from 3eeabb7 to 31b15e4 Jul 1, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Rebased and streamlined this a little.

"Select by type" now works for all players, not only for the players in the original selection. And it doesn't work for allied players. So this feature only changes how things work for spectators (both in "Disable shroud" and "All players" mode).

@abcdefg30
Copy link
Member

left a comment

World.RenderPlayer ?? World.LocalPlayer not working worries me a bit, we use this pattern in several other places.

@@ -271,22 +274,30 @@ public override bool HandleKeyPress(KeyInput e)
if (!World.Selection.Actors.Any())
return false;

var selectedActors = World.Selection.Actors

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Jul 4, 2019

Member

This only seems to be used once? Imho you should combine the vars then:

var ownedActors = World.Selection.Actors
	.Where(x => !x.IsDead && eligiblePlayers.Contains(x.Owner))
	.ToList();
@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

World.RenderPlayer ?? World.LocalPlayer not working worries me a bit, we use this pattern in several other places.

I think I can make it work. It's the line below that seems bogus. That's code that I wrote a while back when I didn't understand the engine at all. I will look into it.

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from 31b15e4 to f9c95cb Jul 5, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

World.RenderPlayer ?? World.LocalPlayer not working worries me a bit, we use this pattern in several other places.

I think I can make it work. It's the line below that seems bogus. That's code that I wrote a while back when I didn't understand the engine at all. I will look into it.

Well, no 😃 World.RenderPlayer ?? World.LocalPlayer doesn't cut it here. See the comments up top at #15813 (review)

I have rebased that and updated this so that double clicking on actors works the same way as using the type selection hotkey (i.e. selects for all relevant players).

I'm not willing to spend much more time on this so I'm ok to close it if it's not considered merge ready.

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from f9c95cb to d74e4b7 Jul 20, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

Rebased and updated.

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from d74e4b7 to dab68be Jul 23, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Updated to address style nit.

@teinarss
Copy link
Contributor

left a comment

LGTM

if (unit != null && unit.Owner == (World.RenderPlayer ?? World.LocalPlayer))
// Players to be included in the selection (the viewer or all players in "Disable shroud" / "All players" mode)
var viewer = (World.LocalPlayer == null || World.LocalPlayer.Spectating) ? World.RenderPlayer : World.LocalPlayer;
var eligiblePlayers = viewer == null || (viewer.NonCombatant && viewer.Spectating) ? World.Players : new[] { viewer };

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Aug 3, 2019

Member

What is the viewer.NonCombatant check here for? In general I think this can be rewritten as

var viewer = World.RenderPlayer ?? World.LocalPlayer;
var eligiblePlayers = viewer == null || viewer.Spectating ? World.Players : new[] { viewer };

(I didn't spot any bugs with that, but maybe I oversaw something.)

This comment has been minimized.

Copy link
@dragunoff

dragunoff Aug 5, 2019

Author Contributor

The viewer.NonCombatant check is there to make sure that this is the special "Everyone" player. Not having it causes a bug when LocalPlayer is not null (the player has surrendered/lost) - things under shroud get selected:
изображение

I think I managed to find a way to use the standard viewer check. The conditions become a little lengthy so I guess pulling them in variables makes things clearer:

var viewer = World.RenderPlayer ?? World.LocalPlayer;
var isShroudDisabled = viewer == null || (World.RenderPlayer == null && World.LocalPlayer.Spectating);
var isEveryone = viewer.NonCombatant && viewer.Spectating;
var eligiblePlayers = isShroudDisabled || isEveryone ? World.Players : new[] { viewer };

This comment has been minimized.

Copy link
@dragunoff

dragunoff Aug 8, 2019

Author Contributor

A slight change and I think it works fine now. I didn't spot any bugs.

var isEveryone = viewer != null && viewer.NonCombatant && viewer.Spectating;

@dragunoff dragunoff force-pushed the dragunoff:feature/select-by-type-and-select-all-for-multiple-players branch from dab68be to aff1ec0 Aug 8, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Updated to use the standard viewer check: World.RenderPlayer ?? World.LocalPlayer.

I think this check is a little wonky, because LocalPlayer is null only for spectators that did not start the game as players. I.e. if a defeated player is spectating in "Shroud off" mode then viewer won't be null... That's why I have a ton of conditions after that check and also why I tried to do it with a different check initially.

@abcdefg30 abcdefg30 merged commit 017eca3 into OpenRA:bleed Aug 9, 2019

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.

Copy link
Member

commented Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.