Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

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

UnitAI.js.patch (3.7 KB ) - added by svott 8 years ago.
to #comment3
patrol_V2.patch (8.0 KB ) - added by svott 8 years ago.
+ config.cfg
out.gif (1.4 MB ) - added by fatherbushido 8 years ago.
patrol_V3.patch (10.8 KB ) - added by svott 8 years ago.
related to comment 23
patrol_V3.1.patch (10.0 KB ) - added by svott 8 years ago.
removed style flaws
patrol_V3.2.patch (10.2 KB ) - added by svott 8 years ago.
patrol_V3.3.patch (10.3 KB ) - added by svott 8 years ago.
patrol_V3.4.patch (10.2 KB ) - added by sanderd17 8 years ago.
Update to current SVN
gui_button_queueing.patch (687 bytes ) - added by Imarok 8 years ago.
Allow queueing with the command buttons by pressing them only once
patrol_V3.5.patch (9.8 KB ) - added by Imarok 8 years ago.
use P as hotkey, some whitespace fix and display hotkey in the tooltip
patrol_V3.7.patch (8.6 KB ) - added by Imarok 8 years ago.
Made the changes of comment 15
patrol_V3.8.patch (8.6 KB ) - added by Imarok 8 years 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 8 years ago.
Based on patch by Imarok
patrol_V3.10.patch (9.1 KB ) - added by Imarok 8 years 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 Stan 8 years ago.
Here you are.
patrol_V3.11.patch (9.1 KB ) - added by Imarok 8 years ago.
Fix units just moving and not attack-moving when patrolling (Found by fatherbushido)
patrol_V3.12.patch (10.2 KB ) - added by Imarok 8 years ago.
mainly fix patrol for rallypoints
patrol_V3.13.patch (10.5 KB ) - added by Imarok 8 years ago.
Don't attack buildings when patrolling. Correctly continue patrolling after an attack

Download all attachments as: .zip

Change History (76)

comment:1 by wraitii, 8 years ago

Milestone: BacklogAlpha 20

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

comment:2 by svott, 8 years ago

Owner: set to svott

comment:3 by svott, 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

by svott, 8 years ago

Attachment: UnitAI.js.patch added

to #comment3

comment:6 by Stan, 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 leper, 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.

by svott, 8 years ago

Attachment: patrol_V2.patch added

+ config.cfg

comment:8 by svott, 8 years ago

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 by Itms, 8 years ago

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 by Itms, 8 years ago

Status: reopenednew

comment:11 by svott, 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.

in reply to:  11 ; comment:12 by Lionkanzen, 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?

in reply to:  12 ; comment:13 by svott, 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.

in reply to:  13 comment:14 by Lionkanzen, 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.

comment:15 by svott, 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)

in reply to:  15 ; comment:16 by Lionkanzen, 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 Itms, 8 years ago

Milestone: Alpha 20Alpha 21

comment:18 by svott, 8 years ago

Owner: svott 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.

in reply to:  16 comment:19 by svott, 8 years ago

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 by Lionkanzen, 8 years ago

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

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

in reply to:  20 comment:21 by svott, 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 fatherbushido, 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).

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

by fatherbushido, 8 years ago

Attachment: out.gif added

comment:23 by svott, 8 years ago

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 8 years ago by svott (previous) (diff)

by svott, 8 years ago

Attachment: patrol_V3.patch added

related to comment 23

comment:24 by svott, 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 svott, 8 years ago

Keywords: review added

comment:26 by fatherbushido, 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:27 by Itms, 8 years ago

In 18088:

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

by svott, 8 years ago

Attachment: patrol_V3.1.patch added

removed style flaws

comment:28 by fatherbushido, 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 svott, 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 svott, 8 years ago

Attachment: patrol_V3.2.patch added

comment:30 by sanderd17, 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 svott, 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 svott, 8 years ago

Attachment: patrol_V3.3.patch added

comment:32 by sanderd17, 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)

by sanderd17, 8 years ago

Attachment: patrol_V3.4.patch added

Update to current SVN

comment:33 by Imarok, 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.

comment:34 by Imarok, 8 years ago

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

by Imarok, 8 years ago

Attachment: gui_button_queueing.patch added

Allow queueing with the command buttons by pressing them only once

by Imarok, 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 bb, 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)

In Commands.js

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

by Imarok, 8 years ago

Attachment: patrol_V3.7.patch added

Made the changes of comment 15

comment:36 by Imarok, 8 years ago

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

comment:37 by Imarok, 8 years ago

Keywords: review added

comment:38 by svott, 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 bb, 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 Imarok, 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 Imarok, 8 years ago

Keywords: review added

by bb, 8 years ago

Attachment: patrol_V3.9.patch added

Based on patch by Imarok

by Imarok, 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:41 by Stan, 8 years ago

Cc: Niek added

comment:42 by Imarok, 8 years ago

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

by Stan, 8 years ago

Attachment: Icon Proposal 3.png added

Here you are.

comment:43 by fatherbushido, 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 Imarok, 8 years ago

Attachment: patrol_V3.11.patch added

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

in reply to:  43 ; comment:44 by Imarok, 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.

in reply to:  44 ; comment:45 by fatherbushido, 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 ?

in reply to:  45 comment:46 by Imarok, 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 fatherbushido, 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)

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

by Imarok, 8 years ago

Attachment: patrol_V3.12.patch added

mainly fix patrol for rallypoints

comment:48 by elexis, 8 years ago

In 18691:

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

by Imarok, 8 years ago

Attachment: patrol_V3.13.patch added

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

comment:49 by fatherbushido, 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 elexis, 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:51 by elexis, 8 years ago

In 18769:

Patrol icon by Stan, refs #1720.

comment:52 by Stan, 8 years ago

Keywords: review patch removed
Resolution: fixed
Status: newclosed

Fixed in r18779 by elexis

comment:53 by elexis, 8 years ago

In 18784:

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

comment:54 by Lionkanzen, 8 years ago

Icons in red must be better visual performance

comment:55 by elexis, 8 years ago

Which icons in red?

comment:56 by elexis, 8 years ago

In r18785:

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

comment:57 by Lionkanzen, 8 years ago

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

comment:58 by Stan, 8 years ago

Indeed but it wouldn't match with the other icons

comment:59 by Lionkanzen, 8 years ago

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 8 years ago by Lionkanzen (previous) (diff)

comment:60 by Stan, 8 years ago

That's why there is yellow

Note: See TracTickets for help on using tickets.