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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jul 14, 2019
Merged

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

merged 1 commit into from
Jul 14, 2019
+21 −17

Conversation

ldpl
Copy link
Contributor

@ldpl 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 •
edited
Loading

@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 client_question_quick_fix branch from 8d05111 to e52c8a1 Compare May 3, 2019 10:22
@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 client_question_quick_fix branch 2 times, most recently from 86ff9d4 to 2ff28f5 Compare July 1, 2019 12:46
@ldpl ldpl force-pushed the client_question_quick_fix branch from 2ff28f5 to 024ff92 Compare July 1, 2019 12:48
@nielsmh nielsmh added the backport requested This PR should be backport to current release (RC / stable) label Jul 1, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Jul 5, 2019

OpenTTD/src/network/network_type.h

Lines 17 to 25 in a0c78c7

/** 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;

OpenTTD/src/network/network_type.h

Lines 40 to 48 in a0c78c7

/** '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;

OpenTTD/src/network/network_server.h

Lines 21 to 22 in a0c78c7

/** 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.

OpenTTD/src/network/network_server.cpp

Line 212 in a0c78c7

this->client_id = _network_client_id++;

michicc pushed a commit to michicc/OpenTTD that referenced this pull request Jul 7, 2019
@ldpl @michicc
Fix: Make GSGoal.QuestionClient work correctly at least for clients w…
21a594b 
…ith ID < 2**16 (OpenTTD#7560)
@michicc michicc mentioned this pull request Jul 7, 2019
Merged
LordAro
LordAro reviewed Jul 7, 2019
View reviewed changes
src/goal.cpp
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pushed a commit that referenced this pull request Jul 7, 2019
@ldpl @michicc
Fix: Make GSGoal.QuestionClient work correctly at least for clients w…
c625142 
…ith ID < 2**16 (#7560)
michicc
michicc approved these changes Jul 14, 2019
View reviewed changes
Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did put it in 1.9.2, so...

@michicc michicc added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Jul 14, 2019
@LordAro LordAro merged commit 36e4bd4 into OpenTTD:master Jul 14, 2019
@ldpl ldpl deleted the client_question_quick_fix branch September 4, 2020 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LordAro LordAro

@michicc michicc

Assignees
No one assigned
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GSGoal.QuestionClient sends message to wrong client
5 participants
@ldpl @PeterN @nielsmh @michicc @LordAro