Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3823 closed task (fixed)

[PATCH] Correctly handle commands for multiple units

Reported by: Itms Owned by: Imarok
Priority: Must Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by Imarok)

When there are multiple units in the selection, we're facing several problems, which arise from the fact that the GUI uses the first unit of the selection (this first unit being the one with the lowest entity ID).

Problems include:

  • The back-to-work button does not always show up (workaround included in r17880 and reverted afterwards)
  • #2205
  • #3302
  • Building structures when female citizens and soldiers are selected together

and possibly others.

This should be fixed by passing selection instead of unitEntState from unit_commands to selection_panels and change the code depending on that. (comment:8) (One should keep in mind, selection only contains the entity ids, so one has to use GetExtendedEntityState or GetEntityState)

Attachments (5)

3823.patch (2.4 KB ) - added by svott 8 years ago.
3823_rebased.patch (2.5 KB ) - added by Imarok 8 years ago.
rebased
upgrade.patch (2.1 KB ) - added by Imarok 8 years ago.
Show the correct number of resources when upgrading multiple entities
upgrade2.patch (2.1 KB ) - added by Imarok 8 years ago.
replace forEach with for
upgradev3.patch (2.0 KB ) - added by Imarok 8 years ago.
style fix

Download all attachments as: .zip

Change History (36)

comment:1 by Itms, 8 years ago

In 17885:

Revert r17880, refs #3745, refs #3823

comment:2 by svott, 8 years ago

Keywords: patch added
Milestone: BacklogAlpha 21
Owner: set to svott
Summary: Correctly handle commands for multiple units[Patch] Correctly handle commands for multiple units

I solved the back-to-work problem again in a more general way for the middle panel, so that is should also fix problems with other commands

Further:

#2205: I think that is another problem. Related to the middle panel: there is not enough space for all icons when units and structures are selected together. Thus, it needs a redesign in another task.

#3302: It looks like that it has another cause that is not related to here.

last bullet point: I don't see the problem. maybe it is solved in another ticket already?

by svott, 8 years ago

Attachment: 3823.patch added

comment:3 by svott, 8 years ago

Keywords: review added
Summary: [Patch] Correctly handle commands for multiple units[PATCH] Correctly handle commands for multiple units

comment:4 by sanderd17, 8 years ago

Some remarks:

  • It's not common to mark an argument as "UNUSED" in JS.
  • It would be nice if you assured us that there's no problem with passing the same selection list every time (it could be modified by some methods).
  • When you alter the arguments of a function that has javaDoc, you should alter the documentation too.

comment:5 by Stan, 8 years ago

Any updates ?

comment:6 by bb, 8 years ago

How does changing L231 of input.js to:

if (r && r.possible) // return true if it's possible for one of the entities

effect this ticket? see #252 and #3492

comment:7 by svott, 8 years ago

@bb: I don't know. Don't? @sanderd17: well, is there another way to get rid of unused arguments? I don't see a possibility in that case because the (general) getItem function needs a unitEntState argument at first place

how can I assure that there is no problem with passing the list? (There are other entries (g_SelectionPanels.Gate i.e.) those use the "selection" parameter as well)

by Imarok, 8 years ago

Attachment: 3823_rebased.patch added

rebased

comment:8 by elexis, 8 years ago

As far as I can see unitEntState shouldn't be passed at all from unit_commands to selection_panels. In some cases it might happen to be a check that works as the player can't select multiple units if the first selected one isn't an own unit. But if the GUI should take all selected units into account, then g_Selection must be processed in every case.

comment:9 by Imarok, 8 years ago

Description: modified (diff)
Keywords: patch review removed
Milestone: Alpha 21Backlog
Summary: [PATCH] Correctly handle commands for multiple unitsCorrectly handle commands for multiple units

comment:10 by Imarok, 8 years ago

Description: modified (diff)

comment:11 by Imarok, 8 years ago

Description: modified (diff)

comment:12 by Imarok, 8 years ago

Owner: changed from svott to Imarok

