Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ships disapeared after merging and splitting #2244

Closed
EnLightning opened this issue Aug 7, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@EnLightning
Copy link

commented Aug 7, 2018

Environment

  • FreeOrion Version: 0.48 RC2
  • Operating System: Windows 10
  • Fetched as
    • Binary release

Description

It seems the 3rd bug on fleets splitting/merging I reported after 0.47, this kind of issue are not really fixed till now. In this case, ships disapeared after merging and splitting

Steps to reproduce

  1. Load 128.sav
    128.zip
  2. At system Soare Gamma, merge the two battle fleets into one.
  3. Split this fleet, ships disapear.
@Dilvish-fo

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

I have replicated this problem, at least with this save file, and taking a variety of approaches. Adjusting the Objects window filter to show ships, fleets and systems, the missing ships appear at the bottom of the list, as not part of any fleet, a debug data dump for them shows their recorded fleet ID to be that of their fleet just prior to the split. Saving and reloading does not make them reappear. freeorion.log does not indicate any errors. The problem seems to only happen with the first split, trying more merging and splitting has not led to additional errors for me.

Having the departing fleet instead remain in system, advancing a turn, and then trying combining and splitting does not seem to lead to the problem happening; the problem therefore seems somewhat related to the save file load, and reminds me of the similar trouble the AI can have at that time #1794.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

I find that after drag-dropping the second fleet in the FleetWnd into the first, then splitting all the fleets, the first created new fleet (id 6745) is considered destroyed. This prevents it showing up in the FleetWnd, but it can be seen in the objects list of the filter is set to show destroyed objects. The ship that's supposed to be in this fleet, Algenib (id 3354) is still present in the objects list even if its fleet isn't shown, and appears at the bottom without a fleet (assuming suitable filters). This ship can be retrieved by merging all system ships into a single fleet. After doing that, even if subsequently re-splitting the fleet, all ships are again visible.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

That issue sounds quite serious. However, if it occurs only very rarely, assigning this to optional is probably sufficient. Otherwise, shouldn't this be mandatory...?

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

Notably, the affected fleet id is a newly created fleet from fleet transfers that happen after the save is loaded. The fleet actually was destroyed when merging the fleets, but then the ID is reused for the subsequent fleet split. The fleet remains marked as destroyed, so is then not shown, even though it's been recreated.

@Dilvish-fo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

@geoffthemedio that seems a great bit of progress in sorting out the problem, I'll try to follow up on that lead also.

@Vezzra I agree this is quite serious, and that it probably does merit being marked as mandatory; given how long this problem has lingered for the AI I had been reluctant to classify it as mandatory, but given Geoff's progress there I'm pretty hopeful we'll be able to resolve this in a reasonable amount of time.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Ok, set this issue to mandatory.

However, if it turns out to take way more time than expected, I suggest to reset it back to optional. The issue might be quite nasty, but doesn't outright break the game, so I don't want to delay the release too much for solving this.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

6e64234 hopefully resolves the immediate issue by re-rolling new object IDs if the provided one has already been used. It tries 100 times, so if many similar orders are issued, then a game saved, then restored, and a similar split/remerge order series issued, something similar could still occur, though.

There's probably a better solution involving making sure the ID allocator saves and restores its already-allocated IDs, but that would / will take longer to implement.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

A bit more info:

When the client issues and executes an order to create a new fleet, it gets an id number for the new fleet from the IDAllocator. When executing, it saves the ids it uses in the order. When a game is saved part way through a turn, all the issued orders are serialized and sent to the server and stored in the save state info for the player. When the game is loaded, the orders are sent back to the player and re-executed by the client. For new fleet orders, this means the saved object ids are used again. This means any subsequent orders that modify the new fleets will also have the correct fleet ids to use. What doesn't happen however is the IDAllocator sending and receiving back its state from part way through the turn. Thus, when a player issues more new fleet orders after loading, the IDAllocator may try to reallocate ids that have already been used in the new fleet orders executed client side when loading.

In this test case, the reused id was also thw id of a fleet the player deleted when merging fleets before splitting the merged fleet. The client thus reused a destroyed fleet's id for a new fleet and the new fleet was thus considered destroyed and hidden in most of the UI.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

The IDAllocator doesnt send its state when saving a game because it is a part of the Universe class. Prior to implementing the current IDAllocator, thwere was no need to send the Universe class as part of a save state message because the server already has the full Universe and just sends suitably filtered versions of it to the player clients as part of turn updates. Any issued orders are effectively saved as a series of diffs on the server's universe gamestate.

Probably the IDAllocator needs to be moved out of the universe and serialized as part of the save game state info sent from players to the server. Or possibly the server could inspect the orders it is sending to playetrs when loading, and possibly also the universe, and set the state of the IDAllocators of all players accordingly before serializing them to send to players.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

Reassigning this issue to post 0.4.8, as 6e64234 obviously is meant as a quick fix for the release branch (so this issue needs to stay open as it isn't fixed in master, but is no longer relevant for the release since it has been fixed in the release branch).

@Vezzra Vezzra modified the milestones: v0.4.8 (mandatory), post 0.4.8 Aug 19, 2018

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

"fixed" sort of, but yeah...

Independent testing requested, also...

@Vezzra

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Well, RC3 will contain your fix, so playtesting it should also take care of that independent testing.

But you're right, in the unlikely case that a proper fix gets committed in time to master, it should probably get cherry picked to the release branch (after rolling back the quick "fix"), so I'll reassign this to optional for the release.

@Vezzra Vezzra modified the milestones: post 0.4.8, v0.4.8 (optional) Aug 20, 2018

@Vezzra Vezzra modified the milestones: v0.4.8 (optional), post 0.4.8 Aug 31, 2018

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

Hopefully 31b1fa8 resolved this "better"...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.