Opened 8 years ago

Closed 3 years ago

#4015 closed defect (fixed)

[PATCH] Stop unpacking siege on stop/move command

Reported by: elexis Owned by: Ágoston Sipos
Priority: Nice to Have Milestone: Alpha 24
Component: Simulation Keywords:
Cc: agoston.sipos.95@… Patch: Phab:D3105

Description (last modified by Silier)

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.

Attachments (7)

change4015.patch (2.2 KB ) - added by Ágoston Sipos 7 years ago.
UnitAI.js.patch (1.9 KB ) - added by Ágoston Sipos 7 years ago.
I believe this solves all the problematic cases.
UnitAI.js.2.patch (1.8 KB ) - added by Ágoston Sipos 7 years ago.
change4015.2.patch (2.2 KB ) - added by Ágoston Sipos 7 years ago.
change4015.3.patch (3.0 KB ) - added by Ágoston Sipos 7 years ago.
This is the whole proposed change.
change4015.4.patch (3.1 KB ) - added by Ágoston Sipos 7 years ago.
change4015.5.patch (3.7 KB ) - added by Ágoston Sipos 7 years ago.
I hope I understood what I'm doing. Note that I still can't test because my build is faulty (even when reverting my changes).

Download all attachments as: .zip

Change History (49)

comment:1 by Ágoston Sipos, 7 years ago

Cc: agoston.sipos.95@… added
Owner: set to Ágoston Sipos

by Ágoston Sipos, 7 years ago

Attachment: change4015.patch added

comment:2 by fatherbushido, 7 years ago

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

comment:3 by Ágoston Sipos, 7 years ago

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.

comment:4 by elexis, 7 years ago

Keywords: rfc added
Milestone: BacklogAlpha 22

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

comment:5 by elexis, 7 years ago

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:6 by bb, 7 years ago

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.

comment:7 by Ágoston Sipos, 7 years ago

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

Where will I find this ShouldPack function?

comment:8 by bb, 7 years ago

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

Last edited 7 years ago by bb (previous) (diff)

in reply to:  8 ; comment:9 by Ágoston Sipos, 7 years ago

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?

in reply to:  9 comment:10 by bb, 7 years ago

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?

comment:11 by Ágoston Sipos, 7 years ago

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.

in reply to:  11 comment:12 by bb, 7 years ago

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

by Ágoston Sipos, 7 years ago

Attachment: UnitAI.js.patch added

I believe this solves all the problematic cases.

comment:13 by Vladislav Belov, 7 years ago

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";
};

comment:14 by Vladislav Belov, 7 years ago

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.

Last edited 7 years ago by Vladislav Belov (previous) (diff)

in reply to:  14 ; comment:15 by bb, 7 years ago

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.

in reply to:  15 comment:16 by Vladislav Belov, 7 years ago

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.

Last edited 7 years ago by Vladislav Belov (previous) (diff)

comment:17 by Ágoston Sipos, 7 years ago

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.

by Ágoston Sipos, 7 years ago

Attachment: UnitAI.js.2.patch added

in reply to:  17 ; comment:18 by bb, 7 years ago

Description: modified (diff)
Summary: Stop unpacking siege on stop/move command[PATCH] Stop unpacking siege on stop/move command

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

in reply to:  18 comment:19 by Ágoston Sipos, 7 years ago

Replying to bb:

Yes, I reorganized the code so. The unpack after unpack is not problematic at me (maybe the change solved the problem too). It just continues the unpacking.

Garrisoning works as well.

by Ágoston Sipos, 7 years ago

Attachment: change4015.2.patch added

comment:20 by bb, 7 years ago

Logic is getting form, good work on that, but we get a nasty bug as below:

  • order a unit to attack something
  • during unpack, order unit to attack something else in range
  • when packing finish it will throw an error for "no projectile actor not found" and it won't attack the newly targetted unit.

