Change Details

This is my proposed fix for the infinite loops problem. There have been various infinite loops reported at #5460. The beginning of the stack trace is always the same: Units go in IDLE. That calls FindNewTargets, which finds a new possible target and calls Order.Attack. Then there are four possible situations: - the order is rejected in Order.Attack (for whatever reason). The unit is still in IDLE state. The safety check from rP22059 triggers -> no infinite loop. - the order is rejected in Order.Attack, but the unit is a turret and has a garrison order which puts us in the IDLE state. Following rP22023, this loops. - the order is OKeyed, and the unit moves to ATTACK.enter. There, the `Move` call fails. The unit calls Finish Order and goes back to Idle. Rinse, repeat, infinite loop. - the order is OKeyed, the state enter goes alright, and the unit carries on attacking. We're fine. On the subject of rP22059: - This was a safety net so that the FindNewTarget calls didn't have to be moved to the timer. Given the new classes of bugs, the last of which can't be caught with rP22059, I no longer think this is reasonable. On rP22023: - Reverting this diff would only fix the second case described above. I believe the diff is conceptually sound, and helps avoid other classes of mistakes, so I am against reverting it. On "IDLE.Timer": - Yes, it is kind of a hack. It's a workaround the fact that IDLE is the default state. It's kind of unavoidable. - It's only called once, so it won't lead us to a "slow infinite loop" situation. - It's perfectly safe On a "correct" fix for these infinite loops: - The general cause of infinite loops is that we don't run some conditions early enough. D2008 for example is one case. - FindNewTargets in particular does not run all conditions from Order.Attack or ATTACK.enter - I don't think they //should//, though. ATTACK.enter, in particular, calls "MoveToTargetRange", which might fail for UAI-reasons (visibility) or for UnitMotion reasons - and the latter //**should**// remain unknown to UAI. For all of the above, I believe this is the only valid fix for this class of issues, which fixes all currently known infinite loops. It's also an example of defensive programming as changing behaviour in the attack order, or in FindNewTargets and related function will not suddenly trigger infinite loops, which a "correct" fix as described above might.

This is my proposed fix for the infinite loops problem. There have been various infinite loops reported at #5460. The beginning of the stack trace is always the same: Units go in IDLE. That calls FindNewTargets, which finds a new possible target and calls Order.Attack. Then there are four possible situations: - the order is rejected in Order.Attack (for whatever reason). The unit is still in IDLE state. The safety check from rP22059 triggers -> no infinite loop. - the order is rejected in Order.Attack, but the unit is a turret and has a garrison order which puts us in the IDLE state. Following rP22023, this loops. - the order is OKeyed, and the unit moves to ATTACK.enter. There, the `Move` call fails. The unit calls Finish Order and goes back to Idle. Rinse, repeat, infinite loop. - the order is OKeyed, the state enter goes alright, and the unit carries on attacking. We're fine. On the subject of rP22059: - This was a safety net so that the FindNewTarget calls didn't have to be moved to the timer. Given the new classes of bugs, the last of which can't be caught with rP22059, I no longer think this is reasonable. On rP22023: - Reverting this diff would only fix the second case described above. I believe the diff is conceptually sound, and helps avoid other classes of mistakes, so I am against reverting it. On "IDLE.Timer": - Yes, it is kind of a hack. It's a workaround the fact that IDLE is the default state. It's kind of unavoidable. - It puts the unit in an infinite slow loop situation if the conditions for "Timer" to put us in another state happen again. - It's perfectly safe On a "correct" fix for these infinite loops: - The general cause of infinite loops is that we don't run some conditions early enough. D2008 for example is one case. - FindNewTargets in particular does not run all conditions from Order.Attack or ATTACK.enter - I don't think they //should//, though. ATTACK.enter, in particular, calls "MoveToTargetRange", which might fail for UAI-reasons (visibility) or for UnitMotion reasons - and the latter //**should**// remain unknown to UAI. For all of the above, I believe this is the only valid fix for this class of issues, which fixes all currently known infinite loops. It's also an example of defensive programming as changing behaviour in the attack order, or in FindNewTargets and related function will not suddenly trigger infinite loops, which a "correct" fix as described above might.

This is my proposed fix for the infinite loops problem. There have been various infinite loops reported at #5460. The beginning of the stack trace is always the same: Units go in IDLE. That calls FindNewTargets, which finds a new possible target and calls Order.Attack. Then there are four possible situations: - the order is rejected in Order.Attack (for whatever reason). The unit is still in IDLE state. The safety check from rP22059 triggers -> no infinite loop. - the order is rejected in Order.Attack, but the unit is a turret and has a garrison order which puts us in the IDLE state. Following rP22023, this loops. - the order is OKeyed, and the unit moves to ATTACK.enter. There, the `Move` call fails. The unit calls Finish Order and goes back to Idle. Rinse, repeat, infinite loop. - the order is OKeyed, the state enter goes alright, and the unit carries on attacking. We're fine. On the subject of rP22059: - This was a safety net so that the FindNewTarget calls didn't have to be moved to the timer. Given the new classes of bugs, the last of which can't be caught with rP22059, I no longer think this is reasonable. On rP22023: - Reverting this diff would only fix the second case described above. I believe the diff is conceptually sound, and helps avoid other classes of mistakes, so I am against reverting it. On "IDLE.Timer": - Yes, it is kind of a hack. It's a workaround the fact that IDLE is the default state. It's kind of unavoidable. - It's only called once, so it won't lead us to a "slow infinite loop" situation puts the unit in an infinite slow loop situation if the conditions for "Timer" to put us in another state happen again. - It's perfectly safe On a "correct" fix for these infinite loops: - The general cause of infinite loops is that we don't run some conditions early enough. D2008 for example is one case. - FindNewTargets in particular does not run all conditions from Order.Attack or ATTACK.enter - I don't think they //should//, though. ATTACK.enter, in particular, calls "MoveToTargetRange", which might fail for UAI-reasons (visibility) or for UnitMotion reasons - and the latter //**should**// remain unknown to UAI. For all of the above, I believe this is the only valid fix for this class of issues, which fixes all currently known infinite loops. It's also an example of defensive programming as changing behaviour in the attack order, or in FindNewTargets and related function will not suddenly trigger infinite loops, which a "correct" fix as described above might.