0.76 support #555

iamgreaser · 2019-12-01T01:50:36Z

This is ready for review, and I'm very much expecting to see stuff that needs to be changed here.

This adds support for the 0.76 protocol. There's also some cleanups of the weapon stuff as we're doing a lot of stuff to pyspades/weapons.py here anyway.

A goal is to eventually be able to have clients speaking different protocols at the same time, which will be useful for a potential overhaul of the protocol as it stands. I don't promise that it will be a good idea to run any pair of protocol or game versions at the same time, but 0.75 and 0.76RC10 should be close enough to each other gameplaywise to not be absolutely awful.

NOTE: I ran tox on my machine and the build failures appear to be due to bcrypt not being pulled in in the requirements as part of twisted/conch/ssh.

Make mypy --strict happy and pylint less grumpy w/ pyspades.weapon This also fixes more potential bugs.

Currently does 0.75 and 0.76RC10 depending on the state of a constant. The 0.76 support currently lacks the extra map packet support which may need to be added back in somehow. Regardless, the game still works completely fine, although I haven't been testing this in the vanilla client.

…20191201-001

* Add `game_version` * Cleaned up WorldUpdate packets and how they're differentiated * Basic support for dual 0.75/0.76 servers, if anyone wants to mess around with that

This also contains some groundwork for enabling the ability to announce to multiple master servers. Currently there's only one master server per game version, but I can think of a way to extend this into something more generic.

…20191201-001

NotAFile · 2019-12-01T13:06:22Z

The tests appear to fail because the assert you added is hit by test_team_join.

iamgreaser · 2019-12-01T17:11:06Z

The tests appear to fail because the assert you added is hit by test_team_join.

Ah right, thanks for digging it up, turns out python3 -m unittest runs the tests just fine w/o hitting bad things not related to the tests, so the test you mentioned fails on my end as well.

Considering that the peer is a mock in this test, which of these would be a better way to test this?

Call on_connect with a side-effect tied to the mock's eventData field.
Force game_version to GAME_VERSION_AOS075 (or possibly iterate through a list of all valid versions).
Allow setting game_version in the ServerConnection constructor.

In future I'd be very much in favour of having unit tests inherit from a base class where one could do, say, self.make_new_server_connection() and it'd spit out a suitable ServerConnection instance.

…20191201-001

iamgreaser · 2019-12-02T04:23:59Z

I've gone with the second option for making the unit test pass as it does less damage to the structure of the unit tests.

NotAFile

Glad I finally got around to reviewing this. My overall thoughts are:

Enabling "zombie" servers adds a bunch of complexity. With no way to advertise them on the server list, complexity already being high, and more capable protocol extensions on the horizon, I'm not sure if this is still worthwhile.
The master server implementation is a bit dated to the current plans, by catering only to one master server per version.
Customizing weapon values in general is already something we desired (Allow simpler modification of weapon damage values #59). The 0.76 damage values can also be back-ported to 0.75 easily, since they are entirely server-side. This leaves only the ammo and accuracy changes, which I understand are a big deal to you though.

In general, I'd make it dependent on you if we want to proceed with this, or poach parts of it and rather put more energy behind using more capable extensions to modify the weapon values.

NotAFile · 2019-12-22T20:24:43Z

piqueserver/server.py

+        self.game_versions = set(map(
+            GAME_VERSION_FROM_STRING.__getitem__,
+            game_versions.get()
+        ))


generator expressions are more readable than map (often also faster):

set(GAME_VERSION_FROM_STRING[i] for i in game_versions.get())

NotAFile · 2019-12-22T20:29:03Z

pyspades/collision.py



-def vector_collision(vec1, vec2, distance=3):
+def vector_collision(vec1: Any, vec2: Any, distance: int = 3) -> bool:


The real type should be Vertex3 here?

NotAFile · 2019-12-22T20:32:59Z

pyspades/constants.py

+    GAME_VERSION_AOS_075,
+    GAME_VERSION_AOS_076RC10,
+
+    GAME_VERSION_COUNT,
+) = range(2+1)
+
+# Mapping of game versions to protocol versions.
+#
+# AoS since version 0.75 has used an increasing single-integer
+# protocol version number.
+#
+# AoS 0.70 and older use a CRC-32 of the client.exe binary.
+#
+# Some prerelease versions of 0.75:
+# - 0o075001: Not known. Possibly a test candidate version.
+#             Yes, that's an octal number.
+# - 1: 0.75RC12 and some older versions.
+# - 2: Some later release candidate.
+#
+# 0.76 seems to have stuck with version 4 as it was never
+# officially released as the mainline version.
+#
+GAME_PROTOCOL_VERSIONS = {
+    GAME_VERSION_AOS_075: 3,
+    GAME_VERSION_AOS_076RC10: 4,
+}


