Add TClientConnection.GetIdentifier in CastleClientServer.pas

1 
TClientConnection = record
private
{$ifndef ANDROID}
OtherID : String;
Context: TIdContext;
{$else}
ID: String;
{$endif}
public
constructor Create({$ifdef ANDROID}AID: String{$else}AContext: TIdContext{$endif});
function GetIdentifier: String;
end;


constructor TClientConnection.Create({$ifdef ANDROID}AID: String{$else}AContext: TIdContext{$endif});
begin
{$ifdef ANDROID}
ID := AID;
{$else}
Context := AContext;
OtherID := IntToStr(Random($8000000)) + IntToStr(NativeUInt(@Context));
{$endif}
end;

function TClientConnection.GetIdentifier: String;
begin
{$ifdef ANDROID}
Result := ID;
{$else}
Result := OtherID;
{$endif}
end;
2

Than you! But when proposing feature requests and / or patches, you need to:

  1. Describe why do you propose a given code change. What is the benefit from having this, what does it allow to achieve the developer (that is not possible right now, or that is not comfortable now).

  2. Also, the changes are better described as pull request, not by pasting code in a forum. Then we can see the “diff”. See Coding Conventions | Manual | Castle Game Engine , “Submitting your code contributions”.

The “diff”, analyzing your snippets, is that

  • you add OtherID on non-Android, which is random, and not used for anything else.
  • And expose TClientConnection.GetIdentifier: String, from either Id or OtherId.

But I don’t know why. TClientConnection contains now internal information, with platform-specific Context / Id. User code should not need to access them (and consequently, we should not need to invent OtherID).

Please write an explanation why is this necessary for us to include this if you wish to propose adding it to the engine :slight_smile:

3

when TcpServer.HandleServerDisconnected(AClient: TClientConnection);

AClient.GetIdentifier can know which AClient Disconnected, in lan game.

I can not edit my post,

After testing， anroid connect window in lan game,anroid can be server,window can be server。Theoretically, iOS is also possible.

fix error OtherID := InttoStr(PtrUInt(Context));

begin
 Li := Length(PlayClientList);
 for i := 0 to Li - 1 do
 begin
  if SameText(PlayClientList[i].Tcpid, AClient.GetIdentifier) then
  begin
    PlayClientList[i].Tcpid:= ‘’;
    PlayClientList[i].FName:= ‘’;
    PlayClientList[i].TCPConnect.Init;
    if ServerRoomInfo.CurrentPlayers > 0 then      Dec(ServerRoomInfo.CurrentPlayers);
    Break;
  end;
 end;
end;

HandleServerConnected(AClient: TClientConnection);
begin
   WritelnLog(‘GameTcpServer.HandleConnected_’,AClient.GetIdentifier);
end;

HandleMessageReceived(const AMessage: String; AClient: TClientConnection);

begin

WritelnLog(‘HandleMessageReceived_’+AClient.GetIdentifier ,AMessage);

end;

4

Thanks for making it clear!

  1. I recognize it would be nice to have some useful identifier for TClientConnection for debug purposes. But then it could be something simpler (than IntToStr(Random($8000000)) + IntToStr(NativeUInt(@Context))) and preferably assigned in the same way for Android and non-Android (to make it easier to debug for us). Like: just an integer, assigned in sequence, each one +1 bigger.

    Actually, TClientConnection should just change to be a class instance, not a record. I can see it would make extending it simpler. I think this is my preferred solution.

    Then, one could just use NativeUInt(TClientConnection) or TClientConnection.GetHashCode for an id.

    It can also then preserve some additional information, like IP, port. Right now these are inside Indy (on non-Android) or behind Android “opaque id” managed in ServiceClientServer.java. It would be nice to expose this information as new TClientConnection properties/methods, that work on both Android and non-Android, to enable investigating “what does this TClientConnection represent” better.

  2. To solve the need to compare them, like you do SameText(PlayClientList[i].Tcpid, AClient.GetIdentifier), you can right now just store TClientConnection and then compare them using CompareMem. I.e. you don’t need to invent a string identifier for this. I.e. make PlayClientList[...].Tcpid be of TClientConnection type, and compare them like if CompareMem(@PlayClientList[i].Tcpid, @AClient, SizeOf(TClientConnection)) then ....

  3. An empty record can be Default(TClientConnection). To solve the need for doing PlayClientList[i].Tcpid:= ‘’ in your example code. So you would instead write PlayClientList[i].Tcpid:= Default(TClientConnection).

Summary:

  • For your needs, I think you don’t need the GetIdentifier:String really :slight_smile: Just store TClientConnection, treat it as “opaque” type, compare it using CompareMem, assign “nothing” using Default(TClientConnection).

  • I would welcome PR to change TClientConnection to be class instance, not a record, and add there methods/properties that expose more info, for debug purposes.

    Of note is that if/when we do this, my advise to use CompareMem or Default(TClientConnection) will still work. CompareMem will then compare pointers (class instances) and Default(TClientConnection) will yield nil, which both will still achieve the same thing. So, changing TClientConnection from record → class will be backward-compatible in this regard.

1 Like
5

yes, you are right!!! I’m confused between Android and Windows.

after test, indy (tcp and udp) can work well (in CGE, android connect windows)