[PATCH] Stop unpacking siege on stop/move command

[elexis]

When siege is unpacking but receives a direct order to move somewhere, there is no reason to wait until the unpacking finished. It should cancel the unpacking immediately and move there.

When the stop command is issued, the packing / unpacking should stop.

Changing UnitAI.js will likely be sufficient. Commands.js is used to pass player commands to the simulation.

[fatherbushido]

Thanks for the patch!

You can add the keyword tag if you want it to be pushed in the (pre)review queue. One should look at that TODO too

TODO: maybe a better way of doing this would be to use priority levels

[Ágoston Sipos]

Hi,

This was going to be my first contribution and I don't want to screw things up.

I feel that this TODO is about different things than this ticket's task. The ticket is about fixing a bug, while the comment is a proposal to rework the existing code.

I am also not sure what a 'keyword tag' is.

[elexis]

@wiki SubmittingPatches Add the rfc and put the milestone to a22 in case you want us to notice ;)

[elexis]

Moving to the new WIP milestone.

[bb]

Thx for working on this, but the choosen approach doesn't seem to solve the problem in all cases. Because what now if siege is packed and I give an attack order to somewhere out of range (instead of a Walk) during unpacking? I guess the siege will unpack, pack and then walk, thus the problem is not resolved :/.

The good news is that you found the correct part of the code, IMO the else if in L3839 is not doing what it should. Now the current packingorder is just hold in front of the new order, but in some cases the packingorder should simply be overridden. I did make a new function "ShouldPack" to determine if the siege needs to pack or not and use this function here to determine what should happen (cancel packing or not). This function can later also be used in some other places in the UnitAI.

[Ágoston Sipos]

Thanks, I checked this and you're right, an attack order does not cancel the unpacking.

Where will I find this ShouldPack function?

[bb]

As the function is mostly (if not only) used in UnitAI I did put it there.

[Ágoston Sipos]

Replying to bb:

As the function is mostly (if not only) used in UnitAI I did put it there.

I'm sorry, but I still can't find a function with that name. How is your patch going? When will it be in the codebase?

[bb]

Replying to odoaker:

I'm sorry, but I still can't find a function with that name.

I don't think such a function exist in current code (I am rather sure it don't), so you will need to create this function and design it so it solves all cases.

How is your patch going? When will it be in the codebase?

Not sure what patch you mean here?

[Ágoston Sipos]

You wrote "I did make a new function "ShouldPack?"". Maybe it was a typo but I understood it literally. I thought that a patch was on its way to be committed that included this function.

[bb]

Replying to odoaker:

You wrote "I did make a new function "ShouldPack?"". Maybe it was a typo but I understood it literally. I thought that a patch was on its way to be committed that included this function.

I think we just interpreted those words differently (I might be mistaken cuz I am not a native). I meant: "if I would fix this issue then I would make a new function."

[Ágoston Sipos]

I believe this solves all the problematic cases.

[Vladislav Belov]

This:

UnitAI.prototype.ShouldPack = function(type, data) 
{ 
    if(type == "Walk") 
        return false; 
    if(type == "Attack") 
    {
        // ... 
    }
    return true; 
};

Could be written shorter:

UnitAI.prototype.ShouldPack = function(type, data) 
{ 
    if(type == "Attack") 
    {
        // ... 
    }
    return type != "Walk";
};

[Vladislav Belov]

It doesn't work for the "Garrison" and "Patrol" commands.

Also shouldn't we revert the process (status bar of packing animation) instead of just reset w/o waiting? Looks weird, when you have 99% unpacked process, you press walk, and it's immediately packed.

[bb]

The patch indeed doesn't fix all issues yet and the implementation is not very trivial e.g. hard to understand what exactly happens.

We could start looking to the question: "When should we be unpacked?" AFAIK the answer would be when we get an attack order and we are in range or when we get an unpack order directly. In all other cases we should be packed IMO. So the ShouldPack function should return false in the cases stated above and true in all other cases. (This function can be reduced to a one-liner, but that is not the most important thing.)

We now want to find the correct behaviour using the new function, this can be found by thinking: if we currently pack/unpack and we should be in that state continue pack/unpack, otherwise cancel the pack/unpack. (For finding out wheter we are packing or unpacking we should check something like is done in the CancelPack/Unpack function in unitAI. Maybe make some new UnitAI function doing this check.) One thing we need to look out for is not giving the same order twice when an cancelPack/Unpack order is given.

Also have a look at our ​Coding Conventions (there were some whitespace issues and a semicolon).

Replying to vladislavbelov:

Also shouldn't we revert the process (status bar of packing animation) instead of just reset w/o waiting? Looks weird, when you have 99% unpacked process, you press walk, and it's immediately packed.

This could be solved in another ticket by altering the cancelPack/UnPack process a bit, but its not the issue for this ticket.

[Vladislav Belov]

Replying to bb:

This could be solved in another ticket by altering the cancelPack/UnPack process a bit, but its not the issue for this ticket.

Then could you ref it here? It should be depending tickets. I think, if implement the issue with reverting, then the solution of the ticket will be really different.

[Ágoston Sipos]

Thanks for your remarks, I simplified ShouldPack which fixed the problem with "Patrol". Not sure about "Garrison" because I can't garrison a siege unit (maybe I don't have the right building). I will attach the patch right now.

Replying to vladislavbelov:

Also shouldn't we revert the process (status bar of packing animation) instead of just reset w/o waiting? Looks weird, when you have 99% unpacked process, you press walk, and it's immediately packed.

I think your proposal could be disputed because it changes the strength of siege units. This patch though is just a bugfix. If this is done, I could work on that too, supposed that's decided to be the right way to have siege in the game.

[bb]

Replying to odoaker:

Thanks for your remarks, I simplified ShouldPack which fixed the problem with "Patrol". Not sure about "Garrison" because I can't garrison a siege unit (maybe I don't have the right building). I will attach the patch right now.

Siege can be garrisoned in a Fortress f.e.

The ShouldPack function looks good now, only it actually returns wheter a unit should UnPack, but that is an easy rename. (And yes naming of the Pack/Unpack function are sometimes a bit confusing but never bad to be clear when we talk about unpacking only.) Also not all cases are solved still: imagine a unit unpacking and getting an unpack order (is not a very likely situation, but it can occur when using selection), that will now get a cancelUnpack order and directly afterwards an unpack order. Not very efficient... This can be solved with the following code structure (which also makes the code more readable imo): (You should correct me if I am messing up here though)

if (IsPacking() && type != "CancelPack" && type != "CancelUnpack")
{
    if (ShouldUnpack())
        // cancel Pack order, hold unpack order;
    else
        // cancel unPack order, hold pack order;

    // Push new order
}

Some side note: for keeping consistency throughout the code, it would be nice if the lines you add in the code (L3854-6) would look as similar as possible to lines which to almost the same (L3848-50).