#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 )
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)
Change History (36)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Keywords: | patch added |
---|---|
Milestone: | Backlog → Alpha 21 |
Owner: | set to |
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 , 8 years ago
Attachment: | 3823.patch added |
---|
comment:3 by , 8 years ago
Keywords: | review added |
---|---|
Summary: | [Patch] Correctly handle commands for multiple units → [PATCH] Correctly handle commands for multiple units |
comment:4 by , 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:6 by , 8 years ago
comment:7 by , 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)
comment:8 by , 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 , 8 years ago
Description: | modified (diff) |
---|---|
Keywords: | patch review removed |
Milestone: | Alpha 21 → Backlog |
Summary: | [PATCH] Correctly handle commands for multiple units → Correctly handle commands for multiple units |
comment:10 by , 8 years ago
Description: | modified (diff) |
---|
comment:11 by , 8 years ago
Description: | modified (diff) |
---|
comment:12 by , 8 years ago
Owner: | changed from | to
---|
comment:13 by , 8 years ago
Keywords: | patch rfc added |
---|---|
Milestone: | Backlog → Alpha 21 |
Summary: | Correctly handle commands for multiple units → [PATCH] Correctly handle commands for multiple units |
github repo link: https://github.com/Imarok/0ad/tree/3823_multiple_selection
follow-up: 16 comment:15 by , 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)
comment:16 by , 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 , 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)
follow-up: 19 comment:18 by , 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...)
comment:19 by , 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 , 8 years ago
Keywords: | review added; rfc removed |
---|
comment:21 by , 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.
follow-up: 24 comment:23 by , 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.
comment:24 by , 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:26 by , 8 years ago
Keywords: | review removed |
---|
switch(
->switch (
data.button.onPress = function() {
-> newlineif (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 :)
by , 8 years ago
Attachment: | upgrade.patch added |
---|
Show the correct number of resources when upgrading multiple entities
In 17885: