Pryaxis / TShock

☕️⚡️TShock provides Terraria servers with server-side characters, anti-cheat, and community management tools.
GNU General Public License v3.0
2.41k stars 377 forks source link

Initialize achievements and the `AchievementManager` on the server #2858

Closed drunderscore closed 1 year ago

drunderscore commented 1 year ago

This formally fixes #2661

See the first commit's message for fine details about the fix itself.

Note that these changes should never cause achievements to be awarded or saved on the server. Some methods in AchievementsHelper check the netMode to ensure it is not on the server. For the methods that do not, it is either checked at the call site that it is not the server, or simply unreachable by control flow on the server. If you do know of an instance where the server could award an achievement, do let me know, because it is undesirable.

Edit: To visualize what this PR is actually achieving (get it??), here's a demonstration

Chat message:

woah i just got the [a:KILL_THE_SUN] achievement!
thats way better than the [a:TOPPED_OFF] achievement
neither of those compare to the [a:THIS_IS_NOT_VALID_LMAO] achievement

Console (no longer throws, and properly logs message): image

punchready commented 1 year ago

How would the server even (accidentally) award achievements? There is no packet for it, only for NPC kills in packet 97, invasions in packet 98, and a few implicit ones from received packets, all of which are obviously part of the normal functionality.

drunderscore commented 1 year ago

@punchready By award achievements on the server, I meant the server invoking AchivementHelper methods and awarding them. Like I said it shouldn't happen, and it would already be a problem now if it was happening.

drunderscore commented 1 year ago

Alright, this is now ready for review. Apologies about the somewhat-unrelated test suite changes in 0ac814c, but I wanted to add a test for this and was unable to without these changes.

CC @SignatureBeef for the changes to the test suite, and the test itself making sense.

hakusaro commented 1 year ago

This is good-to-go if @SignatureBeef is okay with it.

hakusaro commented 1 year ago

Merged as of 24bb002e646c88dacb1d1d864f8c171b48784435 (I fucked the rebase up 🙃 )