Scout fails to scout properly

In fact that could be as easy as
for (int radius_size=10; radius_size>1; radius_size--) {
 mr = Widelands::MapFringeRegion<>(map, Area<>(get_position(), radius_size));
 ...
}

To be honest, the scout code is full of WTFs. It'd be preferable if you could clean those up while you're at it, because piling bandaids onto bandaids is not going to make matters better. Here's an list:

1. Leave returning home to the 'return' program action (needs conf file fixes) and just pop_task() in scout_update when done. Duplicating the return logic in the scout task is nonsense.
2. run_scout fails to update the program pointer.
3. Bad time checks: if (state.ivar1 > game.get_gametime()) won't work when time values wrap around.
4. Catching the "fail" signal in scout_update is definitely WTF-worthy.
5. Calling start_task_movepath() with persist == 0 is bad, bad, bad.

Cleaner way of looking for unseen fields would be to use find_reachable_fields() with an appropriate CheckStep functor that doesn't walk into black territory. Actually, a modified Dijkstra search with an upper time limit would be even better.

Some more explanations to the short list above.

1. It's very plain why the current returning code fails: it tries to walk directly onto the position of the building, which would only work with forceonlast == true, but that would be incorrect, because we need to enter buildings via their flag. That's why that kind of stuff should be left to the existing 'return' task.

2. Every program action needs to increment the program counter (state.ivar1) unless it needs to be run again on next act(). This is what was causing the infinite Start/Update scout loop.

3. When time values wrap around, they are no longer growing monotonically. Rewrite that test as if (static_cast<int32_t>(state.ivar1 - game.get_gametime()) > 0), and everybody will live in peace.

4. Only ever ignore signals if you know where they're coming from. This doesn't seem to be the case here.

5. A persist == 0 findpath will *never* give up unless it has visited *all* fields of the map that are accessible from the starting point. So if you have a huge map, and there happens to be a small island in undiscovered country, you will have a call to findpath() for *every* *field* of that island that will look at *every* *field* of the huge map. Congratulations, you've just utterly killed performance.

I'd be glad to, but i might need some time to get used to the code,
especially the bindings with your 'programs'...

I agree the code looks hacky, and some of the things you pointed here
were already present in the geologist function, which i assumed was
correct.

Since i find that code interesting nevertheless, i might look deeper
at some time. But Im entering my exam session so don't expect me to do
that really soon, sorry.

Regards,

Charly

On 5/21/10, Nicolai Hähnle <email address hidden> wrote:
> To be honest, the scout code is full of WTFs. It'd be preferable if you
> could clean those up while you're at it, because piling bandaids onto
> bandaids is not going to make matters better. Here's an list:
>
> 1. Leave returning home to the 'return' program action (needs conf file
> fixes) and just pop_task() in scout_update when done. Duplicating the return
> logic in the scout task is nonsense.
> 2. run_scout fails to update the program pointer.
> 3. Bad time checks: if (state.ivar1 > game.get_gametime()) won't work when
> time values wrap around.
> 4. Catching the "fail" signal in scout_update is definitely WTF-worthy.
> 5. Calling start_task_movepath() with persist == 0 is bad, bad, bad.
>
> Cleaner way of looking for unseen fields would be to use
> find_reachable_fields() with an appropriate CheckStep functor that
> doesn't walk into black territory. Actually, a modified Dijkstra search
> with an upper time limit would be even better.
>
> ** Summary changed:
>
> - Scouting imporvement
> + Scout fails to scout properly
>
> ** Tags added: gamelogic scout
>
> ** Changed in: widelands
> Status: New => Confirmed
>
> ** Changed in: widelands
> Importance: Undecided => Medium
>
> --
> Scout fails to scout properly
> https://bugs.launchpad.net/bugs/583985
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in Widelands: Confirmed
>
> Bug description:
> I ran into an endless loop with the scout being home, the tooltip telling me
> there were no ration while the building dialog showed it full and the debug
> message showing Starting / Updating scout endlessly at 20ms intervals.
>
> I brwosed the sources and came up with this patch.
> The patch apply against revision 5348 and, while the code structure might
> have changed a bit, it only add the following changes :
>
> - After having tried undiscovered locations and then the old location, the
> scout will try to reach randomly all the locations on the closet frindge
> around him ; in the hope that a new reachable undiscovered location will be
> detected.
> - If no reachable locations are found, it will fail.
>
> I noticed than it never find a path to go home, but then the fail signal is
> emitted and it always manage to reach it at last. I don't know if it's an
> exploited workaround, but thats the reason why i added this call when no
> reachable locations are found.
>
> I am wondering if the pathfinding algorythm in the start_task_movepath
> function is reliable, although i didn't manage to prove it wrong.
>
> This is still not perfect, the scout might move randomly for some time, but
> at least it do not get stuck in his hut anymore. A possible f...

And thanks for the explanations, it really clears things out.

Yes, the geologist is subtly different, because its location is a Flag, not a Building. It would indeed be cleaner to modify the 'return' task so that it can also return to a Flag location, not just a Building location, and then use the 'return' action in the geologist's 'expedition' program.

Hi,

Here is a patch which take into account much of what you suggested above.

- The run_scout function updates the state ivar
- The scout first get out of building when starting
- find_reachables_fields is used to get reachable fields within a radius of 10.
   Coords are parsed randomly for a black undeiscovered field.
   After 100 iterations the scout moves to the furthest one which has not been visited for the longest time.
- The scout use the return function to get back home.

I'm not sure about performance here. find_reachable_nodes will fill the list with some thousands of coordinates, is that wrong?
And is it worth it to stop parsing after 100 items?
And is it worth it to use a functor for undiscovered fields only?

new alternative to find_reachable_fields would be great indeed.

Regards,

Charly

I worked on your patch (there were some datatype problems, an uninitialized variable, etc.) and added some more stuff.
However my version desyncs in multiplayergames for some reason, so I pushed it in a seperate branch lp:~nasenbaer/widelands/scout-fix

Anyone an idea, why the game desyncs?

Okay desync fixed - bug fixed -> Fix committed

Released in build16-rc1