Added selection classes #8212

matija-hustic · 2015-05-21T21:25:17Z

Option 3 from #7469 that would support the type of functionality proposed by @MustaphaTR:

http://ares-developers.github.io/Ares-docs/new/typeselect.html

Basically whenever the standard select-by-type functionality is supposed to be extended to select any custom class of selectable actors, these custom classes can be defined using the Class property of the Selectable trait.

If the basic idea is fine, I'd define any remaining selection classes in the YAMLs.

phrohdoh · 2015-05-21T22:53:28Z

The class ought to default to the actor's type name IMO.

matija-hustic · 2015-05-22T23:14:18Z

Done that @phrohdoh. Simplified stuff a lot.

phrohdoh · 2015-05-23T01:03:41Z

That means you can remove the redundant yaml lines.

matija-hustic · 2015-05-23T01:14:35Z

Not sure what you mean... Defaulting selection class to actor name only simplified the code. Hasn't changed anything for what is needed in YAMLs.

Did you mean defaulting on the Tooltip.Name instead of the actor name?

penev92 · 2015-05-23T01:27:51Z

OpenRA.Game/Traits/Selectable.cs

@@ -22,11 +22,16 @@ public class SelectableInfo : ITraitInfo
 		public readonly int[] Bounds = null;
 		[VoiceReference] public readonly string Voice = null;

+		[Desc("All units having the same selection class specified will be selected with select-by-type commands (e.g. double-click).")]


You should mention that it defaults to the actor's name.

penev92 · 2015-05-23T01:30:45Z

YAML changes for D2k vehicles will be needed. I'm not sure about others (the RA cases you added), though.

matija-hustic · 2015-05-23T01:35:36Z

I'll remove those. So guys, opinions on using Tooltip.Name as default and actor name as fallback in case actor doesn't implement Tooltip or doesn't define Tooltip.Name? That way even those YAML entries for d2k would be rendered unnecessary. Although wouldn't be the cleanest of solutions...

phrohdoh · 2015-05-23T02:38:19Z

Just stick with the actor type as the default. This isn't exposed to the player so doesn't need to be (and shouldn't be coupled with) something as 'pretty' as the tooltip name.

penev92 · 2015-05-23T09:56:51Z

mods/d2k/rules/starport.yaml

@@ -21,7 +21,6 @@ trike.starport:
 	Inherits: trike
 	Buildable:
 		Queue: Starport
-		Prerequisites: starport


This looks out of place in this PR?

Yes. Must make a separate PR?

I don't understand why you're removing it to begin with.

It is redundant. Compare with the other entries - if they're purchasable in the Starport queue, which is only activated once you actually build the Starport, there is no need to put starport in the prerequisites.

Oh, OK, sorry, I see your point. You can keep it here then.

penev92 · 2015-05-23T10:00:16Z

If I understand the whole idea correctly, the Selectable: Class: should go on the startport actors for this to work and maybe on the normal ones for clarity.

matija-hustic · 2015-05-23T10:02:49Z

Yeah. See here

http://ares-developers.github.io/Ares-docs/new/typeselect.html

phrohdoh · 2015-05-23T14:49:12Z

~~You can use a Defaults entry or a parent definition if necessary. This is at the yaml level now though.~~

~~Above I was referring to engine plumbing.~~

Edit: Never mind.

penev92 · 2015-05-31T15:13:18Z

OK, I have no idea why you added those classes in RA, but they seem redundant.
Also D2k needs them on the actor definitions in starport.yaml, not the ones in vehicles.yaml.

matija-hustic · 2015-05-31T15:18:11Z

The Selectable trait gets inherited and so does the Class property when explicitly defined. That way, anyone who inherits from e.g. trike will inherit and share the same Selectable.Class thus making them equivalent as far as selectability is concerned.
I've added this thing for the civilians in order for them all to be in the same class. Try making and selling a couple of Tech Labs and see how fun it is when you get all kinds of different civilians and no quick way to select them all.

penev92 · 2015-05-31T15:25:19Z

Ah, good thinking with the inheritance, although that makes it quite implicit.

matija-hustic · 2015-05-31T15:26:35Z

I can add a note about this to the description of the Class property?

penev92 · 2015-05-31T15:27:47Z

No, it's good. I just forgot that the starport actors inherit from those. I guess only thing left is to test ingame now.

penev92 · 2015-05-31T17:06:16Z

OpenRA.Game/Widgets/WorldInteractionControllerWidget.cs

-							var newSelection2 = SelectActorsInBox(World, worldRenderer.Viewport.TopLeft, worldRenderer.Viewport.BottomRight,
-								a => a.Owner == unit.Owner && a.Info.Name == unit.Info.Name);
+							var newSelection = SelectActorsInBox(World, worldRenderer.Viewport.TopLeft, worldRenderer.Viewport.BottomRight,
+								a => a.Owner == unit.Owner && a.Trait<Selectable>().Class == unit.Trait<Selectable>().Class);


Repeated trait lookups is not the best thing to do, so add a var class = unit.Trait<Selectable>().Class; above and check against that here.
We may even want a GetSelectionClass() actor extention, because this is heavily abused.

matija-hustic · 2015-05-31T17:16:36Z

Fixed commented code @penev92.

penev92 · 2015-05-31T17:18:27Z

OpenRA.Game/Widgets/WorldInteractionControllerWidget.cs

 						.Where(x => x.Owner == player)
-						.Select(a => a.Info);
+						.Select(a => a.Trait<Selectable>().Class);


.ToHashSet() please

matija-hustic · 2015-06-01T02:09:58Z

@penev92, after we talked in IRC, I felt more comfortable changing other things that seem that could use some housekeeping. Could you check if this is more to your liking?

matija-hustic · 2015-06-01T02:12:47Z

OpenRA.Game/Widgets/WorldInteractionControllerWidget.cs

+				// sc == null means that units, that meet all other criteria, get selected
+				return s != null && s.Info.Selectable && (sc == null || sc.Contains(s.Class));
+			});
+		}


