Opened 5 years ago

Closed 14 months ago

Last modified 14 months ago

#1720 closed enhancement (fixed)

[PATCH] Patrol

Reported by: Ben Brian 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)

UnitAI.js.patch (3.7 KB) - added by svott 23 months ago.
to #comment3
patrol_V2.patch (8.0 KB) - added by svott 23 months ago.
+ config.cfg
out.gif (1.4 MB) - added by fatherbushido 20 months ago.
patrol_V3.patch (10.8 KB) - added by svott 19 months ago.
related to comment 23
patrol_V3.1.patch (10.0 KB) - added by svott 19 months ago.
removed style flaws
patrol_V3.2.patch (10.2 KB) - added by svott 19 months ago.
patrol_V3.3.patch (10.3 KB) - added by svott 18 months ago.
patrol_V3.4.patch (10.2 KB) - added by sanderd17 18 months ago.
Update to current SVN
gui_button_queueing.patch (687 bytes) - added by Imarok 17 months ago.
Allow queueing with the command buttons by pressing them only once
patrol_V3.5.patch (9.8 KB) - added by Imarok 17 months ago.
use P as hotkey, some whitespace fix and display hotkey in the tooltip
patrol_V3.7.patch (8.6 KB) - added by Imarok 17 months ago.
Made the changes of comment 15
patrol_V3.8.patch (8.6 KB) - added by Imarok 16 months ago.
Made the changes proposed by bb, besides the behaviour change, as it already is like described in the comment
patrol_V3.9.patch (9.0 KB) - added by bb 16 months ago.
Based on patch by Imarok
patrol_V3.10.patch (9.1 KB) - added by Imarok 15 months ago.
Based on the patch by bb. Fixed a bug introduced in input.js and fixed the handling of multiple selections.
Icon Proposal 3.png (2.7 KB) - added by stanislas69 15 months ago.
Here you are.
patrol_V3.11.patch (9.1 KB) - added by Imarok 15 months ago.
Fix units just moving and not attack-moving when patrolling (Found by fatherbushido)
patrol_V3.12.patch (10.2 KB) - added by Imarok 15 months ago.
mainly fix patrol for rallypoints
patrol_V3.13.patch (10.5 KB) - added by Imarok 14 months ago.
Don't attack buildings when patrolling. Correctly continue patrolling after an attack

Download all attachments as: .zip

Change History (76)

comment:1 Changed 2 years ago by wraitii

Milestone: BacklogAlpha 20

I don't know if we already have this or not, but it seems interesting for A20.

comment:2 Changed 23 months ago by svott

Owner: set to svott

comment:3 Changed 23 months ago by svott

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

Changed 23 months ago by svott

Attachment: UnitAI.js.patch added

to #comment3

comment:6 Changed 23 months ago by stanislas69

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 Changed 23 months ago by leper

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.

Changed 23 months ago by svott

Attachment: patrol_V2.patch added

+ config.cfg

comment:8 Changed 23 months ago by svott

Resolution: fixed
Status: newclosed

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 Changed 23 months ago by Itms

Resolution: fixed
Status: closedreopened

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 Changed 23 months ago by Itms

Status: reopenednew

comment:11 Changed 23 months ago by svott

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.

comment:12 in reply to:  11 ; Changed 21 months ago by 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?

comment:13 in reply to:  12 ; Changed 21 months ago by 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.

comment:14 in reply to:  13 Changed 21 months ago by Lionkanzen

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.

comment:15 Changed 21 months ago by svott

Sorry, what do you mean? The patch should work, at least, it worked 8 weeks ago (Hadn't much time for 0AD recently)

comment:16 in reply to:  15 ; Changed 21 months ago by 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.

comment:17 Changed 21 months ago by Itms

Milestone: Alpha 20Alpha 21

comment:18 Changed 21 months ago by svott

Owner: svott deleted

@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 in reply to:  16 Changed 20 months ago by svott

Owner: set to svott
Status: newassigned

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.

comment:20 Changed 20 months ago by Lionkanzen

how is suppose this work? I don't see any difference after apply the patch.

Last edited 20 months ago by Lionkanzen (previous) (diff)

comment:21 in reply to:  20 Changed 20 months ago by svott

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 Changed 20 months ago by fatherbushido

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

Last edited 20 months ago by fatherbushido (previous) (diff)

Changed 20 months ago by fatherbushido

Attachment: out.gif added

comment:23 Changed 19 months ago by svott

Status: assignednew

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!

Last edited 19 months ago by svott (previous) (diff)

Changed 19 months ago by svott

Attachment: patrol_V3.patch added

related to comment 23

comment:24 Changed 19 months ago by svott

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

Keywords: review added

comment:26 Changed 19 months ago by fatherbushido

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:27 Changed 19 months ago by Itms

In 18088:

Improve the UI icon for the patrol button, refs #1720.

Changed 19 months ago by svott

Attachment: patrol_V3.1.patch added

removed style flaws

comment:28 Changed 19 months ago by fatherbushido

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

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

Changed 19 months ago by svott

Attachment: patrol_V3.2.patch added

comment:30 Changed 19 months ago by sanderd17

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

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

Changed 18 months ago by svott

Attachment: patrol_V3.3.patch added

comment:32 Changed 18 months ago by sanderd17

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)

