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

Add selection modes - "Select All" and "Select Low Priority" #15839

Merged
merged 2 commits into from Jun 29, 2019

Conversation

@dragunoff
Copy link
Contributor

commented Nov 19, 2018

This is a proposed implementation of the modifiers discussed in #14694. It adds two modes that alter the priority calculation. The modes are activated by holding down modifiers keys when selecting by mouse dragging.

Two new fields are added to the Selectable trait:
- IncludeInSelectAllMode = true
- IncludeInSelectLowPriorityMode = false

One new field is added to Selectable:

  • PriorityModifiers with valid values None (default), Ctrl and Alt

Using the new field I have impleted two selection modes as YAML templates. This is how they work:

  • Flat Mode: holding down Ctrl selects all actors that inherit ^FlatSelectionMode. Buildings are not included but would still be selected if there are no higher priority actors.
    Example use case: Retreating all of your units (combat and non-combat).
    Example use case: Moving together a mixed group of units (combat and non-combat).
  • Low Priority Mode: holding down Alt selects all actors that inherit ^LowPrioritySelectionMode. This is added to all non-combat units.
    Example use case: Retreating non-combat units (harvesters or engies for example).
    Example use case: Selecting low priority units among blobs (engies, harvesters, MCV).

изображение

Remarks:

  • I was thinking about adding a third modifier - Ctrl + Alt but decided that this is good enough for now
  • This can be used by modders to set up their own selection modes ("Aircraft Mode" for example)
  • It doesn't work with hotkey selection (hotkeys with modifiers are considered a different hotkey).
  • Possible hotkey implementation will conflict with the default bindings for bookmarks (Ctrl+q and Alt+q).
  • The selection modes are not exposed in any way in the UI. This should be addressed in another PR.
mods/ts/rules/defaults.yaml Outdated Show resolved Hide resolved

@dragunoff dragunoff force-pushed the dragunoff:feature/selection-modes branch from 17e1b28 to 76c39eb Nov 20, 2018

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

I think it's a good approach.

Related to #3387, #14769

@dragunoff dragunoff force-pushed the dragunoff:feature/selection-modes branch from 76c39eb to 0f25a7b Dec 26, 2018

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2018

Rebased and updated.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Would this need an upgrade rule for 3rd party mods and maps?

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Would this need an upgrade rule for 3rd party mods and maps?

This isn't changing any default behaviour, right? If so, then no.

@dragunoff dragunoff force-pushed the dragunoff:feature/selection-modes branch from 0f25a7b to caa8b3c Mar 12, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Would this need an upgrade rule for 3rd party mods and maps?

This isn't changing any default behaviour, right? If so, then no.

It's not changing the default behavior. It's just that the new feature won't be very usable without the YAML setup.

@pchote
Copy link
Member

left a comment

Behaves well in game and code looks mostly fine. Just a couple of requests:

OpenRA.Game/SelectableExts.cs Outdated Show resolved Hide resolved
OpenRA.Game/Traits/Selectable.cs Outdated Show resolved Hide resolved

@dragunoff dragunoff force-pushed the dragunoff:feature/selection-modes branch from caa8b3c to 3abf382 Mar 31, 2019

OpenRA.Game/SelectableExts.cs Outdated Show resolved Hide resolved
OpenRA.Game/SelectableExts.cs Outdated Show resolved Hide resolved

@dragunoff dragunoff force-pushed the dragunoff:feature/selection-modes branch from 3abf382 to 8298ce9 Mar 31, 2019

@matjaeck
Copy link
Contributor

left a comment

Tested in RA, good job @dragunoff.

"Valid values are None (priority is not affected by modifiers)",
"FlatPriority (priority is set to 10 when Ctrl pressed) and",
"LowPriority (priority of actors without this value is set to 0 when Alt pressed).")]
public readonly SelectionPriorityModes SelectionPriorityModes = SelectionPriorityModes.FlatPriority;

This comment has been minimized.

Copy link
@reaperrr

reaperrr Apr 11, 2019

Contributor

I might be missing something here, but doesn't FlatPriority change the default behavior compared to bleed? Shouldn't this be None instead?

This comment has been minimized.

Copy link
@dragunoff

dragunoff Apr 12, 2019

Author Contributor

Either of the values here changes the default behavior when a modifier key (Ctrl or Alt) is pressed. Even if it's None the priority would be set to 0 when Ctrl is pressed during a selection.

Perhaps the description here is not accurate. Actually I've been struggling to come up with good names and description for this feature. Because even though the modes are called "flat" and "low priority" they are not tied to the priority of an actor. They may as well be called "select ctrl" and "select alt"...