I can probably change something with the above three functions, but too tired right now to think clearly.

Can sc really be null here?
Looks like worst-case it can be empty, but even that shouldn't be possible.

Think caryalls in d2k.

Can we get better variable names please?

penev92 · 2015-06-01T15:59:21Z

Code looks good, and it works ingame 👍
You should squash the commits now.

matija-hustic · 2015-06-01T16:07:08Z

Squashed.

Mailaender · 2015-06-04T04:21:49Z

Needs a rebase.

matija-hustic · 2015-06-07T16:41:13Z

Changed variable names as @phrohdoh suggested. Rebased.

phrohdoh · 2015-06-08T13:00:04Z

OpenRA.Game/Traits/Selectable.cs

@@ -33,6 +38,10 @@ public Selectable(Actor self, SelectableInfo info)
 		{
 			this.self = self;
 			Info = info;
+			if (string.IsNullOrEmpty(info.Class))


This could become ternary, but that doesn't change the actual logic so it isn't necessary.

phrohdoh · 2015-06-08T14:01:51Z

Please squash the Typos commit so this can be tested.

matija-hustic · 2015-06-08T14:03:37Z

Squashed.

abcdefg30 · 2015-06-12T23:53:30Z

Looks also good to me. 👍 / ✅

Added selection classes

abcdefg30 · 2015-06-12T23:57:56Z

Changelog

penev92 · 2015-06-13T00:08:29Z

Changelog #2

matija-hustic force-pushed the selectable_classes branch from ae228b1 to 7f2b92c Compare May 21, 2015 21:39

matija-hustic force-pushed the selectable_classes branch 4 times, most recently from 9a9971f to 36b6db4 Compare May 22, 2015 23:04

penev92 reviewed May 23, 2015
View reviewed changes

matija-hustic force-pushed the selectable_classes branch 3 times, most recently from 8ba05d8 to 25df78e Compare May 26, 2015 22:12

penev92 added the PR: Changes requested label May 31, 2015

penev92 removed the PR: Changes requested label May 31, 2015

penev92 reviewed May 31, 2015
View reviewed changes

matija-hustic reviewed Jun 1, 2015
View reviewed changes

matija-hustic force-pushed the selectable_classes branch from 42442ed to e019d55 Compare June 1, 2015 09:41

penev92 added the PR: Needs +2 label Jun 1, 2015

matija-hustic force-pushed the selectable_classes branch from e019d55 to ceacdeb Compare June 1, 2015 16:06

phrohdoh added the PR: Changes requested label Jun 2, 2015

Mailaender added the PR: Rebase me! label Jun 4, 2015

matija-hustic force-pushed the selectable_classes branch from ceacdeb to c201a76 Compare June 7, 2015 16:39

abcdefg30 removed PR: Changes requested PR: Rebase me! labels Jun 7, 2015

phrohdoh reviewed Jun 8, 2015
View reviewed changes

Touched up selection functionality.

e965a0d

matija-hustic force-pushed the selectable_classes branch from b89b6a4 to e965a0d Compare June 8, 2015 14:03

abcdefg30 added a commit that referenced this pull request Jun 12, 2015

Merge pull request #8212 from matija-hustic/selectable_classes

b98d98c

Added selection classes

abcdefg30 merged commit b98d98c into OpenRA:bleed Jun 12, 2015

penev92 mentioned this pull request Jun 13, 2015

D2k actor duplicates from Starport break the Select Units hotkey #7469

Closed

matija-hustic mentioned this pull request Jun 14, 2015

Fixes regression with selection classes #8483

Merged

matija-hustic deleted the selectable_classes branch June 26, 2015 00:47

Added selection classes #8212

Added selection classes #8212

Conversation

matija-hustic commented May 21, 2015

phrohdoh commented May 21, 2015

matija-hustic commented May 22, 2015

phrohdoh commented May 23, 2015

matija-hustic commented May 23, 2015

Choose a reason for hiding this comment

penev92 commented May 23, 2015

matija-hustic commented May 23, 2015

phrohdoh commented May 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penev92 commented May 23, 2015

matija-hustic commented May 23, 2015

phrohdoh commented May 23, 2015

penev92 commented May 31, 2015

matija-hustic commented May 31, 2015

penev92 commented May 31, 2015

matija-hustic commented May 31, 2015

penev92 commented May 31, 2015

Choose a reason for hiding this comment

matija-hustic commented May 31, 2015

Choose a reason for hiding this comment

matija-hustic commented Jun 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penev92 commented Jun 1, 2015

matija-hustic commented Jun 1, 2015

Mailaender commented Jun 4, 2015

matija-hustic commented Jun 7, 2015

Choose a reason for hiding this comment

phrohdoh commented Jun 8, 2015

matija-hustic commented Jun 8, 2015

abcdefg30 commented Jun 12, 2015

abcdefg30 commented Jun 12, 2015

penev92 commented Jun 13, 2015