Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#1210 closed enhancement (fixed)

[PATCH] Player should be able to train units over all buildings in a selection.

Reported by: michael Owned by: O.Davoodi
Priority: Should Have Milestone: Alpha 10
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

Players should be able to train units in multiple buildings in a selection at the same time. For instance, if multiple Barracks are selected (usually via double click), then if the player clicks the "train spearman" button, a spearman should be added to the queues of all of the selected barracks. This removes unnecessary clicks and helps prevent carpal tunnel and tendonitis in players (known from experience).

Attachments (3)

Commands.js.patch (1.8 KB ) - added by O.Davoodi 12 years ago.
input.js.patch (3.3 KB ) - added by O.Davoodi 12 years ago.
ticket-1210-2012-4-20.patch (3.7 KB ) - added by O.Davoodi 12 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Kieran P, 12 years ago

Component: Game engineSimulation
Priority: Must HaveNice to Have

comment:2 by O.Davoodi, 12 years ago

These files contain some modifications from #1215 & #1209. So it is better to apply the patches in those tickets first before this to prevent unwanted errors.

comment:3 by Kieran P, 12 years ago

Keywords: patch review added; buildings "unit training" multi-selection multiple selection removed
Owner: set to O.Davoodi
Summary: Player should be able to train units over all buildings in a selection.[PATCH] Player should be able to train units over all buildings in a selection.

comment:4 by Marco Amadori, 12 years ago

"This removes unnecessary clicks and helps prevent carpal tunnel and tendonitis in players (known from experience)."

Regarding this definition I think this could be related also with #1226, in 1226 there is the code to queue a unit in all selected buildings, in a positional (and easy to remember) way. ZXCV keys queue to 1234 kind of unit.

by O.Davoodi, 12 years ago

Attachment: Commands.js.patch added

by O.Davoodi, 12 years ago

Attachment: input.js.patch added

comment:5 by O.Davoodi, 12 years ago

Simplified the patch according to instructions from Philip.

comment:6 by leper, 12 years ago

Just noticed that this ticket is a dupe of #959 (maybe except the rally point setting, but that is already implemented).

Last edited 12 years ago by leper (previous) (diff)

comment:7 by leper, 12 years ago

Keywords: review removed

I took a look at your patch and you could improve it a bit:

  • You should use FilterEntitiesList(cmd.entities, player, controlAllUnits) in Commands.js instead of the if block.
  • Remove entity and replace it with entities altogether. This way you can use the loop code for both cases in Commands.js

Apart from those points it is looking good. It would be good if you could try to provide a single patch file that only includes changes relevant to this ticket and maybe timestamp your patch.

comment:8 by michael, 12 years ago

Priority: Nice to HaveShould Have

in reply to:  7 comment:9 by O.Davoodi, 12 years ago

Replying to leper:

I took a look at your patch and you could improve it a bit:

  • You should use FilterEntitiesList(cmd.entities, player, controlAllUnits) in Commands.js instead of the if block.
  • Remove entity and replace it with entities altogether. This way you can use the loop code for both cases in Commands.js

Apart from those points it is looking good. It would be good if you could try to provide a single patch file that only includes changes relevant to this ticket and maybe timestamp your patch.

Sorry for the delay. About first point:That should work. but i did that because we need the player be able to control the first selected building (which is entity or entities[0]). But there is also a problem. It seems that FilterEntitiesList can't be used in Command.js so I hardcoded the thing.

If we remove "entity" it causes the AI to stop training units. That needs a much broader change that needs quantumstate's approval.

by O.Davoodi, 12 years ago

Attachment: ticket-1210-2012-4-20.patch added

comment:10 by O.Davoodi, 12 years ago

Feel free to change the patch if necessary if i didn't return for so long.

comment:11 by Kieran P, 12 years ago

Keywords: review added

comment:12 by leper, 12 years ago

Resolution: fixed
Status: newclosed

In 11592:

Allow training in all selected buildings. Fixes #1210.

comment:13 by leper, 12 years ago

Keywords: review removed

Thanks for your patch. I rewrote most of it as some of the code was changed by r11584 and my version adds the ability to add to batches if we have more than one building selected.

Note: See TracTickets for help on using tickets.