#3293 closed enhancement (fixed)
[PATCH] OOS error can display who is oos
| Reported by: | elexis | Owned by: | Itms |
|---|---|---|---|
| Priority: | Nice to Have | Milestone: | Alpha 19 |
| Component: | Network | Keywords: | patch |
| Cc: | Patch: |
Description
(1) Out-of-sync error messages should display which players are actually out of sync.
This has the advantage that players don't need to guess which player is fine and which is not.
The host checks in CNetServerTurnManager::NotifyFinishedClientUpdate if all clients are in sync. The list of clients that are oos should be sent along with the CSyncErrorMessage.
(2) If the CSyncErrorMessage is received by the clients, the list of oos players should be saved in a global variable and displayed in the game & player info dialog #3263.
(3) Also the hash values in the oos error message shouldn't be displayed, since they don't contain any information at all and only distract and confuse users.
Attachments (4)
Change History (12)
by , 9 years ago
| Attachment: | t3293_show_who_is_oos_v1_16870.patch added |
|---|
by , 9 years ago
| Attachment: | t3293_show_who_is_oos_v1.1_16876.patch added |
|---|
Some cleaning (mostly references instead of copies) and added comma to message.
comment:1 by , 9 years ago
| Keywords: | patch review added |
|---|---|
| Milestone: | Backlog → Alpha 19 |
| Summary: | OOS error can display who is oos → [PATCH] OOS error can display who is oos |
| Type: | defect → enhancement |
follow-up: 3 comment:2 by , 9 years ago
NetClient.cpp
m_Playernames->m_PlayerNames
NetTurnManager.cpp
- why switching to a wstringstream? Just apply utf8_from_wstring to the player names
- this code with the
firstboolean is ugly. Just make a standard loop starting at i = 1, and make sure all vector sizes will produce a valid output. - l 446: early return
oos_Playernames->OOSPlayerNames- l 662: typo
Also, could you convert the vector of custom type elements to a regular vector of CStrW in CNetTurnManager::OnSyncError? That would prevent this swarm of ugly pointer types in the other function prototypes.
Thanks for working on this :)
comment:3 by , 9 years ago
1) Replying to Itms:
NetTurnManager.cppwhy switching to awstringstream? Just applyutf8_from_wstringto the player names
a) OsString(OsPath(*path)) is wstring.
If I'm not mistaken the current code in CNetTurnManager::DisplayOOSError is wrong:
msg << L"Dumping current state to " << OsPath(*path).string();
Actual non-UTF8 character like 'ä' or 'ß' in that string are not translated appropriately if you don't use OsString(OsPath(*path)) but utf8_from_wstring(OsPath(*path).string()). In #3258 I got many problems with that because I renamed replay directories with those characters.
b) g_GUI->DisplayMessageBox takes a wstring and all our arguments (playernames and path) are given as wstring too. So if msg should be a stringstream we would have to convert, concatenate and then convert back while introducing translation errors.
2) Replying to Itms:
Also, could you convert the vector of custom type elements to a regular vector of
CStrWinCNetTurnManager::OnSyncError? That would prevent this swarm of ugly pointer types in the other function prototypes.
The uglyness would just be moved to the conversion in CNetTurnManager::OnSyncError. We'd be introducing 4 new ugly lines - either for-loop that calls for simplification or an unusual lambda function.
Just because std::vector<CSyncErrorMessage::S_m_PlayerNames> is an unusual type it doesn't mean that we have to convert it (at least if the conversion is not inline).
Not really a swarm, only 2 functions have that argument (CNetTurnManager::OnSyncError and CNetTurnManager::DisplayOOSError) and we would only remove it from the latter.
Thanks for reviewing! I agree with the other points and have updated the patch.
by , 9 years ago
| Attachment: | screenshot_ospath_utf8_bug.png added |
|---|
comment:4 by , 9 years ago
1) After some more cleanup, I agree to use stringstream instead of wstringstream as with the latter we would need this L in front of every hardcoded string. (The stringstream/wstringstream seems to automatically convert from UTF8 to 16 characters and vice versa, but I'm not sure if thats a bug or a feature).
1)a) Confirmed, the current svn code is broken for non ASCII pathnames. See screenshot above. Reproduce by introducing such a character:
OsPath path = psLogDir()/"öös_dump.txt";
OsPath is implemented in in source/lib/os_path.h and source/lib/path.h.
You can trigger an OOS error by adding true to the oos check in L646 of CNetTurnManager.cpp.
In the patch attached, the bug is fixed by using OsString accordingly instead of .string().
2) This would be the unusual conversion. Not sure why it segfaults, but I wouldn't want to convert anyway.
std::vector<CStr> playerNames2;
std::transform(playerNames.begin(), playerNames.end(), playerNames2.begin(),
[](const CSyncErrorMessage::S_m_PlayerNames& playerName)
{ return utf8_from_wstring(playerName.m_Name); });
Shows who is out of sync and doesn't display the hash value in the error message anymore. Also prevents the
CSyncCheckMessageandCSyncErrorMessagefrom being sent after oos was discovered to improve network performance.