[PATCH] Improve the "back to work" behaviour

[Itms]

This patch allows units to remember a queue of work orders, and not a single one (useful for instance when constructing several buildings).

It also reduces the code and makes the button show up only when it's necessary (thanks mimo !)

The back-to-work button now also works for formations and its members.

[mimo]

Still the new changes have the problem that we loose the queue. In addition, there are some orders like ReturnResource which may be an order we want to remember (in case it was given by the player), but it may also be a sub-order of the gather order, in which case we rather want to remember the gather order.

So I wonder if it would not be simpler to replace the changes in UnitAi (finishOrder, pushOrder and your new ones) and only have in ReplaceOrder something like: if the added (forced) order is a "not work" order, and the first forced order in the queue is a work order, then put all the previous queue in this.lastWorkOrder. And if the added order is a "work" order, reset this.lastWorkOrder.

Futhermore, the gui only needs a bool to know if the unit has some WorkOrder or not, so it's not necessary to pass the full order (even more if we implement the queueOrder for lastWork) to the gui in GuiInterface.

[Marcio]

All units inside a building go to back and then they must y ignore rather rally point and try to back to the work.

[Itms]

Thanks mimo,

This is a patch which implements the back-to-work order queue.

It should work fine, please tell me if anyone experiences any problem !

[mimo]

Thanks for the new patch. The queues works fine now. I've still two comments concerning the UnitAI part: the BackToWork function can be simplified a lot by using the AddOrders function, and as I said in my previous comment, I think spreading the logic inside two functions (ReplaceOrder and FinishOrder) is not necessary.

I attach a slightly modified version of your patch with these two changes in UnitAI. let me know what you think.

[Itms]

I'm sorry I don't have time to test your patch right now, but I don't really understand how your patch could work (there's at least a problem with i0 in your loop in replaceorder)

More generally, I think the way your patch works is far less understandable (but maybe it's because I worked myself on my patch ;) ) ; could you make sure it works in any case ? For instance, queue building orders with multiple builders, take one of them somewhere else, make him go back to work, and once all the buildings are finished, check the button doesn't show up for him (I had a lot of trouble with that, which prevented me to use addOrders in backToWork).

Thanks for your review, anyway =)

EDIT : btw, when you talk about "slight" modifications, I don't really agree ;)

[mimo]

Replying to Itms:

I'm sorry I don't have time to test your patch right now, but I don't really understand how your patch could work (there's at least a problem with i0 in your loop in replaceorder)

Could you be more explicit ? I don't see the problem (but it's true that I have not tested the case where we change order while cheering).

More generally, I think the way your patch works is far less understandable (but maybe it's because I worked myself on my patch ;) ) ; could you make sure it works in any case ? For instance, queue building orders with multiple builders, take one of them somewhere else, make him go back to work, and once all the buildings are finished, check the button doesn't show up for him (I had a lot of trouble with that, which prevented me to use addOrders in backToWork).

Concerning the code lisibility, I fully agree that it may be a question of taste: if the code size is small (about 10 lines here), I prefer to have it all in one function instead of scattered in several functions. I think it is much simpler to maintain or even change it in the future. But certainly others have different opinions.

For the other point, my modifications should rather be seen as a work-in-progress proposition. I've done limited testing, although I've checked that queued orders worked in various cases. The part which is missing is how to deal with units in a formation: when we select a unit which was in a formation, it is removed from the formation while the queued orders were applying to the formation. But I think your patch also does not take it into account.

On a side note, in your BackToOrder function, there is a potential problem if ordering back to work while cheering. The first order (cheering) is already being processed, and you try to process it again.

Thanks for your review, anyway =)

EDIT : btw, when you talk about "slight" modifications, I don't really agree ;)

[Itms]

Okay, I tested it and now I understand ! I was totally unable to figure out how it worked because it's completely different of what I had in mind : you remember works when other orders are given while I remember works when they are ordered. I had not understand it when you explained it in your first comment, I apologise. 

What I like in your idea is above all the fact that the back to work button will show up less frequently.

