If a networked game or replay becomes out of sync,
the C++ code triggers an untranslated popup box informing the clients of that fact.
Besides not being internationalized, this is also ugly as C++ should not know about the presentation layer.
Furthermore informing JS also allows future additions, like displaying a warning icon for the rest of the game.
Details
Details
Add a true || to the if (hashPair.second != expected) condition in NetServerTurnManager.cpp to trigger the error immediately.
Do the same for the if (hash != expectedHash) check in ReplayTurnManager.cpp to test.
Ensure that directory paths are printed correctly on Windows, as that EscapeToPrintableASCII function was needed to escape the path separators.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 641 Build 1015: Vulcan Build Jenkins Build 1014: arc lint + arc unit
Event Timeline
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/263/ for more details.
Comment Actions
Address comment by Sandarac: Remove unneeded messageBox arguments, as that function defaults to an "OK" button without effect.
Comment Actions
Notice we could also change the function to send the GUIDs instead of the names, don't really see the need though unless we'd make use of it somehow (f.e. by showing which players are OOS in the upcoming network, but that would be wrong too as further rejoining clients are likely to become OOS too and since there can be OOS errors which correct themselves few turns after).
Also the OOS message might be sent from the server to further rejoiners too, so that they became aware.
What were really useful is a dialog in the gamesetup that warns the host if he wants to start a networked game with AIs.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/266/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/267/ for more details.
Comment Actions
Looks good to me, but I can't test on windows.
| source/network/NetClientTurnManager.cpp | ||
|---|---|---|
| 136 | Don't we need an AutoRequest here as well? | |
Comment Actions
Ah, then I misunderstood you :D
I thought you ment, you cannot test, because you are on win
Comment Actions
Add missing JSAutoRequest call.
Fix messed up net LOGERROR player concatenation.
Shorten replay LOGERROR message and remove unneeded newlines.
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 610 | I've never seen it written that way. A quick search seems to indicate that I might not be alone with this observation. | |
| 620 | Now might be a chance to slightly reword at least one of those two sentences that are so very similar. Maybe s/is different/differs/? | |
| 628 | I guess a TODO comment about removing this once AI serialization is fixed would be nice. | |
| source/network/NetClientTurnManager.cpp | ||
| 126 | playerNamesStrings.reserve(playerNames.size()); | |
| 131 | You could probably still use a stringstream for this, not that it really matters either way. | |
| 135 | Do we really need the players here? | |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/314/ for more details.
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 610 | Guess it's not about the american english form but not abbreviating it with "out of sync"? The intention is to improve readability, since players don't know what "Out of sync" means and perhaps "Out of synchronization" is slightly better without going into too much detail. | |
| 620 | Ack for differs. Not sure what's the deal with similarity here, but colorization might also be on the table (green/red for the verb) | |
| 628 | Would rather add a TODO to #3858 than to the code since this occurance might change too | |
| source/network/NetClientTurnManager.cpp | ||
| 135 | For the logfile and terminal output, yes! The message box vanishes quickly, but we need to figure out what happened after the fact and who we need to/can ask to upload the file | |
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 610 | Out of those two terms one is likely to at least show up in search results, whereas the other one isn't. Also I'm not sure if there is any readability gain in that. That said players shouldn't see that error message anyway, as there shouldn't be any such errors. And finding out what this thing is about is important in case it actually does show up because someone somewhere messed up. | |
| 620 | Colours would be a nice addition here. Both "important" words do have the same length, so that seems quite similar, and nobody reads error messages. | |
| 628 | Fine by me. | |
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 610 | Maybe an idea might be to add some text saying that this should not occur and where to report? | |
Comment Actions
Add reference to the error reporting page
Add replay path to the error message
Mention that the launched mods are relevant
Print bugreporting details only if it's not a known error
Reserve space for vector
stringstream instead of CStr concatenation
different from -> differs
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 610 |
Changed it. Still, one finds only the occurance in the turn managers, not the place where the error occurs, it doesn't really help with debugging
Seems to be more relevant. The details should be added to http://trac.wildfiregames.com/wiki/ReportingErrors | |
| 620 | Actually it's red alert in all cases | |
| source/network/NetClientTurnManager.cpp | ||
| 126 | Alright, will keep it in mind in future patches to avoid repeated allocations | |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/439/ for more details.
Comment Actions
Notice having the OOS status in JS will also lay the foundation for showing that information persistently in the top right corner (where those lag warnings are shown),
so that players don't just click that message box away and pretend nothing happened, but get persistently disturbed by some red warning.
Comment Actions
Ack, if those inline comments are fixed. (I'm going to assume that you can rebase this without much trouble.)
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 610 | This line uses Out-of-sync for the context, while all below use Out-Of-Sync. (Might be nice to use the same thing everywhere.) | |
| 644 | If we are trying to use title case here it should be "of". | |
| 652 | This version of OOS makes me slightly uneasy (mostly because it isn't used by anyone else). | |
Comment Actions
Thanks for the review and test leper!
I've added the hint to #3858 but didn't extend http://trac.wildfiregames.com/wiki/ReportingErrors (partially due to laziness, but also because I'm expecting 10 false positives on one real bugreport. OOS was eliminated in the last releases and are now occuring in svn test matches with more competent players)
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 654 | In case I didn't mention it above yet, we might have noticed that the turn number isn't contained in the message anymore which is important for debugging. But devs can look at the command line output and still have the logerror while the turn number is printed continuously. | |