Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make GSGoal.QuestionClient work correctly at least for clients with ID < 2**16 #7560

Merged
merged 1 commit into from Jul 14, 2019

Conversation

@ldpl
Copy link
Contributor

ldpl commented May 3, 2019

Partially fixes #7534.
It's a good temporary fix as client ids almost never get that big and properly fixing it is much more complicated.

Way to test:

  1. start server with gs
    testclientquestion.zip
  2. connect with another client (1)
  3. connect with another client (2)
  4. leave server with client (1)
  5. connect with another client (3)
    client 3 won't get welcome message without this fix (in 1.9.1)
@PeterN
Copy link
Member

PeterN commented May 3, 2019

As GOAL_QUESTION_BUTTON_COUNT is 18, this leaves 14 bits available in p2, into which you could move the type and is_client parameters, and then use the full client ID.

@ldpl
Copy link
Contributor Author

ldpl commented May 3, 2019

@PeterN AFAICT full client_id is 32 bits > 27 = 13 + 14 so it won't be a full fix either. And I can't think of an easy way to squeeze buttons in 13 bytes even though it's theoretically possible (coz 1-3 bits only). I can send 16 bits of client id though instead of 13 if you insist.

@ldpl ldpl force-pushed the ldpl:client_question_quick_fix branch from 8d05111 to e52c8a1 May 3, 2019
@ldpl ldpl changed the title Make GSGoal.QuestionClient work correctly at least for clients with ID < 2**13 Make GSGoal.QuestionClient work correctly at least for clients with ID < 2**16 May 3, 2019
@ldpl ldpl force-pushed the ldpl:client_question_quick_fix branch 2 times, most recently from 86ff9d4 to 2ff28f5 Jul 1, 2019
@ldpl ldpl force-pushed the ldpl:client_question_quick_fix branch from 2ff28f5 to 024ff92 Jul 1, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Jul 5, 2019

/** How many clients can we have */
static const uint MAX_CLIENTS = 255;
/**
* The number of slots; must be at least 1 more than MAX_CLIENTS. It must
* furthermore be less than or equal to 256 as client indices (sent over
* the network) are 8 bits. It needs 1 more for the dedicated server.
*/
static const uint MAX_CLIENT_SLOTS = 256;

/** 'Unique' identifier to be given to clients */
enum ClientID {
INVALID_CLIENT_ID = 0, ///< Client is not part of anything
CLIENT_ID_SERVER = 1, ///< Servers always have this ID
CLIENT_ID_FIRST = 2, ///< The first client ID
};
/** Indices into the client tables */
typedef uint8 ClientIndex;

/** Pool with all client sockets. */
typedef Pool<NetworkClientSocket, ClientIndex, 8, MAX_CLIENT_SLOTS, PT_NCLIENT> NetworkClientSocketPool;

As far as I can tell none of this would allow a ClientID to actually break the 8 bit limit, despite the ClientID type likely mapping to int storage. Mainly since the pool defined in network_server.h uses 8 bit indices.

If you have a way to reproduce ClientIDs > 255 do share.

@michicc
Copy link
Member

michicc commented Jul 7, 2019

ClientID is not ClientIndex. The client ID is monotonically increased by the server for each connecting client and never reused.

this->client_id = _network_client_id++;

michicc added a commit to michicc/OpenTTD that referenced this pull request Jul 7, 2019
ClientIndex client = (ClientIndex)GB(p1, 16, 8);
byte type = GB(p1, 24, 2);
bool is_client = HasBit(p1, 31);
ClientID client = (ClientID)GB(p1, 16, 16);

This comment has been minimized.

Copy link
@LordAro

LordAro Jul 7, 2019

Member

If ClientIndex is the "proper" type here (as discussed previously), why the change to ClientID? I'm not a fan of using arbitrary values for enum types, especially potentially well beyond their defined ranges, as certain aggressive gcc optimisations can truncate enum types or skip range checks...

This comment has been minimized.

Copy link
@michicc

michicc Jul 7, 2019

Member

If I'm understanding the issue right, the problem is that ClientIndex is a local value and only ClientID is in fact synced across the network.

michicc added a commit that referenced this pull request Jul 7, 2019
Copy link
Member

michicc left a comment

We did put it in 1.9.2, so...

@LordAro LordAro merged commit 36e4bd4 into OpenTTD:master Jul 14, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190701.5 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.