comment:13 by Imarok, 8 years ago

Keywords: patch rfc added
Milestone: BacklogAlpha 21
Summary: Correctly handle commands for multiple units[PATCH] Correctly handle commands for multiple units

comment:14 by Imarok, 8 years ago

refs #3288

comment:15 by bb, 8 years ago

Input.js L324 Can't you use just for (let entstate in entstates) maybe you need to adapt the loop a bit for that. (Not 100% sure about this)

L58-86 merge the switch with the if else system.


In multiple selection the holes in research icons s removed, only in multiple selection. It is rather strange in my opinion that there are holes in single selection.

When selecting a (Briton, not sure if that matters) barrack + fortress, I see no tech neither barracks or fortress. With fortress + blacksmith I only get the techs of the first one selected. (It would also be nice to merge the icons for the champions, so that is one icon for each champ, but that is another issue. It is just template naming IIRC)

The unit_action buttons does not fit on the bottom panel when we have a back to work + guard + unguard button (+ some more ofc). Not sure how to fix this. This is even getting worse when we also select a building so we get the "focus on really point" and ungarrison button too...

If I raise an alert and then ungarrison one of my woman (or other unit garrisoned by the alert) the woman on't garrisson again when I increase the alert or when ending the alert and raising a new one. They will only listen to a new alert when the unit is ungarrisoned by an "end of alert" (and that is not necessarily the alert when they got garrisoned)

Last edited 8 years ago by bb (previous) (diff)

in reply to:  15 comment:16 by Imarok, 8 years ago

Replying to bb:

Input.js L324 Can't you use just for (let entstate in entstates) maybe you need to adapt the loop a bit for that. (Not 100% sure about this)

Could work (if you mean L324 of selection_details.js)

L58-86 merge the switch with the if else system.

(I guess you mean selection_panels.js): That are two different functions, but I could indeed use a switch case for that too.


In multiple selection the holes in research icons s removed, only in multiple selection. It is rather strange in my opinion that there are holes in single selection.

The holes in single selection are there so that every tech is always at the same position

When selecting a (Briton, not sure if that matters) barrack + fortress, I see no tech neither barracks or fortress. With fortress + blacksmith I only get the techs of the first one selected. (It would also be nice to merge the icons for the champions, so that is one icon for each champ, but that is another issue. It is just template naming IIRC)

That's because there are more then 8(one row) units to train, so paired techs would conflict with the units buttons. (I think I'll improve that)

The unit_action buttons does not fit on the bottom panel when we have a back to work + guard + unguard button (+ some more ofc). Not sure how to fix this. This is even getting worse when we also select a building so we get the "focus on really point" and ungarrison button too...

Not really part of that ticket, as it already appears now with a single unit and I neither know how this should be fixed

If I raise an alert and then ungarrison one of my woman (or other unit garrisoned by the alert) the woman on't garrisson again when I increase the alert or when ending the alert and raising a new one. They will only listen to a new alert when the unit is ungarrisoned by an "end of alert" (and that is not necessarily the alert when they got garrisoned)

That's probably a bug(Maybe intended?) in the alert raiser system and already appears in a20 with a single CC (so should be subject of an other ticket)

comment:17 by Imarok, 8 years ago