For new enums, the enum module should really be used.

NotAFile · 2019-12-22T20:33:32Z

pyspades/contained.pyx

+from pyspades.constants import GAME_VERSION_AOS_075
+from pyspades.constants import GAME_VERSION_AOS_076RC10


use commas to import multiple items

NotAFile · 2019-12-22T20:35:10Z

pyspades/contained.pyx

+        for idx in range(len(self.items)):
+            item = self.items[idx]


use for i, item in enumerate(self.items) instead of range(len())

NotAFile · 2019-12-22T21:12:52Z

pyspades/packet.pyx

-                       "with that id ({}) already exists.".format(cls, cls.id))
-                raise KeyError(msg)
-            _server_loaders[cls.id] = cls
+        for version in range(cls.since_version, cls.until_version, 1):


I'd prefer if instead of building the lookup table for all versions in between at initialization, a loader dictionary for a specific version was requested via parameters (and then cached).

NotAFile · 2019-12-22T21:17:57Z

pyspades/server.py

+from pyspades.constants import GAME_PROTOCOL_VERSIONS
+from pyspades.constants import GAME_VERSION_TO_STRING


imports again

NotAFile · 2019-12-22T21:19:48Z

pyspades/server.py

+        # 0.76RC10+ compact world update
+        world_update_076 = loaders.WorldUpdate076()
+        world_update_076.items = items[:highest_player_id+1]
+        self.send_contained(world_update_076, unsequenced=True)
+


It seems unclean to construct the packets for all versions versions here.

NotAFile · 2019-12-22T21:22:09Z

pyspades/server.py

+        log.info('Got master connection for version {!r}'.format(
+            GAME_VERSION_TO_STRING[connection.game_version],
+        ))
+        self.master_connections[connection.game_version] = connection


This could probably get a lot cleaner using async/await.

The restriction on only differentiating master connections only by version has obviously not aged that well.

NotAFile · 2019-12-22T21:28:19Z

pyspades/weapon.py

+        position1: The starting position of the shot.
+        position2: The position of the target we're hitting.
+        """
+        raise NotImplementedError()


Abstract methods on ABCs that are not overriden raise automatically on class creation.

iamgreaser added 9 commits December 1, 2019 11:39

Start splitting pyspades/weapon.py versionwise

e856dec

Hide weapon class map behind function call

34b0218

Throw mypy at pyspades.weapon and fix likely bugs

f809255

pyspades.weapon lint and strict check

2c7bac0

Make mypy --strict happy and pylint less grumpy w/ pyspades.weapon This also fixes more potential bugs.

pyspades.collision mypy --strict + pylint cleanup

3cb07d8

Merge remote-tracking branch 'origin/master' into gm-prs/076-support-…

965069c

…20191201-001

0.76 support updates

e33deb1

* Add `game_version` * Cleaned up WorldUpdate packets and how they're differentiated * Basic support for dual 0.75/0.76 servers, if anyone wants to mess around with that

Add 0.76 master server support

61ad115

This also contains some groundwork for enabling the ability to announce to multiple master servers. Currently there's only one master server per game version, but I can think of a way to extend this into something more generic.

iamgreaser marked this pull request as ready for review December 1, 2019 06:23

Merge remote-tracking branch 'origin/master' into gm-prs/076-support-…

59da77c

…20191201-001

NotAFile added the PR: improvement This PR includes an enhancement to piqueserver label Dec 1, 2019

iamgreaser added 2 commits December 2, 2019 17:18

Merge remote-tracking branch 'origin/master' into gm-prs/076-support-…

cf27a3e

…20191201-001

Get the unit tests passing w/ a game version

07faf53

NotAFile reviewed Dec 22, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.76 support #555

0.76 support #555

iamgreaser commented Dec 1, 2019 •

edited

NotAFile commented Dec 1, 2019

iamgreaser commented Dec 1, 2019 •

edited

iamgreaser commented Dec 2, 2019

NotAFile left a comment

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019

NotAFile Dec 22, 2019



		def vector_collision(vec1, vec2, distance=3):
		def vector_collision(vec1: Any, vec2: Any, distance: int = 3) -> bool:

		from pyspades.constants import GAME_VERSION_AOS_075
		from pyspades.constants import GAME_VERSION_AOS_076RC10

		from pyspades.constants import GAME_PROTOCOL_VERSIONS
		from pyspades.constants import GAME_VERSION_TO_STRING

0.76 support #555

Are you sure you want to change the base?

0.76 support #555

Conversation

iamgreaser commented Dec 1, 2019 • edited

NotAFile commented Dec 1, 2019

iamgreaser commented Dec 1, 2019 • edited

iamgreaser commented Dec 2, 2019

NotAFile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamgreaser commented Dec 1, 2019 •

edited

iamgreaser commented Dec 1, 2019 •

edited