And as more of these bugs can easily appear, I don't think we should clean the orderQueue so brutally but only clean it when we actually should clean it (so when we need to cancel the (un)Pack). When we need to hold the current order, we should simply leave that order in the orderQueue and push the new order after that. The current approach works for now, but it is really easy to get this bugged. We now send in a new (un)Pack order via the newly given order and if we are already packing that order gets destroyed all the way down in Pack.js. (This is also the reason that the situation I described in last post was actually a false positive :S) So we will need to add another nested if else block. But this will need another change in L1845 of UnitAI, it now simply swaps state, but it should use a PushOrderFront call. (Otherwise we end up with units not responding correctly to orders given during unpack)

Talking of PushOrderFront: this new logic should also be put in there (whith some minor change due to the fact that PushOrderFront keeps the whole queue only puts one order before and replaceOrder destroys the whole queue).

Little style issue: the coding conventions say us if (Check) (watch the whithespace)

Last edited 7 years ago by bb (previous) (diff)

in reply to:  20 ; comment:21 by Ágoston Sipos, 7 years ago

Replying to bb:

Logic is getting form, good work on that, but we get a nasty bug as below:

  • order a unit to attack something
  • during unpack, order unit to attack something else in range
  • when packing finish it will throw an error for "no projectile actor not found" and it won't attack the newly targetted unit.

If I use the following approach:

else if (this.IsPacking() && type != "CancelPack" && type != "CancelUnpack")
{
	if (this.ShouldUnpack(type, data))
	{
		this.orderQueue = [];
		if (this.GetCurrentState().indexOf(".PACKING") !== -1)
		{
			this.PushOrder("CancelPack", { "force": true });
		}
	}
	else
	{
		this.orderQueue = [];
		if (this.GetCurrentState().indexOf(".UNPACKING") !== -1)
		{
			this.PushOrder("CancelUnpack", { "force": true });
		}
	}
	this.PushOrder(type, data);
}

Then things work fine at the situation you wrote: it attacks the lastly right-clicked enemy. However, the error message still comes at some occasions, but randomly, I couldn't find out what I have to do to cause it. (Is this message surely meaningful?)

But in the reverse situation, when:

  • Being unpacked, and started packing
  • I give an attack order to some unit in range

The packing is cancelled, but then the catapult just attacks the closest enemy and ignores the command. I couldn't find out why, but it's late, I will look at it tomorrow.

in reply to:  21 ; comment:22 by Ágoston Sipos, 7 years ago

Replying to odoaker:

But in the reverse situation, when:

  • Being unpacked, and started packing
  • I give an attack order to some unit in range

The packing is cancelled, but then the catapult just attacks the closest enemy and ignores the command. I couldn't find out why, but it's late, I will look at it tomorrow.

I have no idea. The likely problem is that when packing is cancelled, the unit auto attacks the nearest enemy. I don't know why my last line does not replace that order. (Even if I recursively call ReplaceOrder there.)

in reply to:  22 ; comment:23 by bb, 7 years ago

Replying to odoaker:

Then things work fine at the situation you wrote: it attacks the lastly right-clicked enemy. However, the error message still comes at some occasions, but randomly, I couldn't find out what I have to do to cause it. (Is this message surely meaningful?)

The error is triggered when a unit is not yet unpacked but already shoots a projectile, this is caused by some timer problem when reorder a unit to unpack, so a solution would be to avoid this reordering but simply holding the order (as I suggested in previous post).

But in the reverse situation, when:

  • Being unpacked, and started packing
  • I give an attack order to some unit in range

The packing is cancelled, but then the catapult just attacks the closest enemy and ignores the command. I couldn't find out why, but it's late, I will look at it tomorrow.

I have no idea. The likely problem is that when packing is cancelled, the unit auto attacks the nearest enemy. I don't know why my last line does not replace that order. (Even if I recursively call ReplaceOrder there.)

