#3570 closed enhancement (fixed)
[PATCH] Change message for failed join to host.
Reported by: | scythetwirler | Owned by: | Stan |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 21 |
Component: | Network | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
"Unknown reason" is hardly actually the case when one cannot join another's hosted game. The message should detail that most likely the host has not configured port forwarding/firewall properly.
Strings are located at binaries/data/mods/public/gui/common/network.js .
Attachments (9)
Change History (32)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Description: | modified (diff) |
---|
comment:3 by , 8 years ago
Unknown reason is still the best we know. Adding something more about possible causes can't hurt though.
comment:4 by , 8 years ago
It would be really awesome to add a notice about the host probably not having forwarded UDP port 20595 on the router and that the host should look up the router manual.
The lobby is spammed day by day with people who don't know why they can't connect, annoying.
comment:5 by , 8 years ago
Most notably, people tend to blame something on their own side rather than the host's, leading them to leave/uninstall etc.
comment:6 by , 8 years ago
Thanks for submitting a patch. For the next time, if you want to get the patch reviewed and committed, please follow the steps described in the wiki: SubmittingPatches This makes sure it shows up on the list of patches waiting for review.
To the patch: In my opinion, the message isn't quite optimal yet in regard to scythetwirler's comment Replying to scythetwirler:
Most notably, people tend to blame something on their own side rather than the host's, leading them to leave/uninstall etc.
This message gets to the client who tries to join a game. However, most likely the problem is actually the host because port forwarding is not configured. I'd say it's quite rare that outgoing connections are blocked on a network. This is usually the case in corporate networks, but people shouldn't be playing games there. In addition, it's quite likely that the connection to the multiplayer lobby wouldn't have worked in the first place if outgoing connections were blocked (you could still be connecting manually, but that's quite rare too). In my opinion, the message should point out that the problem is most likely on the host side (missing port forwarding on the router or a firewall), but still could be on the client side in rare cases.
comment:7 by , 8 years ago
Some other comments from leper in yesterday's IRC conversation
00:14 <@leper> should keep the Unknown Reason part 00:15 <@leper> keep that part, then add possible causes for it 00:16 <@leper> since it could be the wrong ip address, wrong forwarding rules, missing cable, no wifi, ...
comment:9 by , 8 years ago
Keywords: | review patch added |
---|---|
Owner: | set to |
Status: | new → assigned |
Summary: | Change message for failed join to host. → [PATCH] Change message for failed join to host. |
by , 8 years ago
Attachment: | 3570.2.diff added |
---|
Should take into account leper's elexis' and scythetwirler's comments.
comment:10 by , 8 years ago
Keywords: | review removed |
---|
Notice the disconnect event can either happen when failing to join or when being disconnected from a game.
So having the port-forward-hint displayed after being disconnected from a running game is wrong, as the server clearly works.
Implementation: If I understand correctly and there was an explicit disconnect (server shutdown, banned, rejecting join because the game is loading), then the reason byte will be != 0.
If the client can't connect to the server (port not forwarded, server not running, network timeout, no internet) or has been disconnected from the server without being explicity disconnected by the server (network timeout, no internet), this byte will be 0.
We may only give the clients a hint that the server didn't forward the port, if we were not connected to the game previously.
To accomodate that, getDisconnectReason
and reportDisconnect
must have a boolean argument wasConnected
, which must be
true
when callinggetDisconnectReason
frommessages.js
(disconnect from session)true
when callingreportDisconnect
fromgamesetup.js
(disconnect from gamesetup)false
when callingreportDisconnect
fromgamesetup_mp.js
(could not connect to server)
Not sure about the message itself.
open
should be replaced withforwarded
to be more accurate.- I agree with leper that it is a bit weird to mention some possible reasons (port forward, firewall) but don't mention others (no internet connection, network timeout).
- Having three sentences is also quite long. Maybe we could just list those reasons?
Sorry that I didn't have time to post this earlier.
comment:11 by , 8 years ago
About the first part I think that's for another ticket as the problem here is just supposed to be the message. For the second part I agree with all points, I don't know how to make the dots though.
comment:12 by , 8 years ago
Keywords: | review added |
---|
comment:13 by , 8 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|
comment:14 by , 8 years ago
Also notice the text for NDR_UNEXPECTED_SHUTDOWN
is actually wrong. It is not an "unexpected shutdown" but as far as I can see it only happens when either the host or the client willfully closes the game. When the client closes the game, we don't report the disconnect reason, so it should be safe to return "The host has ended the match" in getDisconnectReason
.
comment:15 by , 8 years ago
Keywords: | review removed |
---|
comment:17 by , 8 years ago
Milestone: | Alpha 20 → Alpha 21 |
---|
comment:18 by , 8 years ago
IIRC the only thing to be changed in the patch uploaded by Stan is the string. It shouldn't claim to know why the disconnect occured, but it could indeed hint that the host perhaps didn't forward UDP port 20595 on the router (as that's by far the most common case).
comment:19 by , 8 years ago
Phrasing:
- Generally -> often
- port -> UDP port
- It should first state something true (i.e. "Could not connect to host.") then follow with the speculation. the text seems too long, kill the last sentence and continue the prior one with ... or firewall bla
Logic:
- Don't nuke the
translateWithContext
- That patch misses the change to
reportDisconnect
.
comment:20 by , 8 years ago
Keywords: | review added |
---|
comment:22 by , 8 years ago
Keywords: | simple review removed |
---|
Thanks
Last changes:
- the "Failed to connect to server" is already contained in the reportDisconnect, so no need to add it to the reason string itself
- tricky: the dot is added to the reason itself (someone should fix that sometime probably. maybe not now as it would change all translated strings)
Failed to connect to server
->Failed to connect to the server.
an antivirus-software
->antivirus-software
, as software doesn't have a singular- some newlines
Not extremely urgent, but if we're breaking string freeze at the moment, we may as well address this. Push back if necessary.