So, thanks again for your help, I attach a new version of the patch. The problem with i0 may not be an error, but a different point of view ; I think we should slice the orderQueue from i (i.e. the first work order) and not from i0.

I fattened a bit backToWork because I think the back-to-work order should replace the queue, not just push work orders at its end.

I also reordered the functions in UnitAI, grouping backToWork with other workorders functions.

Next step : formation orders.

[mimo]

Thanks for looking into it, and sorry if i was not clear the first time :-)

Yes, may-be keeping only from the first work order (i) instead of the first order in the list (i0) is better. I've no strong opinion about it. But then, you may remove from the patch the definition of i0 and start the loop at 0 ? that would simplify a bit the function.

[Itms]

It hasn't been easy but I think this new version handles formations properly. I also cleaned up Commands.js.

[mimo]

I've tried your last patch, but it does not work for me (For tests, I've sent several builders to build several houses, then remove part of them and then send them back to work). When you test the queue orders with building order, are you sure your foundations are far enough one from the other ? when no more orders, builders look for another nearby foundation to build and this can fake a working queue order ?

Sorry, but I've modified again your patch to implement changes which I think were needed. After these changes, everything seems to work. But more tests are welcome. Here is a description of my changes (lines number are from my patch version)

line 35 (modif in Formations.js): when a unit is selected in order to be reaffected, it is removed from its formation, so we have to keep track of its formation orderQueue.

line 46: I do not understand why you changed that ? certainly a typo ?

lines 159 to 186: If a unit has already workOrders, we should keep them, otherwise if in a formation, we use the orderQueue of the formation, and if still nothing, we use the unit orderQueue. With this logic, all my tests worked fine.

line 208 to 212: when a unit is sent back to work, if it was in a formation, it is removed from it.

[Itms]

Thanks again for your help. Line 46 was indeed a typo, I was confused between orders and workOrders, and the last ones don't need to be accessed by the gui.

Other changes seem perfect, you're right, nearby foundations were faking work orders when I was testing.

[mimo]

Great, I also did some tests which were ok. So if you've tested it and are happy, I think the patch is ready. We can nonetheless try to clean a bit the UpdateWorkOrders before commiting it.

We could define const WORK_ORDER = [ "Gather", "Trade", "Repair", "ReturnResource"];

and replace the lines "type == "Gather"

type == "Trade"

type == "Repair"

type == "ReturnResource"" with WORK_ORDER.indexOf(type)

Another possibility, but I'm not sure if it would be really cleaner, is to put the identical loops in a function which would be called with cmpUnitAI or this as argument.

May-be other idea ?

[Itms]

Ok, I just replaced the (type == ) conditions with a convenience function (more adapted than an array we would search in, IMO).

The loop-function doesn't seem much cleaner, and here we only have two versions of the loop, that handle two different cases (use this's or member.cmpUnitAI's queue to construct the order queue). So the unification may not be this justified.

After testing, one problem remained : when you have a formation, you give it a work order, you order it to go somewhere (button shows up) -> when you click back to work they obey but the button doesn't disappear. I corrected it by removing my changes on Formation.js, which interfered with line 203 of the patch and are useless with our new way of handling the remembered works.

I think the patch is ready. =)

[mimo]

But the change in Formation.js is needed in case we have a formation at work with queued orders, and we only select a subset of it to give them some orders. This subset is removed from its initial formation, and when it arrives in ReplaceOrder, it has lost its formation orders.

I think the way to solve the problem you've reported would be to move the reset of workOrders from line 199 of your patch to line 204.

[Itms]

How can I have forgotten this ? x(

Thanks for the tip, it works indeed.

[Itms]

New version, based on some comments on IRC

[Itms]

And again, cleaning up some conditionals.

[mimo]

In 14063:

Improve back to work behaviour (queues taken into account), fix #2206, patch from Itms

[leper]

Just as a future note (it can stay in this way as hitting that codepath would already be an error) but calling Engine.QueryInterface() for some non-system component and then using this straight away will lead to weird errors when something that wasn't tested or forseen happens (some entity not having some component).