Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3790 closed defect (fixed)

[PATCH] Remove UnitMotion planning

Reported by: mimo Owned by: Itms
Priority: Should Have Milestone: Alpha 20
Component: Core engine Keywords: patch
Cc: Patch:

Description

in UnitMotion, the function PlanNextStep introduced in r17229 tries to anticipate the obstructions from moving units in the following turns, but it has several problems as:

1) it is supposed to check two turns in advance, but as it is called before the current move, it effectively checks only one turn 2) it does not check if we have already a path in computation, which results in a lot of useless short path being computed 3) it does not check that the tested path will not go beyond our goal

These drawbacks are fixed by the attached patch. But the main problem remains which is that it does not take into account the motion of the obstructing units, so for example if units are following each others, the following ones will always be found obstructed and new short paths be recomputed. I've check using valgrind on 2000 turns of a 4 AIs game.

with svn, there is 2500 requestLongPath and 10400 requestShortPath, with the patch, there is 2500 requestLongPath and 9800 requestShortPath, without the planning, there is 2300 requestLongPath and 6500 requestShortPath, without any apparent effect on the unit movements.

These numbers are not fully comparable as the simulation is obviously different between these 3 cases, but it definitively shows that this planning creates a lot of short path requests. So my proposition is to completely remove it.

Attachments (2)

planNext.patch (2.5 KB ) - added by mimo 8 years ago.
remove_planning.patch (10.6 KB ) - added by Itms 8 years ago.

Download all attachments as: .zip

Change History (6)

by mimo, 8 years ago

Attachment: planNext.patch added

comment:1 by Itms, 8 years ago

Not much time to review that, but I'm still CCing myself, I can take a look in two weeks.

comment:2 by Itms, 8 years ago

Keywords: patch review added
Summary: UnitMotion planning[PATCH] Remove UnitMotion planning

You are entirely right :)

This patch would need to be tested in multiplayer just to be sure, but I didn't encounter any problems. It includes the removal of this planning along with some cleanup.

by Itms, 8 years ago

Attachment: remove_planning.patch added

comment:3 by Itms, 8 years ago

Owner: set to Itms
Resolution: fixed
Status: newclosed

In 17866:

Remove the now useless UnitMotion planning, and cleanup of CCmpUnitMotion.cpp (unused variables, whitespace). Fixes #3790.
This should have a noticeable impact on performance (in the good way!)

Thanks mimo for noticing something was off with the planning system!

comment:4 by Itms, 8 years ago

Keywords: review removed
Note: See TracTickets for help on using tickets.