#1720 closed enhancement (fixed)
[PATCH] Patrol
Reported by: | historic_bruno | Owned by: | svott |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 21 |
Component: | UI & Simulation | Keywords: | |
Cc: | Niek | Patch: |
Description
A unit moves back and forth between two or more waypoints, waiting to find enemy entities in LOS. Then it takes action based on stance (usually attacking). Similar to but different than attack move (#1001) because it doesn't stop moving.
Attachments (18)
Change History (76)
comment:1 by , 8 years ago
Milestone: | Backlog → Alpha 20 |
---|
comment:2 by , 8 years ago
Owner: | set to |
---|
comment:3 by , 8 years ago
I've implemented a minimalistic solution since a lot of basics are already given and can be reused.
-- UnitAI.CheckPatrolWalk() when the first and last order of the unit are ORDER.walk or ORDER.walkAndFight orders and have the same coordinates, a patrol is automatically activated.
Advantage: an additional hotkey or GUI element is not needed
Example walk: Start a walk-patrol (without auto attack in visual range) by press shift + right mouse button to coordinates x1 -> x2 -> x1
Example walkAndFight: Start a walkAndFight-patrol (attacks enemies in visual range, continue patrol automatically if enemy was killed) by press shift PLUS ctrl key + right mouse button to coordinates x1 -> x2 -> x1
Stop a patrol: Give a new order without pressing the shift key or just press the stop-GUI element
Notes:
- a patrol walk can compose of an arbitrary number of walk orders
- a patrol between two building is possible; however, you must click in the same position of the building otherwise the first and last point of the queue could are too far away for registering a patrol walk
- patrol also works for formations
- you can mix order.walk and order.walkAndFight (well, probably not smart)
- you can easy append a patrol later, since adding a walk order by pressing the shift key and right mouse button would just append the order queue without cleaning it
- since it is quite difficult to hit the same coordinate by mouse again, a small epsilon of 2.0 is currently implemented -> I don't say that is the best value, could probably a bit higher.
Open problems (for new tickets):
- a GUI flag could be implemented. Otherwise, it is a bit complicated to hit the first order again.
- a sound or message or this GUI flag stuff would be nice to give the user feedback that the unit(s) is(are) now on a patrol
! there is still some debug code included. I will clean it when you think the behaviour is okay so far
comment:6 by , 8 years ago
Keywords: | patch added |
---|
Haven't tested the patch itself but I can say you should remove warnings if they are commented out same for that for loop. Also fix that Todo by asking on IRC for instance.
comment:7 by , 8 years ago
Don't make UnitAI any "smarter", make this a command and make the user actually order the unit to patrol, not do things automatically. (Why do you know that the user does not want to make the units walk around a bit, but wants the units to show up as idle afterwards?)
Also see the coding conventions.
comment:8 by , 8 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Hotkey: X or Shift + X
The first try is deleted. New try contains no smarter UnitAI and has a hotkey. The GUI Button implementation is still missing.
And there was a difficult design decision.. quote from UnitAI line 1605
A-B-A-B-..: if the user only commands one patrol order, the patrol will be between last position and the defined waypoint A-B-C-..-A-B-..: otherwise, the patrol is only between the given patrol commands and the last position (position of the unit at the time of the first order) is not included -- this is a design decision because the order queue can be extended at any time and this makes it difficult to estimate the last walk back to the origin position
comment:9 by , 8 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I don't think you wanted to close that ticket: if you think your patch is ready just follow the steps in SubmittingPatches :)
comment:10 by , 8 years ago
Status: | reopened → new |
---|
follow-up: 12 comment:11 by , 8 years ago
Keywords: | review added |
---|---|
Summary: | Patrol → [PATCH] Patrol |
hmm.. I don't think that I will have time next week for the gui button. So I am done if you guys have no complaints and do the GUI stuff in an extra ticket.
follow-up: 13 comment:12 by , 8 years ago
Replying to svott:
hmm.. I don't think that I will have time next week for the gui button. So I am done if you guys have no complaints and do the GUI stuff in an extra ticket.
I can make one. what is the sized required?
follow-up: 14 comment:13 by , 8 years ago
Replying to Lionkanzen:
Replying to svott:
hmm.. I don't think that I will have time next week for the gui button. So I am done if you guys have no complaints and do the GUI stuff in an extra ticket.
I can make one. what is the sized required?
There is already an icon. It's more about the implementation and would be nice to receive a feedback about the logical behaviour one day.
comment:14 by , 8 years ago
Replying to svott:
Replying to Lionkanzen:
Replying to svott:
hmm.. I don't think that I will have time next week for the gui button. So I am done if you guys have no complaints and do the GUI stuff in an extra ticket.
I can make one. what is the sized required?
There is already an icon. It's more about the implementation and would be nice to receive a feedback about the logical behaviour one day.
Ok this patch need build? If isn't I can test, I'm very interesting in.
follow-up: 16 comment:15 by , 8 years ago
Sorry, what do you mean? The patch should work, at least, it worked 8 weeks ago (Hadn't much time for 0AD recently)
follow-up: 19 comment:16 by , 8 years ago
Replying to svott:
Sorry, what do you mean? The patch should work, at least, it worked 8 weeks ago (Hadn't much time for 0AD recently)
I'm asking if don't need the autobuild, I'm not programmer.
But I try to testing but didn't work, how is suppose to work? Even I don't see any change in the GUI.
comment:17 by , 8 years ago
Milestone: | Alpha 20 → Alpha 21 |
---|
comment:18 by , 8 years ago
Owner: | removed |
---|
@Lionkanzen: Looks like the changes are not conform with recent code changes of the SVN version. However, I don't have much time and a patch doesn't seem to be that necessary. So I won't continue working on it for now.
comment:19 by , 8 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
Replying to Lionkanzen:
Replying to svott:
Sorry, what do you mean? The patch should work, at least, it worked 8 weeks ago (Hadn't much time for 0AD recently)
I'm asking if don't need the autobuild, I'm not programmer.
But I try to testing but didn't work, how is suppose to work? Even I don't see any change in the GUI.
What is your problem? The patch still works for me! Maybe the usability is not the best. I would like to finalize the patch, but then I need feedback from the core team about the usability.
follow-up: 21 comment:20 by , 8 years ago
how is suppose this work? I don't see any difference after apply the patch.
comment:21 by , 8 years ago
Replying to Lionkanzen:
how is suppose this work? I don't see any difference after apply the patch.
Did you press X in game? (check comment 8)
comment:22 by , 8 years ago
Keywords: | review removed |
---|
Here is ideas on how patrol order was designed: wiki.
Here is a first quick review. I didn't deeply look at the code.
- I tried the patch, it applies and work fairly well.
- you should take care of wich unit can and can't patrol are allowed to patrol. I don't know for example if animals should be allowed to patrol. Moreover i got an error when ordering patrol to them :
ERROR: Animal got moved into INDIVIDUAL.* state
- when giving patrol order to a formation, the walking animation disappear. Then if we give a walk animation to this formation, the walking animation is still missing. (see attached gif)
- about the gui, it needs to be included (but you know that).
- The actual choice of the X hotkey is weird because we can't see what happened when giving patrol order to a structure (it produces batch of units instead). Same problem might occur too with ships or hero who can produce units.
- on the form: there are some whitespaces issues like missing spaces in
{"type": "Patrol", "data": this.patrolStartPos}
Thanks for (continue) working on this feature (it's a To Have).
by , 8 years ago
comment:23 by , 8 years ago
Status: | assigned → new |
---|
Thx for the review. Here a new patch.
- The logic does not allow patrolling animals. Let me know if there are other units which should not be able to do a patrol
- I fixed the animation. please check it
- the GUI button is available (now) -- However, the patrol is only possible between the current position and a selected point. for a multi-patrol order use: shift + patrol-key
- I changed the hotkey to Y. It should be unused and avoid conflicts
- I don't see the whitespace issue, but general, I will rework the code style after the logic has been accepted
I am looking forward to new feedback!
comment:24 by , 8 years ago
(indeed, shift + Y is not a friendly combination for players with small hands, I am open for any suggestion
you still can stack patrol orders by pressing the GUI button and shift key. But it is not nice to press the GUI button for any additional order again )
comment:25 by , 8 years ago
Keywords: | review added |
---|
comment:26 by , 8 years ago
All the previous remarks have been adressed and that sounds good.
Indeed animals should not patrol (so you don't need to add many stuff in UnitAI
).
I m not an artist but the gui button seems a bit too big.
I have also something wich is not a bug, with the same group and the same order, we can have clockwise and anticlockwise patrolling.
About formal issues : there is a space after { and before }. You should use fatarrow at L179 of Commands.js.
comment:28 by , 8 years ago
I looked again at the patch.
- I send you the vid about the clockwise stuff (but it's not a bug).
- There are style formal issues (you need space in { } L1145 of UnitAI, you might use a fatarrow at L179 of Commands.js.)
- Imo the logic of L5100 of UnitAI.js (the "UNIT" class) should be move to unit_action.
Thanks for working on that (as player i want this !).
comment:29 by , 8 years ago
Thx for the hints. I reworked all your mentioned flaws.
AND: I removed the queue=true statement in L146 of unit_actions.js I think it was just for testing a few month ago. Now patrol orders are not queued automatically. You need to press the shift key to queue commands -- yes, shift+Y is not a nice combination, but everything else is occupied by other functions
by , 8 years ago
Attachment: | patrol_V3.2.patch added |
---|
comment:30 by , 8 years ago
The patrol could be part of the return statement of unit_actions (line 218) to always enable it. Though you probably should disable it for animals in the GUI too.
The rest looks good (still need to test it though).
comment:31 by , 8 years ago
Thx for the hints -- I guess you mean input.js line 218
Isn't it deactivated for animals in the GUI? I can't do an animal to patrol
by , 8 years ago
Attachment: | patrol_V3.3.patch added |
---|
comment:32 by , 8 years ago
Thanks for the patch, finally found some time to test the patch, and there are some issues with it.
- Shift+Y is very annoying to reach with one hand, while patrol commands will probably often have queued paths. Perhaps P makes more sense, and P is closer to the Shift
- When holding Shift and using the action button to patrol, you need to click the action every time. It should stay active until Shift is released.
You should also update the in-game manual to show the hotkey (hard-coded), and also show the hotkey in the tooltip (not hard-coded, but fetched from the settings)
comment:33 by , 8 years ago
Seems like there is not enaough place for a patrol button. The gui only suppoorts up to 6 buttons. If back to work is available it's 7 buttons: delete, stop, garrison, repair, back to work, guard and patrol.
by , 8 years ago
Attachment: | gui_button_queueing.patch added |
---|
Allow queueing with the command buttons by pressing them only once
by , 8 years ago
Attachment: | patrol_V3.5.patch added |
---|
use P as hotkey, some whitespace fix and display hotkey in the tooltip
comment:35 by , 8 years ago
Keywords: | review removed |
---|
In unit_actions.js
- L247 no need for whitheline
- split out return objects over several lines and change the order of returns (so put the return false first) e.g.
if (!Engine.HotkeyIsPressed("session.patrol") || !getActionInfo("patrol", target).possible) return false; return { "type": "patrol", "cursor": "action-patrol", "target": target };
In UniAI.js
- no braces around one line if's
- use cmpPosition instead of cntPosition
- use let instead of var whenever possible (yes UnitAI is a mess)
- L4899 seems to be misplaces (the following lines were use by "GatherNearPosition")
In Commands.js
- arrow function with only one statement does not need braces
comment:36 by , 8 years ago
patrol_V3.X.patch and gui_button_queuing.patch needs to be committed both
comment:37 by , 8 years ago
Keywords: | review added |
---|
comment:38 by , 8 years ago
Thank you for improving my patch :) I checked the usability it works fine for me. (I am too busy for contributing, unfortunately - well, I like the summer time)
comment:39 by , 8 years ago
Keywords: | review removed |
---|
Default.cfg use spaces for intentation, not tabs
unitAI L1148-50 unneeded parenthesis L1659-61 same
L1143-46 This comment does not seem to be correct, the actual behaviour is that the patrol will happen between the last point of the unit and last given patrol order and not between the points given under the patrol order. Either chance this comment to be correct or chance the behaviour. I would prefer the second option as the current behaviour is not what is logical too me. (The first part of the comment is correct though and that behaviour is expected too)
by , 8 years ago
Attachment: | patrol_V3.8.patch added |
---|
Made the changes proposed by bb, besides the behaviour change, as it already is like described in the comment
comment:40 by , 8 years ago
Keywords: | review added |
---|
by , 8 years ago
Attachment: | patrol_V3.10.patch added |
---|
Based on the patch by bb. Fixed a bug introduced in input.js and fixed the handling of multiple selections.
comment:42 by , 8 years ago
what about a pentagonal shape like this: https://i.imgur.com/keu9wKR.png?
follow-up: 44 comment:43 by , 8 years ago
- Perhaps healers should heal too when patrolling instead of ignoring wounded units.
- Did you decide what must happen when using patrol on a building or a ressource ? Players should take care for example if you patrol between markets, it doesn't make a traderoute, same if you patrol a fishboat from fish to fish.
by , 8 years ago
Attachment: | patrol_V3.11.patch added |
---|
Fix units just moving and not attack-moving when patrolling (Found by fatherbushido)
follow-up: 45 comment:44 by , 8 years ago
Replying to fatherbushido:
- Perhaps healers should heal too when patrolling instead of ignoring wounded units.
Fixed now
- Did you decide what must happen when using patrol on a building or a ressource ? Players should take care for example if you patrol between markets, it doesn't make a traderoute, same if you patrol a fishboat from fish to fish.
The units just ignore the building/resource/market/etc.
follow-up: 46 comment:45 by , 8 years ago
The units just ignore the building/resource/market/etc.
Ok, in v3.10 it was like that but perahps we should not see the icon in this case ?
comment:46 by , 8 years ago
Replying to fatherbushido:
The units just ignore the building/resource/market/etc.
Ok, in v3.10 it was like that but perahps we should not see the icon in this case ?
Why? For the functionality it doesn't matter if there is a building/resource
comment:47 by , 8 years ago
I don't know, it's just a question.
- when you use commands of the same panel (guard, repair) on something wrong it is grayd.
- but you can use a move order on a building
so it seems ok like it is.
Another (I hope the last one) remark, when selecting a cc, we can use the modifier hotkey to have the patrol order (but we don't have this order in the panel)
by , 8 years ago
Attachment: | patrol_V3.13.patch added |
---|
Don't attack buildings when patrolling. Correctly continue patrolling after an attack
comment:49 by , 8 years ago
thx for having adressed this building attack stuff. The patch seems pretty fine now. let s wait for a last input/test/review. one other minor issues i encountered is when patrolling ram, then it will attack unit but not structure as intended, so it s stupid to patrol with rams (that s not really a bug).
comment:50 by , 8 years ago
I noticed that too, that the unit action hotkeys can be reused for rallypoints but the building not having these unit icons. Seems like a more general improvement. (Also might become crowded showing all unit and structure icons). The functionality is really nice. The code besides the UnitAI part can be considered reviewed by me. If the UnitAI review is needed (can take a closer look at those functions later).
if (g_Selection.toList().every(ent => !GetExtendedEntityState(ent).unitAI))
-> No need for the extended one
comment:52 by , 8 years ago
Keywords: | review patch removed |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Fixed in r18779 by elexis
comment:56 by , 8 years ago
comment:59 by , 8 years ago
The classic games iused this icons in red because is better to human eye than dark blue. See my post
I don't know if we already have this or not, but it seems interesting for A20.