Changed 18 months ago by sanderd17

Attachment: patrol_V3.4.patch added

Update to current SVN

comment:33 Changed 17 months ago by Imarok

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.

comment:34 Changed 17 months ago by Imarok

and queing with buttons is a general bug. Here is a patch for it:

Changed 17 months ago by Imarok

Attachment: gui_button_queueing.patch added

Allow queueing with the command buttons by pressing them only once

Changed 17 months ago by Imarok

Attachment: patrol_V3.5.patch added

use P as hotkey, some whitespace fix and display hotkey in the tooltip

comment:35 Changed 17 months ago by bb

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)

In Commands.js

  • arrow function with only one statement does not need braces
Last edited 17 months ago by bb (previous) (diff)

Changed 17 months ago by Imarok

Attachment: patrol_V3.7.patch added

Made the changes of comment 15

comment:36 Changed 17 months ago by Imarok

patrol_V3.X.patch and gui_button_queuing.patch needs to be committed both

comment:37 Changed 17 months ago by Imarok

Keywords: review added

comment:38 Changed 16 months ago by svott

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

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)

Changed 16 months ago by Imarok

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

Keywords: review added

Changed 16 months ago by bb

Attachment: patrol_V3.9.patch added

Based on patch by Imarok

Changed 15 months ago by Imarok

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:41 Changed 15 months ago by stanislas69

Cc: Niek added

comment:42 Changed 15 months ago by Imarok

what about a pentagonal shape like this: https://i.imgur.com/keu9wKR.png?

Changed 15 months ago by stanislas69

Attachment: Icon Proposal 3.png added

Here you are.

comment:43 Changed 15 months ago by fatherbushido

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

Changed 15 months ago by Imarok

Attachment: patrol_V3.11.patch added

Fix units just moving and not attack-moving when patrolling (Found by fatherbushido)

comment:44 in reply to:  43 ; Changed 15 months ago by Imarok

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.

comment:45 in reply to:  44 ; Changed 15 months ago by 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 ?

comment:46 in reply to:  45 Changed 15 months ago by Imarok

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

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)

Last edited 15 months ago by fatherbushido (previous) (diff)

Changed 15 months ago by Imarok

Attachment: patrol_V3.12.patch added

mainly fix patrol for rallypoints

comment:48 Changed 15 months ago by elexis

In 18691:

Allow queuing orders if an action is preselected. Patch by Imarok, refs #1720.

Changed 14 months ago by Imarok

Attachment: patrol_V3.13.patch added

Don't attack buildings when patrolling. Correctly continue patrolling after an attack

comment:49 Changed 14 months ago by fatherbushido

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

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

In 18769:

Patrol icon by Stan, refs #1720.

comment:52 Changed 14 months ago by stanislas69

Keywords: review patch removed
Resolution: fixed
Status: newclosed

Fixed in r18779 by elexis

comment:53 Changed 14 months ago by elexis

In 18784:

Fix broken string as noticed by fatherbushido, refs #1720.

comment:54 Changed 14 months ago by Lionkanzen

Icons in red must be better visual performance

comment:55 Changed 14 months ago by elexis

Which icons in red?

comment:56 Changed 14 months ago by elexis

In r18785:

Add 2px of black border to the patrol icon. Icon by Stan, refs #1720.

comment:57 Changed 14 months ago by Lionkanzen

the points in the final icon can be more visible in red

comment:58 Changed 14 months ago by stanislas69

Indeed but it wouldn't match with the other icons

comment:59 Changed 14 months ago by Lionkanzen

The classic games iused this icons in red because is better to human eye than dark blue. See my post

https://wildfiregames.com/forum/index.php?/topic/16932-updated-list-of-buttonscommands-for-gui-session/

Last edited 14 months ago by Lionkanzen (previous) (diff)

comment:60 Changed 14 months ago by stanislas69

That's why there is yellow

Note: See TracTickets for help on using tickets.