made the changes (link is just the same: https://github.com/Imarok/0ad/tree/3823_multiple_selection)

Edit: This patch will fix #3492, #3288, #3668, #3302 too. (Maybe some more)

Last edited 8 years ago by Imarok (previous) (diff)

comment:18 by bb, 8 years ago

Selection_details.js L58-69 can't we return out of the function instead of breaking out of the switch?

L58-85 switches have a tabulation less inside see http://trac.wildfiregames.com/wiki/Coding_Conventions

L228-30 not sure about that indentation, shouldn't it be all aligned and also not sure if that newline is needed.

Can L267-8 be moved into the nestled loop (L263) like:

if (info)
{
    info.name = command;
    commands.push(info);
    break;
}

and then skip the check at L265-6? (same for L199-208)

L735-39 isn't that a tab too many?

L745-47 why is that comment in the middle of a statement? and align with spaces instead of a tab here.

L1095 trailing whitespace

When selection a blacksmith and a fortress only the techs of the first one are selected due to that not all techs fit on that one line, but the line isn't full either. Maybe the bar should just be filled up with as many tech's as possible, like it is done with units? (yes with maur it is possible to get the 3 lines full...)

in reply to:  18 comment:19 by Imarok, 8 years ago

Replying to bb:

L745-47 why is that comment in the middle of a statement?

Because it just applies to the part after this comment

and align with spaces instead of a tab here.

Why should I use spaces here?

When selection a blacksmith and a fortress only the techs of the first one are selected due to that not all techs fit on that one line, but the line isn't full either. Maybe the bar should just be filled up with as many tech's as possible, like it is done with units? (yes with maur it is possible to get the 3 lines full...)

That's intended. I think its better to show all or none techs from one building.

comment:20 by bb, 8 years ago

Keywords: review added; rfc removed

comment:21 by Itms, 8 years ago

I love this patch ;)

A few comments though:

  • The changes to raiseAlert/increaseAlertLevel seem bogus: both functions are identical except that increase calls raise, thus sending twice the command.
  • selection_details.js has some leftover code line 410
  • I noticed a typo in the name updateGarrisionHealthBar (line 446 of the same file). Can the typo be easily fixed while we're at it or would it be too many changes?
  • You should improve the wording of the comment line 644 of selection_panels.js, it's a bit difficult to understand at first.
  • The rightmost alert button is a bit overlapping the center panel, it would be better to keep the position it used to have.

comment:22 by Imarok, 8 years ago

fixed comment:21

comment:23 by Itms, 8 years ago

Sorry for missing the latest update :/

The delete hotkey is broken. If you try to kill one or more entities by pressing Del, it will spit an error once and won't work. The unit_actions button works though.

in reply to:  23 comment:24 by Imarok, 8 years ago

Replying to Itms:

Sorry for missing the latest update :/

No problem

The delete hotkey is broken. If you try to kill one or more entities by pressing Del, it will spit an error once and won't work. The unit_actions button works though.

Fixed

comment:25 by elexis, 8 years ago

Resolution: fixed
Status: newclosed

In 18773:

Correctly handle commands and production for multiple units and mixed selections. Patch by Imarok, reviewed by Itms, fixes #3823. Fixes #3492, #3288, #3668.

comment:26 by elexis, 8 years ago

Keywords: review removed
  • switch( -> switch (
  • data.button.onPress = function() { -> newline
  • if (unitEntStates.some(state => state.alertRaiser && state.alertRaiser.hasRaisedAlert && state.alertRaiser.canIncreaseLevel)) -> newlines. would have split them to multiple lines, but meh.

Thanks for the patch, fixes quite a lot of things that have been wrong since early alphas :)

comment:27 by elexis, 8 years ago

Refs #4251

by Imarok, 8 years ago

Attachment: upgrade.patch added

Show the correct number of resources when upgrading multiple entities

by Imarok, 8 years ago

Attachment: upgrade2.patch added

replace forEach with for

by Imarok, 8 years ago

Attachment: upgradev3.patch added

style fix

comment:28 by elexis, 8 years ago

In 18787:

Display the correct cost when selecting multiple entities that can perform the same upgrade. Patch by Imarok, refs #2706, #3823.

comment:29 by elexis, 8 years ago

In 18788:

Don't overwrite cached template data by by cloning the referenced object, thus fix a GUI bug revealed by refs #3823. Patch by Imarok, refs #4251.

comment:30 by elexis, 8 years ago

In 18790:

Fix paired techs following r18773. Patch by Imarok, fixes #4251, refs #3823.

comment:31 by elexis, 7 years ago

In 18803:

Fix a warning when pressing the delete hotkey when nothing is selected introduced by r18773, refs #3823.

Note: See TracTickets for help on using tickets.