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

Fix last phase #816

Merged
merged 4 commits into from
Jul 22, 2022
Merged

Fix last phase #816

merged 4 commits into from
Jul 22, 2022

Conversation

lightvector
Copy link
Contributor

Ported from adamlerer#138

This PR fixes the API for the last turn of finished games.

First bug:
Webdip will duplicate the orders of the last phase of games that did not end by solo, creating an entire new set of phases (creating a whole new set of Diplomacy + Retreat + Build where the orders are the exact same as on the previous Diplomacy + Retreat + Build) . There is specifically code to do this, which makes me think it's deliberate and I have some suspicisons why, but anyways the API didn't handle it well.

Since the duplicated orders are actually in the database, we need a way to detect this and filter them out. We can't simply check whether the game was "Won" or not, because some games can be won by resignation/concession rather than by solo, and those games will have the duplication. We can't simply filter out the last turn unconditionally, because solo games don't have this duplication. We could try to count supply centers, but that's awkward. So I just added a heuristic check - if the game is over and the last two turn's orders look like duplicates, we just get rid of one of the copies.

Second bug:
The API unconditionally avoided populating the units array for the last phase from the historical tables, expecting them to be populated by the slightly different database tables that were used during a live game. But if the game is finished, the live game database tables are empty, so we have to populate the last phase units array from the historical tables.

Third detail:
I made the API append an extra dummy phase with the phase name "Finished" for finished games, with the final unit positions. This makes things a bit nicer on the API-caller side. (I suspect the reason why webdip duplicates the final phase orders was a hacky way of doing this for the old UI, except of course it leaves a permanent bit of ugliness in the database in that the final phase orders are now in there twice whereas all other orders are in there only once).

Lastly, I stripped out all of the draw phase idx hackery we had in the UI side. I think we don't need it any more.

Screenshots of second to last phase and last phase after this change:

image
image

@kestasjk kestasjk merged commit 4bd019e into kestasjk:staging Jul 22, 2022
@kestasjk
Copy link
Owner

Good stuff thanks. @jmo1121109 do you know why the last phase/orders get duplicated? I don't recall the system doing that

@kestasjk
Copy link
Owner

Looks like it's working fine on test.webdiplomacy.net, are you happy to push it to production? @adamlerer @lightvector

@lightvector
Copy link
Contributor Author

@kestasjk @jmo1121109 - If it helps, here is the code that seems to be duplicating the last phase:
https://github.com/kestasjk/webDiplomacy/blob/master/gamemaster/game.php#L1256-L1262
https://github.com/kestasjk/webDiplomacy/blob/master/gamemaster/game.php#L1308-L1314

I'm not sure if these are the only places, but they are definitely two of the relevant spots. I'm inferring that the duplication is intentional from nothing here looking like a typo, the code is really SELECTing from wD_MovesArchive on ($this->turn-1), and inserting an exact copy of that data that it just selected right back into wD_MovesArchive for that turn +1. But although it looks intentional, of course I'm not 100% sure what the intention was. :)

@lightvector
Copy link
Contributor Author

Looks like it's working fine on test.webdiplomacy.net, are you happy to push it to production? @adamlerer @lightvector

If it all seems to be working on test, then sure, I'm okay with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants