[PATCH] Trade cart unusable if origin market is destroyed before finishing the trade route setup

[elexis]

Today a very critical error occured. A player without resources but more than 80 trade carts lost all of his trade carts due to a bug.

Reproduce the bug:

1. Build two markets
2. Produce a trade cart
3. Select the trade cart and set the origin market (but not the destination market)
4. Delete the origin market
5. Select the trade cart and hover the remaining market

Expected result: Tooltip saying that this market will be the origin if clicked. Actual result: That error:

ERROR: JavaScript error: gui/session/unit_actions.js line 317
TypeError: tradingDetails.gain is null
  unitActions["setup-trade-route"].getActionInfo@gui/session/unit_actions.js:317:1
  getActionInfo@gui/session/input.js:244:8
  unitActions["setup-trade-route"].actionCheck@gui/session/unit_actions.js:329:8
  determineAction@gui/session/input.js:324:8
  updateCursorAndTooltip@gui/session/input.js:63:7
  onTick@gui/session/session.js:405:2
  __eventhandler91 (tick)@sn tick:0:1

Since you can't setup the traderoute anymore, your affected tradecarts become idle forever.

Players of alpha 18 can only omit this bug if they don't forget to click the destination market immediately after the origin market.

[Vladislav Belov]

Second market doens't need to reproduce the bug (only 1 - 4). And this error shows every time, when mouse is over an other market (with selected tradecart).

[elexis]

Since you deleted the first market, you need the second (remaining) market, so that you have something to hover over.

[Vladislav Belov]

Replying to elexis:

Since you deleted the first market, you need the second (remaining) market, so that you have something to hover over.

I mean, that you don't need build both of them, you can build second after 4 step. I don't know how, but in first time this error was shown me without second market hovering.

[elexis]

Works for me.

[mimo]

