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

Unreal2: Add extra server info parsing #145

Open
Douile opened this issue Oct 30, 2023 · 2 comments
Open

Unreal2: Add extra server info parsing #145

Douile opened this issue Oct 30, 2023 · 2 comments
Labels
enhancement New feature or request protocol This is something regarding a protocol
Milestone

Comments

@Douile
Copy link
Collaborator

Douile commented Oct 30, 2023

What is this feature about?
Some games that use the unreal2 protocol include extra data at the end of the ServerInfo response. The node version detects this based on the rules/mutators response and goes back to parse the extra data.

We could do the same, or specify which game we are querying in GatheringSettings and decide whether to try to gather this data based on that.

Additional context/references
Node-gamedig implementation

@Douile Douile added enhancement New feature or request protocol This is something regarding a protocol labels Oct 30, 2023
@CosminPerRam
Copy link
Member

CosminPerRam commented Nov 9, 2023

As per node-gamedig's implementation:
If it's an unreal2 response it will get the following fields:
ping
If it's extended ("These fields were added in later revisions [...]":
flags, skill
If the response is from Killing Floor, it has the following fields:
ping, flags, skill (these are already present in unreal extended) and wavecurrent, wavetotal.

I think [...] specify which game we are querying in GatheringSettings and decide whether to try to gather this data based on that. could be avoided, with the following solution proposals:

For the simplicity's sake (as I do not think we will have another game that will also report more fields here), we could just add them to the response (extra info) as Option's (ping is always present, so without that one).
Advantages:

  • Simpler implementation, like the node one.

Disadvantages:

  • Wasting 2 options worth of memory on every query that isn't killing floor.

Another solution would be to always parse the ping (and the extended fields) and add a new implementation that parses them correctly afterwards, sort of what JC2M does, but just for this extra info.
Advantages:

  • Properly avoiding game-specific implementation in a protocol.
  • Simpler response type.

Disadvantages:

  • On an extended unreal2 response, the following parsing happens: u32, u32 and an unreal string.
    On a Killing Floor response: u32, u32, u32, u32 and an unreal string.
    So there is a possibility that when parsing a Killing Floor response as an extended Unreal2 response that it will fail, but this could be avoided by checking if the response is from it and if so, dont parse it yet, this also means storing the extended info in the struct (similar to the first solution's disadvantage).
  • Possible more code needed to do 'from' parsing and for common response.

Thoughts about this? Got any other ideas going?

@Douile
Copy link
Collaborator Author

Douile commented Nov 11, 2023

Wasting 2 options worth of memory on every query that isn't killing floor.

2 Option<u32> is not a lot, especially since allocation will likely be rounded.

Another solution would be to always parse the ping (and the extended fields) and add a new implementation that parses them correctly afterwards, sort of what JC2M does, but just for this extra info.

I'm not sure how well this would work because we don't know how many u32 are before the string and the string is indistinguishable from u32s. I think the extended/killingfloor parsing works best given the remaining &[u8] after parsing standard headers.

I do like the idea the idea of the game-specific impl of killing floor though: maybe get_server_info() could return the remaining bytes ((ServerInfo, &[u8]) or (ServerInfo, Buffer)). Then the game-specific impl could handle that.

I'm not sure the extended impl should be in src/games though as it applies to multiple games, maybe Unreal2 could have a standard_query and a extended_query, or pass extended: bool to the query function. If doing this though I kinda feel like maybe killing floor shouldn't be a game specific impl as it is 1/4 of the unreal2 protocol.

The problem with game-specific impls here is that by doing so we restrict our ability to do detection of what kind of packet we get at parse time: the caller must know what kind of packet the server will return so they can decide which impl to call.

Simpler implementation, like the node one.

I do think that specifying what kind of query we want in GatherSettings is simpler than the node-implementation as it means we don't have to implement the server kind detection logic. However it does make our parser more strict than the node one: it would fail for servers that node would work for.


I think a good way to do this would be:

  1. Return the remaining buffer from get_server_info() -> (ServerInfo, Buffer)
  2. Create an enum for extra info response
enum Unreal2ExtraServerInfo {
  Extended {
    /* ... */
  },
  KillingFloor {
   /* ... */
  },
  None,
}
  1. After fetching rules do the same detection as node, then attempt to parse the remaining buffer from get_server_info() into this enum accordingly
  2. Add enum to the response:
pub struct Response {
    pub server_info: ServerInfo,
    pub mutators_and_rules: MutatorsAndRules,
    pub players: Players,
    pub extra_server_info: Unreal2ExtraServerInfo,
}

Advantages:

  • Matches how node handles parsing
  • Doesn't over-complicate the ServerInfo response type by needing to store un-parsed or possibly parsed data there

Disadvantages:

  • Game-specific impl is in the protocol query function
  • Caller has no way to specify exactly how they want the response parsed: e.g. only wanting to parse killing floor responses
  • To access the extra data caller has to unwrap an enum
  • If game detection logic is wrong extra info parsing could fail (this is also a problem with the node impl)

Got any other ideas going?

I was thinking of doing a trait based impl of this in #148 but that has the same issue of game detection as here, so detection logic would be the same.

@cainthebest cainthebest added this to the Backlog milestone Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request protocol This is something regarding a protocol
Projects
None yet
Development

No branches or pull requests

3 participants