Opened 2 years ago

Closed 19 months ago

Last modified 18 months ago

#3570 closed enhancement (fixed)

[PATCH] Change message for failed join to host.

Reported by: scythetwirler Owned by: stanislas69
Priority: Should Have Milestone: Alpha 21
Component: Network Keywords: patch
Cc: Patch:

Description (last modified by scythetwirler)

"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)

3570.diff (791 bytes) - added by Mikita Hradovich 2 years ago.
Updated message for NDR_UNKNOWN
screenshot0001.png (325.0 KB) - added by Mikita Hradovich 2 years ago.
Dialog with new message
3570.2.diff (820 bytes) - added by stanislas69 2 years ago.
Should take into account leper's elexis' and scythetwirler's comments.
3570.2.2.diff (3.5 KB) - added by stanislas69 2 years ago.
Do as indicated in the comment above.
3570.3.diff (3.6 KB) - added by stanislas69 2 years ago.
Some fixes, thanks elexis for the feedback
3570.4.diff (3.6 KB) - added by stanislas69 2 years ago.
Fix a typo
3570.5.diff (3.1 KB) - added by stanislas69 19 months ago.
Rebased
3570.6.diff (3.9 KB) - added by stanislas69 19 months ago.
Should fix the above.
3570.7.diff (3.9 KB) - added by stanislas69 19 months ago.
grammar fix.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 2 years ago by scythetwirler

Not extremely urgent, but if we're breaking string freeze at the moment, we may as well address this. Push back if necessary.

comment:2 Changed 2 years ago by scythetwirler

Description: modified (diff)

comment:3 Changed 2 years ago by leper

Unknown reason is still the best we know. Adding something more about possible causes can't hurt though.

comment:4 Changed 2 years ago by elexis

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 Changed 2 years ago by scythetwirler

Most notably, people tend to blame something on their own side rather than the host's, leading them to leave/uninstall etc.

Changed 2 years ago by Mikita Hradovich

Attachment: 3570.diff added

Updated message for NDR_UNKNOWN

Changed 2 years ago by Mikita Hradovich

Attachment: screenshot0001.png added

Dialog with new message

comment:6 Changed 2 years ago by Yves

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 Changed 2 years ago by stanislas69

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:8 Changed 2 years ago by leper

Any updates?

comment:9 Changed 2 years ago by stanislas69

Keywords: review patch added
Owner: set to stanislas69
Status: newassigned
Summary: Change message for failed join to host.[PATCH] Change message for failed join to host.

Changed 2 years ago by stanislas69

Attachment: 3570.2.diff added

Should take into account leper's elexis' and scythetwirler's comments.

comment:10 Changed 2 years ago by elexis

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 calling getDisconnectReason from messages.js (disconnect from session)
  • true when calling reportDisconnect from gamesetup.js (disconnect from gamesetup)
  • false when calling reportDisconnect from gamesetup_mp.js (could not connect to server)

Not sure about the message itself.

  • open should be replaced with forwarded 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 Changed 2 years ago by stanislas69

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.

Changed 2 years ago by stanislas69

Attachment: 3570.2.2.diff added

Do as indicated in the comment above.

Changed 2 years ago by stanislas69

Attachment: 3570.3.diff added

Some fixes, thanks elexis for the feedback

comment:12 Changed 2 years ago by stanislas69

Keywords: review added

Changed 2 years ago by stanislas69

Attachment: 3570.4.diff added

Fix a typo

comment:13 Changed 2 years ago by leper

Milestone: Alpha 19Alpha 20

comment:14 Changed 2 years ago by elexis

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 Changed 2 years ago by leper

Keywords: review removed

comment:16 Changed 22 months ago by elexis

In 17710:

Show a more specific disconnect-message in case the host closed the server. Refs #3264, #3570.

comment:17 Changed 21 months ago by Itms

Milestone: Alpha 20Alpha 21

comment:18 Changed 19 months ago by elexis

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).

Changed 19 months ago by stanislas69

Attachment: 3570.5.diff added

Rebased

comment:19 Changed 19 months ago by elexis

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.

Changed 19 months ago by stanislas69

Attachment: 3570.6.diff added

Should fix the above.

Changed 19 months ago by stanislas69

Attachment: 3570.7.diff added

grammar fix.

comment:20 Changed 19 months ago by stanislas69

Keywords: review added

comment:21 Changed 19 months ago by elexis

Resolution: fixed
Status: assignedclosed

In 18176:

Show a hint when failed to connect to a game stating that it is likely caused by the host not forwarding the UDP port. Patch by Stan, fixes #3570.

comment:22 Changed 19 months ago by elexis

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
Last edited 19 months ago by elexis (previous) (diff)

comment:23 Changed 18 months ago by elexis

Component: UI & SimulationNetwork

(changed component to network)

Note: See TracTickets for help on using tickets.