Thanks for working on this patch. Using getEntityState for checking if an entity still exists is really overkill. Futhermore in this particular case, the first market could also have been captured (won't crash but still we may want to catch that), so I would suggest to test if the trader can still trade with the first market instead of testing the existence of the first market.

[elexis]

Replying to mimo:

Thanks for working on this patch. Using getEntityState for checking if an entity still exists is really overkill. Futhermore in this particular case, the first market could also have been captured (won't crash but still we may want to catch that), so I would suggest to test if the trader can still trade with the first market instead of testing the existence of the first market.

And what if the user decides to delete the market for any reason? Or what if siege destroys it?

[elexis]

Uses a simple template check to find if the entity exists (the same way GetEntityState does it )

[elexis]

Even cleaner and faster - suggested by mimo.

[mimo]

Thinking again about this patch, there is a problem because the gui will change the state of the simulation because of the StopTrading, and will give OOS.

[elexis]

Indeed - if you apply this patch you will get an instant OOS (without rejoining) on step 5.

The player that owns the trade cart sees his trade cart moving to the second market, when that is determined to be the origin market, while the other player sees that the trade cart doesn't move to the remaining market but rather stays in the same place.

[mimo]

I think the way to go is to add a OnGlobalOwnershipChanged in simulation/components/trader.js, so that traders can react when markets are destroyed. As a bonus, we could take advantage of it to modify the behaviour of traders: if a market is destroyed or captured and the trader is walking towards it, it would be better than the trader go back to the other market rather than continuing its route towards enemies.

[leper]

Global messages are slow, better to register the trader with the market and let the market send the message (or just call into the trader) in case it is destroyed/captured.

Though I'm not sure if having traders walk back in case of the target market not being visible (see the shared vision patch, or trading with neutral players) is a good thing.

[sanderd17]

The correct solution would indeed be to create a TradingMarket component, use that component to register the traders, and get all the HasClass("Market") checks replaced with checks if the TradingMarket component exists (in the Trader.js component, and in various places in the GUI).

Similarly, the HasClass("Ship") and HasClass("NavalMarket") should be replaced with definitions in the Trader and TradingMarket schema.

Taking it out of the review queue, as this needs more work.

[Stan]

Patch here is not the correct approach. Backlogging it for no progress.

[elexis]

In case we go with the idea of stopping trade carts once the market is destroyed, we should also stop the trade carts once the market has been captured. The bug might apply in that case too.

[otero]

Fixed problems highlighted in the ticket an other problems.

This patch includes a new component (Market.js) which is related with the markets created by the user. This component is used to subscribe the traders that are making transactions with this market, that way if the market gets destroyed, captured or the diplomacy changes(this is not discussed but has a similar problem) the traders that are subscribed to the market can take action. The behavior of the traders for every market is given by the simple algorithm:

1. If the market is not any more under control of the user or its allies (because was destroyed, captured or diplomatic change) and is the first Market for the traders, and their second market is still under control of the user or allies then send them to their second Market and viceversa.
2. In case that any of the two markets is any more under the control of the users they stop trading.

Inside the stop trading I eliminated the step where the resources the trader had were dropped, in my opinion those resources can be used still and can be used for trading or to redirect them inside other market. But, if this affects any behavior of the AI or something, then it should be ignored this change.

The expected behavior is the following:

Having two markets and a trader:
1. The trader add the market as a source and is registered inside the market
2. The trader then add the second (target) market and is registered inside it


If on of the markets  is destroyed, captured or became an enemy:
3. The trader stops, and starts walking towards the other market registered inside the trader

If the other market is destroyed, captured or became an enemy:
4. The trader stops where it is waiting for instructions and trading is stopped

In my opinion other behavior could be added to the trader, like if both markets are destroyed the trader should move to the closer civil center, etc. I didn't implemented this because it seems more of how the AI should behave and not the unit managed by the user.

If you find a problem please let me know.

[elexis]

• Next upload, upload one patch that contains the changes to all files please.
• Hopefully this won't conflict with #3697, in case we go with that.
• Market.prototype.OnOwnershipChanged, OnDiplomacyChanged -> three early returns
• if( => if (
• this.tradeEntities.forEach(function(trader) Either use a fat-arrow or for (let trader of this.tradeEntities). With for...of you won't need currentMarket anymore.
• Limit comments to one or two sentences per function (with JSDoc syntax /**)
• Remove that warn(JSON.stringify
• Maybe the calls to trader.StopTrading(); could be unified?
• Duplicate code in those functions. Better write a function that is called by those Market functions.

[mimo]

all the logic on what to do when a market is destroyed or captured should go inside UnitAI. A possibility would be that when a market is destroyed, it calls a UnitAI function for each of its traders. Futhermore, this reaction should only be done when the market in question is visible by the trader player (you can trade with neutrals or before you share visibility with allies). Using the market mirage could help for these tests.

typo in line 297 of Trader_js.patch (enitity -> entity)

I don't think we should remove the dropping of carried goods when stopping a trade route. This could lead to all kind of abuses (as starting a route with a faraway market, immediately stopping it and depositing the resources in a nearby one).

[otero]

Fixed the problems in the previous patch.

1. Contained all the changes inside a single patch upload
2. If the code has conflicts with the other ticket then I have no problem if you just ignore the patch.
3. I didn't understand the three early returns part but I hope with the change of the code it was eliminated
4. Fix the if
5. Change the for to use a traditional loop instead a for each loop
6. Fixed the comments for the methods. Some comments are a little long because I had to explain the logic behind the decision of the code at the moment.
7. Eliminated the warn (Sorry for that)
8. Unified the StopTrading method, but I found a little edge case with the Stop trading of UnitAI
9. Eliminated the duplicated code
10. Moved all the logic inside the UnitAI (although I don't understand this quite well as I think breaks the encapsulation principle)
11. Fixed the typo enitity
12. Returned the behavior where the trader drops its resources when StopTrading is called

By the way I don't know if I should open a ticket but the forum doesn't send me the confirmation email and I can't finish my sign up.

Any other problem please let me know

[mimo]

Thanks for the new patch. Here are some remaining comments

5. You could also use for (let trader of this.tradeEntities) and replace this.tradeEntities[i] by trader

also in line 79, we usually just use cmpUnitAI

6+8. This should be understood. Do you have a way to reproduce the problem ?

10. I was maybe not clear enough, but i meant all the logic about the unit behaviour (the trader stopping its route, and going back to its remaining market). So the MoveToMarket should be inside UnitAI, but not the InvalidateMarket.

Concerning the MoveToMarket, it should only be done when the market is visible. So when a market is destroyed or changes ownership or ..., you should check if the owner of the trader has a mirage on that market. If yes, it means that the market is not visible for him, and the order should only be done when the mirage is destroyed. But all that part about the change of the UnitAI behaviour can be delayed to a future patch is it complicates too much that one.

You can trade with a neutral player (see line 181 of Trader.js), so your OnOwnershipChanged in the Market component should be fixed accordingly.

I suppose you wanted your SubscribeTrader to return false when already registered ?

Useless bracket in Market.js (lines 49, 51, 64, 66)

You should check if the entity component exists before using it (for example line 63 for cmpOwnership)

[otero]

1. I have a problem with for...of loops, they are slower than plain old for loops. The reasons are because the assigned variable is recreated in every loop, the for loop has to resolve the iterator symbol, call the next function (adding unnecessary function calling into the stack), check functions like return and throw and make a redeference to the variables done and value (more about this in ​https://hacks.mozilla.org/2015/04/es6-in-depth-iterators-and-the-for-of-loop/). Also I searched in the internet about the performance costs of each for loop type and according to the measurements a plain for loop in the fastest. Considering that one of the problems in the game is performance, and that the javascript code is slower by nature, I think we should avoid forEach and for...of loops. Of course if that is a problem I could change it.

2. To reproduce the error is simple, if you add the patch and change the two lines that have the comment of the problem into this.StopTrading, you are going to need two markets from different players (I make a multiplayer game were I control both players). Build a market in both players and train a trader. Then create a trade route. First destroy the first market (the market that belongs to the user owner of the trader) this would invalidate that market for the trader and make it move into the other market. Then, change the diplomacy of the other player into an enemy this should call the method StopTrading.

Inside StopTrading the method StopMoving will be called, this method will call the command MoveCompleted for UnitAI which calls FinishOrder. After that, the method StopMoving will return an the next line the method FinishOrder is called again, this time the queue order is clean because of the first call to FinishOrder and it will raise an error. I was thinking to eliminate this line, as this method is called in other place but I couldn't reproduce any error if I eliminated that line so I don't know if is been used in other places and I couldn't find out or if its just redundant, if that is the case then I recommend eliminating the line.

3. I don't know how to check if the trader has a mirage in a Market to stop the trading, Is there some section of the code where something similar is used?.

I'm already working in the patch based in your comments, as soon as I have it, I'll upload it.

[fatherbushido]

Otero : There are some modifications in Trader.js and unitAI.js so you will need to rebase a bit.

[mimo]

I usually find "for of" more readable, the overhead is not so big (compared to "for in" for example which is really much slower) and I guess next SM versions will have it optimized. But you are right that a simple old for loop is still faster, and there are no problems to keep it in your patch (both are used in the code).

I've also not yet used mirages for such a purpose, but you can find an example which should correspond to your needs in lines 4454-4455 of UnitAI.js

[otero]

Fixed and rebased the patch according to comments. The only change I couldn't implement correctly was the mirage of the Market. I would comment what I tried for future references if someone wants to implement it.

1. Add a future queue into a Market where all the traders that are not inside the range of the market gets added, and try to communicate with them once they reach the target: This solution requires a type of global message to be sent every time a Market is destroyed as is impossible to access the Entity when needed, this is slow and create one of the problems commented above.

2. Create a while loop inside the event methods and keep the iteration until the condition that the market is in range is satisfied: This solution generates the problem that it creates iterators that consume cpu doing nothing and reducing the FPS in a game with many traders (at least there should be a iterator operation for each trader). The worst condition for this solution is when in a multiplayer game with AI, the AI decides to put the markets further away and create dozens of traders, this generates that for each tick in the game the engine should calculate if the trader is close enough to the trader to send finally the order to return to its market. If we consider the case where half the trader are moving from market 1 to market 2 and the other half are moving in the opposite direction, and the market 2 launches an event, the traders moving to the market 2 would have to wait less and waste less cpu than the traders moving to the market 1, in a massive game this create a big amount of lag.

3. A queue of events is added to UnitAI where futures can be managed, and when a certain condition is reached the future queue is review: This creates as many queues as units there are in the field consuming a really big amount of memory (most of those queue would be empty anyway) and if the future condition could never be accomplished and if the queues are not emptied correctly it could lead to memory leaks or to big time of garbage collectors ( the queue should store the action and the condition to launch the action, and the necessary information to execute the action).

4. A similar solution than (3) but generate the future queue only if necessary: This creates the problem of increasing the code for freeing the queues when the object is deleted.

So far I had no success trying to fix the problem of all the units stopping at the same time when an event is executed in a market.

[otero]

I also like to notice that I left the part of InvalidateFistMarket and InvalidateSecondMarket inside the UnitAI as calling the trader entity from the Engine is done anyway so it reduces one query.

In the method StopTrading removed FinishOrder as is called by other method (read comment above) and I didn't find any problem with trading, but I put this as a reference if something is found in the future.

[mimo]

One thing I just noticed is that you only register new traders, but never unregister them. You should when a trader stop a route (maybe catching the stopTrading()).

Concerning the mirages, my proposition would be to set a timer (fired for example every second) when a destroyed market is not visible, and at each iteration, the timer would check the visibility and stop the trader when needed.

[otero]

I didn't think about the unregistering part, I'll add it to the next patch, thanks.

About the timer, wouldn't that create a burden in memory and cpu? If the number of traders is big the number of object that has to be analyzed could be quite big, worst case scenario all traders are in the return path, outside the mirage of the market taking having number_of_trader * number_of_ticks operations in the slice of time. I think is similar to the solution with the while but it could be "sampled" less number of times.

A solution I was thinking was to make it dynamic. The first time the trade route is created we take the amount of time that the trader took traveling from one market to the other, and then checking every n ticks where n in a factor of the total time. For example if it took 150 ticks in the game traveling from one side to the other, we can capture that and check every 50 ticks or it could be even more complex. The problem is not the programming part, when I tried an algorithm a little more complex that the one I mentioned before, the game become really slow in a 2 v 2 game with three machines and at the end it slowed the FPS between 24 too 27. I'm not advocating for not implementing the solution but I think it could hurt performance somewhat.

Either way I would work in the algorithm and see if I'm capable of programming something that doesn't make the game harder to play.

[mimo]

I don't think the overhead will be big (the number of timer required will always be very small compared to what is used for gatherers for example, and they would be needed in very rare occasion, i.e. only when one of the market is not in the trader visibility).

Anyway, as the Feature Freeze is near, I push it to A21.

[otero]

Fixed the problem of the traders not been unsubscribed when the market is destroyed, and added a timer.

[mimo]

Thanks for the new version of the patch, and here are my new comments (sorry for the delay)

• there are some useless comments (lines 21, 22, 24 and 25 for example should be removed as the code is clearly understandable by itself).

• alliances are not transitive (A can be ally to B and C while B and C are enemies), so the tests you do on line 43 and 61 will not work on every case. I think the notify should be always send, and the check itself be done individually for each trader in NotifySubscribedTraders (and reusing the Trader.prototype.CanTrade would be better).

• line 71: are you sure the caching of l is not done by the compiler ? anyway, i think the (potential ?) gain is not worth the loss of readability.

• early continue allows to have less indent and improve readability. For example replace line 73 by if (! this.tradeEntities[i]) continue;

• to replace the alliance tests done when ownership or diplomacy changed, replace line 76 by

if (cmpUnitAI && !cmpUnitAI.CanTrade(this.tradeEntities[i]))

• lines 276 to 285: since the fix for the trade exploit (i don't remember the ticket number) there are no more this.firstMarket and this.secondMarket, but the array this.markets in the Trader component.

• InvalidateTradingMarket: you should change the trader order only if its order.data.target == market, otherwise better let it finish its route and have the corresponding gain.
• line 5271: why the test on !this.CheckTargetVisible(market) ? you should only test on cmpFogging.IsMiraged(cmpOwnership.GetOwner())).

[mimo]

In fact, i did some tests and fixing the behaviour according to visibility should be done in another patch as it will require quite some changes. The gui to set a trade route for example will also have to be fixed as it is presently not working with miraged markets (making it difficult to trade with neutrals or even with allies before the shared visibility tech).

So I wrote a simplified patch only fixing the bug, and a new ticket will be open for the visibility problem. The important point is that the list of traders for each market, and the list of markets for each traders is now fully consistant which was not the case in previous patches.

[otero]

Well, I fixed some of the problem that you mentioned and yes there is a problem with the visibility section; but, there are other problems I found that I think should be fixed too. For example, when using a Dock with a merchant ship and calling the function InvalidateTradingMarkets inside the timer, in the section where the trader is queried to the Engine it returns a new object with all the attributes initialized to the default values, instead of the values the object had, this create the problem that the markets the trader has registered after the function is called are empty and the trader stops trading.

I upload my patch with all the changes that were mentioned, It took me a week because the odd behavior the dockers have with this, I was trying to figure out a way to fix the problem, I think that after fixing the visibility problem is really easy to adapt my patch (in my opinion).

[mimo]

Thanks for the new patch. But i do think that we should commit first the fix for the original bug, and then work on fixing the visibility problems. So i join a second version of the simplified patch, which includes today's comment on IRC.

[otero]

Ok, don't worry.

[mimo]

updated with new comments

[mimo]

In 18028:

add a new market component, fixes #3277

[mimo]

r18028, inspired from otero's patch only fixes the original bug reported in this ticket. New improvments related to management of markets'visibility should now be discussed in #3894.

[elexis]

#4134 and #5127 describe the same GUI error stack, but it cannot be reproduced in the way described in this ticket.