What happens here is that this.PushOrder("CancelPack", { "force": true }); cancels the pack directly and then the unit switches over to idle state. From that it will simply attack something random (L1552). After this the code in replace order continues and we push an attack order to the queue (so after we started attacking). In order to solve this we need to avoid that "idle" state. To do so we would need to keep the orderQueue filled so we first add the new order in queue (simply by stating this.orderQueue = [order]) and after that PushOrderFront the cancel order. The PushOrderFront needs some new logic to behave in this use case aswell (this is more or less the same logic as in ReplaceOrder, but not exactly the same as stated in last post). We would end up with code in ReplaceOrder like

	else if (this.IsPacking() && type != "CancelPack" && type != "CancelUnpack")
	{
		if (this.ShouldUnpack(type, data))
		{
			if (this.GetCurrentState().split(".").pop() == "PACKING")
			{
				this.orderQueue = [order];
				this.PushOrderFront("CancelPack", { "force": true });
			}
			else
				this.orderQueue = [this.orderQueue.shift(), order];
		}
		else
		{
			if (this.GetCurrentState().split(".").pop() == "UNPACKING")
			{
				this.orderQueue = [order];
				this.PushOrderFront("CancelUnpack", { "force": true });
			}
			else
				this.orderQueue = [this.orderQueue.shift(), order];
		}
	}

As you see I changed the checks for "(UN)PACKING" a bit since this is the way we already check for them in other parts of code. The order holding is implemented in this code aswell.

FYI: I found out what was happening here by using the dev overlay (alt+D) and then using the option "display selection state". You see then that the new order is added, only after a random attack order. By deducing were this order can come from results in the "idle" state.

Last edited 7 years ago by bb (previous) (diff)

in reply to:  23 comment:24 by Ágoston Sipos, 7 years ago

Replying to bb:

Ok, this change makes PushOrderFront usable, I think:

@@ -3767,7 +3767,7 @@
 		var cheeringOrder = this.orderQueue.shift();
 		this.orderQueue.unshift(cheeringOrder, order);
 	}