This comment has been minimized.

Copy link
@dragunoff

dragunoff Apr 12, 2019

Author Contributor

A value of None probably makes more sense as a default. I didn't do it like that because most of the actors are included in the "flat" mode and I wanted to spare some YAML lines...

This comment has been minimized.

Copy link
@pchote

pchote Apr 12, 2019

Member

IMO the difficulty in describing this probably reflects that this isn't exposed to the yaml in the best way.

What if we changed the behaviour such that None, the default, keeps things as they are on bleed. Then expose usefully named templates that can be inherited, e.g. Inherits@SELECTIONPRIORITY: ^SupportActorSelectionPriority?

This comment has been minimized.

Copy link
@dragunoff

dragunoff Apr 12, 2019

Author Contributor

I agree that it's probably not the best way to expose it in YAML. I am open to suggestions. However I don't understand the template and inheritance idea -- could you provide an example or point me to a similar implementation of another feature?

This comment has been minimized.

Copy link
@pchote

pchote Apr 12, 2019

Member

The ^AutoTarget* templates that most actors implement.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

What's the status here @dragunoff?

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@pchote I definitely want to finish this. But it turned out to be more complex than I thought. I think this warrants some more discussion.

The problem for me is that the modes are not tied to the selection priority, so calling them "flat" and "low priority" doesn't make sense... I thought about just calling them "default", "mode 1", "mode 2", "mode 3" and then let actors have different priority for each mode (falling back to the default priority). We can even let mods define the names for these modes and if the want to even use them. That would make the feature very flexible.

# A cobmat unit, priority lowered in MODE2
Priority: 10
Priority@MODE1: 10
Priority@MODE2: 0

# A support unit, priority raised in MODE1
Priority: 6
Priority@MODE1: 10

# A high priority combat unit,  priority lowered in MODE2 and raised in MODE3
Priority: 10
Priority@MODE2: 0
Priority@MODE3: 20

I'm not sure about the YAML notation, just trying to explain the idea.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

How important is the "low priority" modifier? Can we get most of the gameplay benefits without the complexity by ignoring all the per-actor definitions and implementing this as a global "ignore priority" modifier during selection?

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Yes, ignoring priority is fine - that way we can keep the normal priority based selection and introduce modified selections that force raise/lower the priority. The naming problem remains though.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

What is there to name at this point? We wouldn't need to expose anything to the mod rules.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

What is there to name at this point? We wouldn't need to expose anything to the mod rules.

How are we going to then define which actors should be included in which selection? 🤔

I may be misunderstanding your idea...

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

My suggestion was to have it include all actors that are controllable by the player.

If we really do need a distinction for buildings we could call have something like "ValidForFlattenedPriority: (true|false)".

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Ah, I see - you suggest dropping the "low priority" mode entirely. I'd really like to make it work though ☺️ And the naming problem won't go away - what would stop modders from repurposing the "flat" mode into something else? That may not be a huge problem but it bugs me...

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

My motivation with exploring these other options is so that we can find a way to get this merged in time for the playtest branching.

@dragunoff dragunoff force-pushed the dragunoff:feature/selection-modes branch from 8298ce9 to 8c3a6ae May 8, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Okay, so I have simplified this a little bit. And I'm finally (mostly) satisfied with the result.

I have dropped the selection modes names from the C# code and just named the flags after the actual modifier keys (Ctrl and Alt). This solves the naming problem and gives modders the freedom to use the feature however they want. For the default mods I've created two selection modes that are implemented as YAML templates which actors inherit.

I've also dropped my original idea for the "Low Priority" mode (where selection priority was honored) - now modifiers simply raise the selection priority to int.MaxValue thus always flattening the priority for affected actors.

@dragunoff dragunoff force-pushed the dragunoff:feature/selection-modes branch from 8c3a6ae to 6c4b372 Jun 25, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Rebased on latest bleed.

@abcdefg30
Copy link
Member

left a comment

Seems like you missed engineers (and thumpers?) in D2k. Looks good to me otherwise.

@dragunoff

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Seems like you missed engineers (and thumpers?) in D2k. Looks good to me otherwise.

It is intentional as they don't have a lower selection priority in D2k. This should be tackled as part of #16662

@teinarss
Copy link
Contributor

left a comment

Tested in all the mods and the code looks fine

@teinarss teinarss merged commit e230546 into OpenRA:bleed Jun 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
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
7 participants
You can’t perform that action at this time.