Opened 21 months ago

Closed 14 months ago

Last modified 14 months 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 20 months ago.
3823_rebased.patch (2.5 KB) - added by Imarok 17 months ago.
rebased
upgrade.patch (2.1 KB) - added by Imarok 14 months ago.
Show the correct number of resources when upgrading multiple entities
upgrade2.patch (2.1 KB) - added by Imarok 14 months ago.
replace forEach with for
upgradev3.patch (2.0 KB) - added by Imarok 14 months ago.
style fix

Download all attachments as: .zip

Change History (36)

comment:1 Changed 21 months ago by Itms

In 17885:

Revert r17880, refs #3745, refs #3823

comment:2 Changed 20 months ago by svott

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?

Changed 20 months ago by svott

Attachment: 3823.patch added

comment:3 Changed 19 months ago by svott

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

comment:4 Changed 19 months ago by sanderd17

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 Changed 19 months ago by stanislas69

Any updates ?

comment:6 Changed 19 months ago by bb

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 Changed 18 months ago by svott

@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)

Changed 17 months ago by Imarok

Attachment: 3823_rebased.patch added

rebased

comment:8 Changed 17 months ago by elexis

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 Changed 15 months ago by Imarok

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 Changed 15 months ago by Imarok

Description: modified (diff)

comment:11 Changed 15 months ago by Imarok

Description: modified (diff)

comment:12 Changed 15 months ago by Imarok

Owner: changed from svott to Imarok

comment:13 Changed 15 months ago by Imarok

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

comment:14 Changed 15 months ago by Imarok

refs #3288

comment:15 Changed 15 months ago by 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)

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 15 months ago by bb (previous) (diff)

comment:16 in reply to:  15 Changed 15 months ago by Imarok

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 Changed 15 months ago by Imarok

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 15 months ago by Imarok (previous) (diff)

comment:18 Changed 15 months ago by bb

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...)

comment:19 in reply to:  18 Changed 15 months ago by Imarok

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 Changed 15 months ago by bb

Keywords: review added; rfc removed

comment:21 Changed 14 months ago by Itms

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 Changed 14 months ago by Imarok

fixed comment:21

comment:23 Changed 14 months ago by Itms

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.

comment:24 in reply to:  23 Changed 14 months ago by Imarok

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 Changed 14 months ago by elexis

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 Changed 14 months ago by elexis

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 Changed 14 months ago by elexis

Refs #4251

Changed 14 months ago by Imarok

Attachment: upgrade.patch added

Show the correct number of resources when upgrading multiple entities

Changed 14 months ago by Imarok

Attachment: upgrade2.patch added

replace forEach with for

Changed 14 months ago by Imarok

Attachment: upgradev3.patch added

style fix

comment:28 Changed 14 months ago by elexis

In 18787:

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

comment:29 Changed 14 months ago by elexis

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 Changed 14 months ago by elexis

In 18790:

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

comment:31 Changed 14 months ago by elexis

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.