Blamed: unittools.c, calling unit_restore_movepoints() being called inside update_unit_activity()
Although FCW and FC3.2 are not identical, both are evolving in complexity and specifically in the area where units may lose/gain move_points as a result of the actions of other units upon them.
The current method of unit_restore_movepoints() being called one at a time from a players_iterate { units_iterate { } pattern originally coming out of srv_main.c ... this has been exposed to create problems.
At FCW we have several ways this might happen:
#1.Go...And ; i.e., GOTO-then-do-order (attack/sabotage,bombard/etc.)
#2."Action_Success_Target_Move_Cost", whether from self, enemy, or allied player.
#3.EFT_PASSENGER_MOVE_COST_BP, a FCW effect in which cargo on a transport loses mp at a ruleset defined rate proportionate to the unit_move_rate of its transporter.
It was definitely confirmed to happen at FCW in the case #3, but ONLY FOR unfinished GOTOS which have incomplete paths that are executed at TC. In such cases, it happens only if (ptrans->id > pcargo->id == TRUE). This suggests it would also happen for similar cases in players_iteration with one player coming after the other. To me this was counterintuitive because update_unit_activity() restores mp and is called for every players' units prior to execute_orders(). Nevertheless it does create a bug in mp restoration in exactly half the cases: based on whether the ->id of the punit (and thus probably also pplayer) is greater or lesser.
Since execute_orders() being done at TC is where the bug happens, it appears to me that all cases of #1, #2, and #3 above are vulnerable to the same bug. Thus the report, even though FC3.2 does not yet have an EFT_PASSENGER_MOVE_COST_BP.
I suggest recreation of the bug in FC 3.1 and 3.2 by having a unit do something triggering an Action_Success_Target_Move_Cost on another unit of same nationality; try it both ways, but I think it's actor_unit->id > tgt_unit->id is the trigger case. NB: It will only happen if execute_orders() processes the action at TC.
FCW fixed this for ourselves with the following steps:
(1)
in srv_main.c right above update_unit_activities iteration:
/* Due actors now able to do TC-processed actions which affect tgt_unit->moves_left,
move point restoration now has to happen for ALL players' units before any single
one is allowed to process activity/orders. */
phase_players_iterate(pplayer) {
update_unit_move_points(pplayer);
} phase_players_iterate_end;
phase_players_iterate(pplayer) {
update_unit_activities(pplayer);
flush_packets();
} phase_players_iterate_end;
(2)
In 3.0/3.1, update_unit_activity() used to access int moves_left_at_start = punit->moves_left which I see 3.2 no longer accesses. Since this could no longer be done AFTER punit had its mp fully restored, we had to cache this value in punit->server.moves_left_at_start, as you see below in the second half of the solution in unittools.c:
/**********************************************************************//**
Iterate through all units for a player and restore their move points.
Has to be called before player/unit activities and orders processing
in order to avoid some units getting their mp refreshed AFTER
another player's units (or their own) affected their mp.
**************************************************************************/
void update_unit_move_points(struct player *pplayer)
{
unit_list_iterate_safe(pplayer->units, punit) {
/* Cache moves_left for later calculating of unit activity progress: */
punit->server.moves_left_at_start = punit->moves_left;
unit_restore_movepoints(pplayer, punit);
} unit_list_iterate_safe_end;
}
and finally (3) a change in update_unit_activity() to int moves_left_at_start = punit->server.moves_left_at_start instead of = punit->moves_left. Note that FCW now uses long int for all moves_left kind of values now because of how path_finding.c was only giving us half of a short int which caused integer overflow/underflow in any ruleset using granularity for move_fragments.
lexxie9952
Blamed: unittools.c, calling unit_restore_movepoints() being called inside update_unit_activity()
Although FCW and FC3.2 are not identical, both are evolving in complexity and specifically in the area where units may lose/gain move_points as a result of the actions of other units upon them.
The current method of unit_restore_movepoints() being called one at a time from a players_iterate { units_iterate { } pattern originally coming out of srv_main.c ... this has been exposed to create problems.
At FCW we have several ways this might happen: #1. Go...And ; i.e., GOTO-then-do-order (attack/sabotage,bombard/etc.) #2. "Action_Success_Target_Move_Cost", whether from self, enemy, or allied player. #3. EFT_PASSENGER_MOVE_COST_BP, a FCW effect in which cargo on a transport loses mp at a ruleset defined rate proportionate to the unit_move_rate of its transporter.
It was definitely confirmed to happen at FCW in the case #3, but ONLY FOR unfinished GOTOS which have incomplete paths that are executed at TC. In such cases, it happens only if (ptrans->id > pcargo->id == TRUE). This suggests it would also happen for similar cases in players_iteration with one player coming after the other. To me this was counterintuitive because update_unit_activity() restores mp and is called for every players' units prior to execute_orders(). Nevertheless it does create a bug in mp restoration in exactly half the cases: based on whether the ->id of the punit (and thus probably also pplayer) is greater or lesser.
Since execute_orders() being done at TC is where the bug happens, it appears to me that all cases of #1, #2, and #3 above are vulnerable to the same bug. Thus the report, even though FC3.2 does not yet have an EFT_PASSENGER_MOVE_COST_BP. I suggest recreation of the bug in FC 3.1 and 3.2 by having a unit do something triggering an Action_Success_Target_Move_Cost on another unit of same nationality; try it both ways, but I think it's actor_unit->id > tgt_unit->id is the trigger case. NB: It will only happen if execute_orders() processes the action at TC.
FCW fixed this for ourselves with the following steps: (1) in srv_main.c right above update_unit_activities iteration:
/* Due actors now able to do TC-processed actions which affect tgt_unit->moves_left, move point restoration now has to happen for ALL players' units before any single one is allowed to process activity/orders. */ phase_players_iterate(pplayer) { update_unit_move_points(pplayer); } phase_players_iterate_end; phase_players_iterate(pplayer) { update_unit_activities(pplayer); flush_packets(); } phase_players_iterate_end;(2) In 3.0/3.1, update_unit_activity() used to access int moves_left_at_start = punit->moves_left which I see 3.2 no longer accesses. Since this could no longer be done AFTER punit had its mp fully restored, we had to cache this value in punit->server.moves_left_at_start, as you see below in the second half of the solution in unittools.c:/**********************************************************************//** Iterate through all units for a player and restore their move points. Has to be called before player/unit activities and orders processing in order to avoid some units getting their mp refreshed AFTER another player's units (or their own) affected their mp. **************************************************************************/ void update_unit_move_points(struct player *pplayer) { unit_list_iterate_safe(pplayer->units, punit) { /* Cache moves_left for later calculating of unit activity progress: */ punit->server.moves_left_at_start = punit->moves_left; unit_restore_movepoints(pplayer, punit); } unit_list_iterate_safe_end; }and finally (3) a change in update_unit_activity() to int moves_left_at_start = punit->server.moves_left_at_start instead of = punit->moves_left. Note that FCW now uses long int for all moves_left kind of values now because of how path_finding.c was only giving us half of a short int which caused integer overflow/underflow in any ruleset using granularity for move_fragments.