Optionally add prefix to bot names

[wight-airmash]

The prefix should also not be available for use by players to prevent mimicry.

From #83.

[congratulatio]

Alternative suggestion: broadcast a new type of SERVER_CUSTOM message that contains player IDs of bots, and have the frontend make some UI change for those players that is not possible to mimic through choice of name, e.g. differently coloured prefix text, or some special bot icon or flag.

I think this will be most useful for servers with ALLOW_NON_ASCII_USERNAMES enabled, as Unicode permits a variety of approaches for mimicry, and it is difficult to mitigate (UTR #36, UTR #39).

For backward compatibility, have compatible frontends send an additional attribute in the LOGIN.session field that indicates support for this feature. Players that connect without this may receive a prefix instead.

[wight-airmash]

Another alternative suggestion: using a predefined bots id pool. For example the first 1000 identifiers (starting with 1024, the current minimal mob id) will be reserved for the bots. This amount should be enough for any purpose. In this case, prefixes are always added to bot names. Updated frontend clients can remove them by template and indicate bots with its own methods.

This solution doesn't require a protocol update, but isn't as flexible as @congratulatio suggested.

[congratulatio]

That sounds like it may be simpler to implement on the server side, but I think there would be a compatibility issue with steamroller-airmash/airmash-server. As I understand it, it issues nearly the full range of IDs to players, with only 0, 1 and 2 reserved.

This would need some detection on the updated frontend client so it knows it has connected to a server with a reserved ID ranges for bots, perhaps also a new SERVER_CUSTOM message type.

[wight-airmash]

Yes, you're right.

Since we are going to update the protocol, I have a suggestion to update it a little differently.

As you mentioned above, if we change the SERVER_CUSTOM data, it will require additional checks of the client version on the server side. Yes, now it will be only one protocol difference for different clients, but what if we start to accumulate these changes and need more checks for different packets. Basically, nothing bad, but we can do without checks at all and not be afraid to forget about them when updating the server code.

I would suggest not changing, but adding data to existing packets. The frontend packet parser is pretty universal and it allows us to add new fields to the end of packets, such changes won't cause any errors. We need to know the bot identifiers after joining the game and whether the player who joins is a bot, i.e. we need to add data to LOGIN and PLAYER_NEW, for example:

[SERVER_PACKETS.LOGIN]: [
  ['success', DATA_TYPES.boolean],
  ['id', DATA_TYPES.uint16],
  ['team', DATA_TYPES.uint16],
  ['clock', DATA_TYPES.uint32],
  ['token', DATA_TYPES.text],
  ['type', DATA_TYPES.uint8],
  ['room', DATA_TYPES.text],
  [
    'players',
    DATA_TYPES.array,
    [
      ['id', DATA_TYPES.uint16],
      ['status', DATA_TYPES.uint8],
      ['level', DATA_TYPES.uint8],
      ['name', DATA_TYPES.text],
      ['type', DATA_TYPES.uint8],
      ['team', DATA_TYPES.uint16],
      ['posX', DATA_TYPES.coordx],
      ['posY', DATA_TYPES.coordy],
      ['rot', DATA_TYPES.rotation],
      ['flag', DATA_TYPES.uint16],
      ['upgrades', DATA_TYPES.uint8],
    ],
  ],
  ['botsNamePrefix', DATA_TYPES.text],
  [
    'bots',
    DATA_TYPES.array,
    [
      ['id', DATA_TYPES.uint16],
    ],
  ],
]

[SERVER_PACKETS.PLAYER_NEW]: [
    ['id', DATA_TYPES.uint16],
    ['status', DATA_TYPES.uint8],
    ['name', DATA_TYPES.text],
    ['type', DATA_TYPES.uint8],
    ['team', DATA_TYPES.uint16],
    ['posX', DATA_TYPES.coordx],
    ['posY', DATA_TYPES.coordy],
    ['rot', DATA_TYPES.rotation],
    ['flag', DATA_TYPES.uint16],
    ['upgrades', DATA_TYPES.uint8],
    ['isBot', DATA_TYPES.boolean],
],

What do you think?

[congratulatio]

Nice! And very useful that the frontend packet parser makes this possible. As we have this option, I agree that both logically and practically it makes more sense to hold these additional fields within the same LOGIN or PLAYER_NEW packet, rather than sending another packet that the client must parse separately.

Would this also be accepted by other clients, e.g. the Q bots on CTF? I think the FFA bots reuse the frontend packet parsing code, so those should be okay, but I will check.

[wight-airmash]

the Q bots on CTF?

I only tested with an extended LOGIN packet, they worked well, no errors in their logs.

[congratulatio]

I've been experimenting with this in these forks/branches: https://github.com/congratulatio/ab-protocol/commits/bot-players https://github.com/congratulatio/ab-server/commits/bot-players https://github.com/airmash-refugees/airmash-frontend/commits/bot-players

This implements just your LOGIN.bots and PLAYER_NEW.isBot fields for now, and some UI based on the level plate. I've not looked at adding/removing of name prefixes yet.

Frontend for testing, server is "Bot players test" in US region: https://bot-players.test.airmash.online/#us-bot-players-test

[peace2000]

I checked that test server. Looked funny as those bots had a robot icon :D but anyway, it's good that bots can be recognized easily!

[wight-airmash]

@congratulatio, based on your changes I added prefix setup with BOTS_NAME_PREFIX (no prefix by default). The feature is ready in the feature/bot-markers branch. The protocol dependency already points to the right branch.

If I got it right, spatie bots work as well as original and starmash clients, so a little later I'll test Q-bots again and merge changes.

I updated the readme (how to update schemas) and fixed generators (noticed you had to fix eslint errors after them).

[congratulatio]

Thanks that's great, I've updated the server to run your changes with a prefix of "ᵇᵒᵗ " for testing, and added bot prefix removal to the frontend in https://github.com/airmash-refugees/airmash-frontend/commit/086aeca852820293f72b4f2aeb8e32cb806f5ce6.

It seems to be working as intended: https://bot-players.test.airmash.online/#us-bot-players-test versus https://test.airmash.online/#us-bot-players-test

[congratulatio]

I checked that test server. Looked funny as those bots had a robot icon :D but anyway, it's good that bots can be recognized easily!

Agreed it does look kind of funny, I've opened an issue in the frontend to track improving the bot marker UI: https://github.com/airmash-refugees/airmash-frontend/issues/24

[wight-airmash]

Q-bots work well with extended packets, but crash with custom SERVER_CUSTOM content:

# ERR: Beagle 2, Deserialize(DeserializeError { ty: InvalidEnumValue(128), trace: [FieldSpec { field: Name("ty"), ty: Some("server::ServerCustom") }, FieldSpec { field: Name("ServerCustom"), ty: Some("ServerPacket") }] })
# ERR: Lunokhod 3, Deserialize(DeserializeError { ty: InvalidEnumValue(128), trace: [FieldSpec { field: Name("ty"), ty: Some("server::ServerCustom") }, FieldSpec { field: Name("ServerCustom"), ty: Some("ServerPacket") }] })
# ERR: Sojourner, Deserialize(DeserializeError { ty: InvalidEnumValue(128), trace: [FieldSpec { field: Name("ty"), ty: Some("server::ServerCustom") }, FieldSpec { field: Name("ServerCustom"), ty: Some("ServerPacket") }] })

[congratulatio]

Thank you checking this - that is unfortunate, I really should have tested the Q-bots before submitting https://github.com/wight-airmash/ab-server/pull/88.

What do you reckon if we change LOGIN.botsNamePrefix field to LOGIN.serverConfiguration and send the config JSON there, with the bot name prefix included in that data?

[wight-airmash]

This fix https://github.com/wight-airmash/ab-server/commit/c1484442c5fbee2af1239a08d3f9a5857edcfc98 doesn't really determine that a connected bot is a Q-bot, but it uses CTF_QBOTS_FEATURES, the true value of which assumes Q-bots usage. Later, I'll try to do a more accurate check based on the request headers from the bots.

What do you reckon if we change LOGIN.botsNamePrefix field to LOGIN.serverConfiguration and send the config JSON there, with the bot name prefix included in that data?

Yes, let's change it. Unfortunately, we won't be able to update the data if we need to, as it was with SERVER_CUSTOM, but we can do that by adding a field to some other packet.

[congratulatio]

Yes, let's change it. Unfortunately, we won't be able to update the data if we need to, as it was with SERVER_CUSTOM, but we can do that by adding a field to some other packet.

Thanks, I've submitted PRs for this change in https://github.com/wight-airmash/ab-server/pull/92 and https://github.com/wight-airmash/ab-protocol/pull/7.

Also I tried adding a new packet type and testing with the Q-bots, but got the same sort of error you saw for the SERVER_CUSTOM enum. Looks like it's strict on everything except packet length.

[wight-airmash]

Resolved in v5.5.0

[congratulatio]

@wight-airmash, in case we need this for the future, I've made a patched copy of the Q-bots binary that doesn't exit upon a deserialize error: https://github.com/airmash-refugees/Q-bots