Needs test: ships of a fleet can have the same name

[toptopple]

Ships of a single player's fleet can show the same name although they are only few in numbers. In my case there were 6 ships in the fleet. This is a bug compared to feature definition in bug #963842.

Don't know the cardinality of the ship name lists, but intuitively 6 appears too small to be real. So I assume it's a bug.

Build 8181.

[GunChleoc (gunchleoc)]

This could be solved by removing the ship name from the available names list each time a ship is created, and then call them Ship 12, Ship 13, etc. when there are no available names left.

[TiborB (tiborb95)]

Should not happen. We even have test for this: test_many_ships.lua - that creates 20 ships and verifies that granted names are unique.
List of available ship names are in data/tribes/[tribename].lua files.
We can increase the number to 50 to be sure how it behaves if the ship names "overflow"...
But I presume this is not the problem here, I presume you do not destroy ships...

[toptopple (7010622-q-deactivatedaccount)]

If you read the first post in bug #963842 by Desserud, everything is nicely pictured there. So there should be an unmodifiable "mother" list of names per race, then an operative list of names where random samples are drawn with elimination. When the operative list is empty, it is replaced by another copy of the mother list. When a ship is destroyed, its name can be back-pushed into the operative list, if one want to be peculiar (advantageous, but isn't needed).

The error is expected to loom somewhere in popping the name from the operative list.

[TiborB (tiborb95)]

The post is only request, the actual implementation is not exactly the same - ship names once used are not returned to pool and once the names are exhausted the new name is 'ship'. Were save/load involved when this happened? Also, are you sure it was single fleet?
Also you can attach a savegame if you have one at hand, it is not enough to debug exactly what happened, but still...

[toptopple (7010622-q-deactivatedaccount)]

I remember having looked carefully, also I remember seeing this phenomenon earlier in other games. Yes, saving/loading was involved prior to the event. Unluckily there is no save-game available now. I will supply one if it occurs again.

[TiborB (tiborb95)]

The responsible function is here: http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/src/logic/player.cc#L1191 - Player::pick_shipname()

I you feel like you can add some printf there, you are running a lot of seafaring games anyway...

but it really looks bulletproof to me...

[GunChleoc (gunchleoc)]

Maybe it was different players playing the same tribe that had the same shipname? Looking at the code, that's a possible scenario that I can think of.

[toptopple (7010622-q-deactivatedaccount)]

Why is this set to "Confirmed"? I suggest for now to set the report to "Needs Information".

[toptopple (7010622-q-deactivatedaccount)]

Finally gotten hold of a proof sample! In this game on "dustwind" there emerge two ships with name "Lynx". There was save/reload involved before this event was detected, so I would surmise the error has to do with reloading stuff not removing already existing ship names from the base (operations) list of names. In consequence ship names may emerge as often as reloads have occurred (prediction).

Who wants to look into this?

[TiborB (tiborb95)]

According to the description it seems to be an issue with remaining_shipnames_ list, as if it was not properly saved/reloaded - and got fully populated on load. Though code does not indicate such flaw.

[toptopple (7010622-q-deactivatedaccount)]

Interesting! - It depends a bit on what the target is, but if re-occurrence of previously used ship-names (e.g. of sunk ones) shall be allowed - which would make sense to me - then we don't need save and reload the remaining-ship-list. Under the additional condition that ship names preserve identity through the serialisation/de-serialisation process, the algorithm could look like this:

(on loading a game)
- initialise remaining-ship-list with a fresh full list of ship-names
- for all loaded ships: remove the ship's name from remaining-ship-list

Btw, could someone mark this bug "confirmed" or "triaged" now?

[TiborB (tiborb95)]

I improved the regression test the way
- create 20 ships
- save/load
- create 10 ships
and no duplicates. Weird. Also I can not see in the code how the situation could happen.

Yes, we can redesign the way how remaining ship names list are populated on load... but still we should find out what is really going on...

[TiborB (tiborb95)]

Well, I inserted few printfs into code and I see that when loading the game, both codes are running - fully populating by names from lua file and then adding actual remaining names from savegame.

Maybe simple remaining_shipnames_.clear() on savegame load would do the trick. According to the printf, loading the savegame takes place after player initialization. But I am bit surprised, I thought the savegame logic is better designed.

[toptopple (7010622-q-deactivatedaccount)]

We should seek to establish a test which verifies the error before performing corrections.

About testing: How large are the full lists of ship names? Noteworthy the error cannot be verified with a primary ship building number of near or larger than the full list because then remaining_names becomes empty and the error doesn't occur. So after current recognitions I would predict a positive test with half-size of the full name list. The test has to make sure "both" codes are performed (be realistic). Also, run it several times!

[TiborB (tiborb95)]

So I implemented my idea to put remaining_shipnames_.clear() before reading the savegame and it works, as can be confirmed by printfs:

1: Populating shipnames vector, now 34
1: Loading shipnames from savegame, now 30
2: Populating shipnames vector, now 34
2: Loading shipnames from savegame, now 34
3: Populating shipnames vector, now 58
......

So player 1 has 4 ships, correct?

Somebody more skilled in regression tests should advice how to make sure fullfledged save and load of game is be done in regression tests. If this can be done, also test like you suggested should be trivial...

[GunChleoc (gunchleoc)]

Does the stable_save not do a full saveload?

We would need to add the list of remaining shipnames to the Lua interface, then we could count them before and after calling stable_save.

[TiborB (tiborb95)]

It seems so.

According to my extension to regression test, but also I just looked for my printfs and they are nowhere in console when i run the test with my branch. So obviously only save is done.

Or look at the test: http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/test/maps/ship_transportation.wmf/scripting/test_many_ships.lua#L25

Is anything more needed to have full game load?

[TiborB (tiborb95)]

Also, after the test adds 10 ships I have the code to check the names of all ships and there are no duplicates. I did it few times...

[TiborB (tiborb95)]

I looked at regression_test.py, and there is function discover_loadgame_tests() but obviously it is not used now. I spent some time investigating it, but it probably needs some polish. I wanted to used savegame with 20 ships as a starting point for new test case.
Anyway, I pushed what I have into linked branch - propose it for merge, or take it over and add the test - as you wish.

[GunChleoc (gunchleoc)]

Since getting a test for this seems a bit or work, let's just have the fix for now.

[GunChleoc (gunchleoc)]

lp:~widelands-dev/widelands/bug-1643284 fixed the bug. Leaving this open in case we still want to write a test.

[GunChleoc (gunchleoc)]

Migrated to https://github.com/widelands/widelands/issues/1656