docs: NetworkConfig.ClientConnectionBufferTimeout [skip ci]#1927
Merged
JesseOlmer merged 3 commits intodevelopfrom May 5, 2022
Merged
docs: NetworkConfig.ClientConnectionBufferTimeout [skip ci]#1927JesseOlmer merged 3 commits intodevelopfrom
JesseOlmer merged 3 commits intodevelopfrom
Conversation
Add more detail to where in the process the setting takes effect, how it relates to transport-level timeouts, and indicates that it's a server-side setting only.
| /// <summary> | ||
| /// The amount of seconds to wait for handshake to complete before timing out a client | ||
| /// The amount of seconds for the server to wait for the connection approval handshake to complete before the client is disconnected. | ||
| /// |
Contributor
There was a problem hiding this comment.
Suggested change
| /// | |
| /// This could happen if for example you're calling external services to authenticate your users and those services timeout. |
For users that never worked on connected games, this could help.
Contributor
Author
There was a problem hiding this comment.
This isn't the appropriate type of comment to add to a Summary section. I'll add some similar flavor to Remarks
| /// The period begins after the <see cref="NetworkEvent.Connect"/> is received on the server. | ||
| /// The period ends once the server finishes processing a <see cref="ConnectionRequestMessage"/> from the client. | ||
| /// | ||
| /// This setting is independent of any Transport-level timeouts that may be in effect. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// This setting is independent of any Transport-level timeouts that may be in effect. | |
| /// This setting is independent of any Transport-level timeouts that may be in effect. To calculate the overall connection timeout for a player connecting to a server, add transport + buffer timeout. |
Contributor
Author
There was a problem hiding this comment.
In the new draft there are already 3-4 other references to the Transport timeout so I don't feel this explicit calculation one is necessary. It's also not necessarily accurate depending on Transport implementations.
Contributor
There was a problem hiding this comment.
How about something like "a player's total connection timeout would be influenced by both transport side (if any) and network manager side timeouts". It'd just help give the big picture to users, right now we're showing this as if they were two tech in silos.
SamuelBellomo
approved these changes
May 4, 2022
Contributor
SamuelBellomo
left a comment
There was a problem hiding this comment.
added some nitpicking, looks good otherwise
added 2 commits
May 5, 2022 08:14
Add more context and detail to the time period affected by this setting.
ashwinimurt
pushed a commit
that referenced
this pull request
May 5, 2022
…ns [skip ci] (#1927) Add more detail to where in the process the setting takes effect, how it relates to transport-level timeouts and indicates that it's a server-side setting only.
ashwinimurt
pushed a commit
that referenced
this pull request
May 5, 2022
…ns [skip ci] (#1927) Add more detail to where in the process the setting takes effect, how it relates to transport-level timeouts and indicates that it's a server-side setting only.
0xFA11
added a commit
that referenced
this pull request
May 5, 2022
* chore: Separate trunk tests and trigger weekly (#1926) * chore: Separate trunk tests and trigger weekly * Rename to weekly * Turn off badges trigger dependencies on trunk * Run Code Coverage on validation_editor 2020.3 * fix: Fix code coverage config name in nightly trigger (#1933) * docs: NetworkConfig.ClientConnectionBufferTimeout xmldoc clarifications [skip ci] (#1927) Add more detail to where in the process the setting takes effect, how it relates to transport-level timeouts and indicates that it's a server-side setting only. * chore: update `CODEOWNERS` (#1935) Co-authored-by: Jesse Olmer <jesseo@unity3d.com> Co-authored-by: Fatih Mar <mfatihmar@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add more detail to where in the process the setting takes effect, how it relates to transport-level timeouts and indicates that it's a server-side setting only.
Note that in documenting this, I discovered that connection approval timeout appears bugged/inoperable on the client-side and has been that way for a very long time. Will file a follow-up issue.
Changelog
Testing and Documentation