-	else if (this.order && this.IsPacking())
+	else if (this.order && this.IsPacking() && type != "CancelPack" && type != "CancelUnpack")
 	{
 		var packingOrder = this.orderQueue.shift();
 		this.orderQueue.unshift(packingOrder, order);

This is the same way ReplaceOrder's similar else if works. It seems to me, that neither of the previously mentioned bugs is occurring now.

by Ágoston Sipos, 7 years ago

Attachment: change4015.3.patch added

This is the whole proposed change.

comment:25 by bb, 7 years ago

The changes for PushOrderFront does indeed work for current svn, as that function is not called in all situations where ReplaceOrder can be called (as PushOrderFront is mostly called from inside UnitAI and ReplaceOrder has roots from players commands). But in order of bug prevention (is not bugfix) it would be nice to keep ReplaceOrder and PushOrderFront behave similar is given situations. So it would be nice to add all new logic from ReplaceOrder (e.g. check for ShouldPack() and (Un)Packing) in PushOrderFront too.

Besides this there still is a bug: Order a unit to attack something out of range, order during unpacking to attack something else in range, unit will attack first target.

This is caused by L1845 simply swapping state and not giving a new order. After that when we give an order the attack order is hold with the new logic instead of an unpack order, because there never has been an unpack order... The solution for this is to use an PushOrderFront call at L1845.

Little stylefixes: There is a misplaced whithespace in programming.js L145 We use Let instead of var whenever possible so L3834 needs let. (Yes, our code is not very consistent but newly added or changed lines should use let.)

in reply to:  25 comment:26 by Ágoston Sipos, 7 years ago

Replying to bb:

Besides this there still is a bug: Order a unit to attack something out of range, order during unpacking to attack something else in range, unit will attack first target.

This is caused by L1845 simply swapping state and not giving a new order. After that when we give an order the attack order is hold with the new logic instead of an unpack order, because there never has been an unpack order... The solution for this is to use an PushOrderFront call at L1845.

Sorry, this did not occur at my attempts but I could indeed reproduce it. The code change fixes it (or at least I couldn't reproduce it). I hope you were thinking the following (L1844):

if (this.CanUnpack())
{
	this.PushOrderFront("Unpack", {});
	this.SetNextState("UNPACKING");
}

by Ágoston Sipos, 7 years ago

Attachment: change4015.4.patch added

comment:27 by bb, 7 years ago

The SetNextState call is dumb now (PushOrderFront will take care of swapping the state), so can removed, also the data object in the pushOrderFront call (which is empty now) should contain something under the "force" key I think it is best to put { "force": this.order.data.force } there. (Maybe similar this can be added in L3855 and L3865 too changing true to data.force).

Also the remarks about PushOrderFront (see previous post) doesn't seem implemented yet.

Edit: found a new inconsistency: when unpacking and pressing "Stop", unpack is cancelled, when packing and pressing "Stop" packing continues. Imo the (un)Packing should be cancelled every time. One way to do this is putting a new if else before else if (this.IsPacking() && type != "CancelPack" && type != "CancelUnpack") and checking for IsPacking() && type == "Stop". Another approach would be to add type != "Stop" in L3848 and ordering CancelPack and CancelUnpack from "order.Stop". This part probably needs to change when we implement comment 14, but that can be done in another ticket. (I don't see a way to split the patches and not changing lines twice.)

Last edited 7 years ago by bb (previous) (diff)

in reply to:  27 ; comment:28 by Ágoston Sipos, 7 years ago

Replying to bb:

The SetNextState call is dumb now (PushOrderFront will take care of swapping the state), so can removed, also the data object in the pushOrderFront call (which is empty now) should contain something under the "force" key I think it is best to put { "force": this.order.data.force } there. (Maybe similar this can be added in L3855 and L3865 too changing true to data.force).

Sorry about that, I don't really know how "force" works, changed the things you said.

Also the remarks about PushOrderFront (see previous post) doesn't seem implemented yet.

I don't see why PushOrderFront should be more like ReplaceOrder (apart from what I already changed there). ReplaceOrder has to care about things like what we are talking about (e.g. what to do when getting an attack order while (un)packing), but as far as I know PushOrderFront just pushes them to the front of the queue (like a regular queue.pushFront() of a normal queue, with a few exceptions). Should the function also do the examinations about the order that ReplaceOrder is doing? Can't there be any other places where this would cause problems?

Edit: found a new inconsistency: when unpacking and pressing "Stop", unpack is cancelled, when packing and pressing "Stop" packing continues. Imo the (un)Packing should be cancelled every time. One way to do this is putting a new if else before else if (this.IsPacking() && type != "CancelPack" && type != "CancelUnpack") and checking for IsPacking() && type == "Stop". Another approach would be to add type != "Stop" in L3848 and ordering CancelPack and CancelUnpack from "order.Stop". This part probably needs to change when we implement comment 14, but that can be done in another ticket. (I don't see a way to split the patches and not changing lines twice.)

I rearranged the code like that:

else if (this.IsPacking() && type != "CancelPack" && type != "CancelUnpack")
{
	if ((this.ShouldUnpack(type, data) || type == "Stop") && this.GetCurrentState().split(".").pop() == "PACKING")
	{
		this.orderQueue = [order];
		this.PushOrderFront("CancelPack", { "force": this.order.data.force });
	}
	else if ((!this.ShouldUnpack(type, data) || type == "Stop") && this.GetCurrentState().split(".").pop() == "UNPACKING")
	{
		this.orderQueue = [order];
		this.PushOrderFront("CancelUnpack", { "force": this.order.data.force });
	}
	else
		this.orderQueue = [this.orderQueue.shift(), order];
}

Problem was that ShouldUnpack returned false for "Stop" and this was wrong only while packing.

in reply to:  28 ; comment:29 by bb, 7 years ago

Replying to odoaker:

Replying to bb:

The SetNextState call is dumb now (PushOrderFront will take care of swapping the state), so can removed, also the data object in the pushOrderFront call (which is empty now) should contain something under the "force" key I think it is best to put { "force": this.order.data.force } there. (Maybe similar this can be added in L3855 and L3865 too changing true to data.force).

Sorry about that, I don't really know how "force" works, changed the things you said.

Nvm, that is the reason we do reviews :P. The force flag give us wheter the order is given by the player or not. So it should be set true on player commands and commands directly followup on that and false on other commands. Of the current order it is stored under this.order.data.force. The flag is used f.e. when deciding wheter a unit should attack an attacking unit or should continue its current order (there is also some messing with stances in this code). In the code you posted you have used the "wrong" force tag. As you used the this.order.data.force we give through the data from the current executed order. But we need to use the data from the new order, so we should use data.force (and not this.order.data.force). (The call to PushOrderFront you changed elsewhere in UnitAI Should use this.order.data.force, just ReplaceOrder needs change.)

Also the remarks about PushOrderFront (see previous post) doesn't seem implemented yet.

I don't see why PushOrderFront should be more like ReplaceOrder (apart from what I already changed there). ReplaceOrder has to care about things like what we are talking about (e.g. what to do when getting an attack order while (un)packing), but as far as I know PushOrderFront just pushes them to the front of the queue (like a regular queue.pushFront() of a normal queue, with a few exceptions). Should the function also do the examinations about the order that ReplaceOrder is doing? Can't there be any other places where this would cause problems?

I think the analysis is correct that with your changed the code is unbugged for all current use cases of PushOrderFront. (I didn't test or dug deep in the code so cannot be 100% certain.) So for current code there is no problem. But it can produce problems in future when f.e. we have a unit packing and we PushOrderFront an attack in range to it (for whatever reason, probably useful when siege gets logic to attack units out of range, this is disabled currently). Also PushOrderFront and ReplaceOrder are very similar in a way, they both put a new order at front of the OrderQueue. The difference is that ReplaceOrder clears the queue and PushOrderFront leaves the queue as is behind the new order. With this ticket we only look what happens with that first order we add to the queue, so for that there is no difference in PushOrderFront and ReplaceOrder (yes, the code won't be exactly the same because we shouldn't clear the OrderQueue). Given this I would add the same logic in PushOrderFront i.o. bug prevention. Anyway it is good you come with these questions and probably other people disagree with me on this change. (But they can then complain that to me.)

I rearranged the code like that: ... Problem was that ShouldUnpack returned false for "Stop" and this was wrong only while packing.

The logic in the code is correct now AFAIK, only that force tag should be changed as above.

in reply to:  29 ; comment:30 by Ágoston Sipos, 7 years ago

Replying to bb:

Sorry for the break, I was busy taking exams and other stuff.

This is what I wrote for PushOrderFront, for the first else if line.

else if (this.order && this.IsPacking() && type != "CancelPack" && type != "CancelUnpack")
{
	if ((this.ShouldUnpack(type, data) || type == "Stop") && this.GetCurrentState().split(".").pop() == "PACKING")
	{
		this.orderQueue = [{"type": "CancelPack", "data": { "force": data.force }}, order, this.orderQueue.shift()];
	}
	else if ((!this.ShouldUnpack(type, data) || type == "Stop") && this.GetCurrentState().split(".").pop() == "UNPACKING")
	{
		this.orderQueue = [{"type": "CancelUnpack", "data": { "force": data.force }}, order, this.orderQueue.shift()];
	}
	else
	{
		this.orderQueue = [this.orderQueue.shift(), order];
	}
}

Problem is that I can't test it because in my fresh Linux build horrible things happen (catapult gets duplicated when unpacking then packing). It's also constantly throwing error messages. If my code can cause this than I'm sorry (and surprised), but don't think so. Is there maybe a known problem with the head?

About the code: I'm not entirely sure that orderQueue.shift() works correctly the way I used it. In the else branch, imho it makes no sense to push the new order before the ongoing packing order, so I put it after that.

in reply to:  30 ; comment:31 by bb, 7 years ago

Replying to odoaker:

Replying to bb:

Sorry for the break, I was busy taking exams and other stuff.

No problem, many of us, including myself, have those from time to time, Hope they went well ;)

This is what I wrote for PushOrderFront, for the first else if line.

 else if (this.order && this.IsPacking() && type != "CancelPack" && type != "CancelUnpack")
 {
...
 }

The code should work as expected when the orderQueue has length 1 when the function is called. But orderQueue can have an arbitrary length (by ordering with shift+click we add new orders to the end of the queue, see PushOrder()). This is caused by the fact we overwrite what we have in orderQueue, a way to keep the rest of the queue intact would be to use the unshift method. One would get something like: this.orderQueue.unshift({ "type": "CancelPack", "data": { "force": data.force } }, order).

Problem is that I can't test it because in my fresh Linux build horrible things happen (catapult gets duplicated when unpacking then packing). It's also constantly throwing error messages. If my code can cause this than I'm sorry (and surprised), but don't think so. Is there maybe a known problem with the head?

I don't know what is the cause of it, maybe my suggestions solve it or perhaps it is caused by some calls to PushOrderFront with an Unpack order as these doesn't work as expected (I guess). This issue is probably also present in ReplaceOrder and can be solved by adjusting the ShouldUnpack function to return true also when we get an Unpack order (no matter of any other condition).

If this also doesn't work please upload a full patch so I can try to debug. (Making a patch up from all the code fragments is a bit mehh)

About the code: I'm not entirely sure that orderQueue.shift() works correctly the way I used it. In the else branch, imho it makes no sense to push the new order before the ongoing packing order, so I put it after that.

Indeed it makes no sense to push a new order before a onhoing packing order when the packing should be done. But here also don't destroy the rest of the queue and use some unshift method. In this part a shift method would also be useful here. (As shift removes the first item from the queue also, and we here want to put a new item in between first and second)

(Also don't forget to remove braces in one line if's and add spaces in objects when uploading this as a patch)

by Ágoston Sipos, 7 years ago

Attachment: change4015.5.patch added

I hope I understood what I'm doing. Note that I still can't test because my build is faulty (even when reverting my changes).

in reply to:  31 comment:32 by Ágoston Sipos, 7 years ago

Replying to bb: Oops, seems like an attachment does not notify anyone...

My build is still faulty, so I would be glad if you tested my patch.

comment:33 by bb, 7 years ago

ye, srry for taking a time, but didn't have time till now to have a proper look, thx for waiting.

wrt patch: the only real bug there still is (AFAIK), is what I tried to explain in last comment about the "unpack" order. (Ordering a unit to pack when unpacking (or other way around)). For proper functionality of ShouldPack || type == "Unpack" should be appended. Only this doesn't solve the root cause. The problem lays deeper into the the unitAI.Pack (or unitAI.Unpack, this whole thing should be done for that too) function, this only checks for CanPack, returning false when unpacking (as it should). But when getting a pack ordering while unpacking, the unpacking should simply be cancelled (agreed with wraitii and scythetwirler_). This can be done by adding an cancelUnpack order when unpacking in the unitAI.Pack function.

(You can order a pack order while unpacking by selecting 2 sieges, 1 unpacking and 1 unpacked)

Also while on it L3778 var -> let and L1845 whitespace error { "force" : this.order.data.force }); -> { "force": this.order.data.force });

For the build error: did you try redownloading the svn, so delete all the folders from svn and redownload? If this doesn't work, feel free to drop by on IRC, so we might find out what is happening.

Last edited 7 years ago by bb (previous) (diff)

comment:34 by Stan, 6 years ago

Description: modified (diff)
Keywords: simple rfc removed
Milestone: Work In ProgressBacklog

No news for one year backlogging.

comment:35 by Imarok, 5 years ago

Component: UI & SimulationSimulation

Move tickets to Simulation as UI & Simulation got some sub components.

comment:36 by elexis, 5 years ago

Patch: Phab:D1520

comment:37 by Silier, 4 years ago

Milestone: BacklogWork In Progress

comment:38 by Silier, 4 years ago

Resolution: fixed
Status: newclosed

In 23298:

Automatically cancel packing or unpacking based on given order

Cancel packing when next command requires unpacked state and cancel unpacking when next command requires packed state.

Fixing unit refusing to move because keeps unpacking to attack targets in range.

Partially Fixes: #4015, #5328
Differential Revision: https://code.wildfiregames.com/D1520
Patch by: @causative
Comments by: elexis, Freagarach, Stan

comment:39 by Silier, 4 years ago

Milestone: Work In ProgressBacklog
Patch: Phab:D1520
Resolution: fixed
Status: closedreopened

stop command remaining

comment:40 by Silier, 4 years ago

Description: modified (diff)

comment:41 by Silier, 3 years ago

Milestone: BacklogAlpha 24
Patch: Phab:D3105

comment:42 by Silier, 3 years ago

Resolution: fixed
Status: reopenedclosed

In 24191:

Cancel packing orders by Stop command

Make packing states to react to Stop command, which is supposed to cancel current orders.

Fixes: #4015
Differential Revision: D3105

Note: See TracTickets for help on using tickets.