Parallax subsystem & Highpop parallax disable config

[PowerfulBacon]

closes #6985

About The Pull Request

Parallax movement is no longer handled by atom movement, but instead by a parallax subsystem. This should make parallax less resource intensive at high pops, however may result in the parallax updates being slower/delayed. This also completely removes client_mobs_in_contents and replaces it with a signal driven version.

Why It's Good For The Game

Should help to slightly mitigate some high parallax CPU usage at higher pops. Needs testing on high pops to confirm this won't make it worse. This will especially affect ghosts and AI eyes, which trigger a ton of parallax updates due to their high move speed.

Testing Photographs and Procedure

Standard case:

https://user-images.githubusercontent.com/26465327/172351633-774b279b-1605-49e9-ac39-e7293b87dc8a.mp4

Extreme case (Setting the parallax system to run every second):

https://user-images.githubusercontent.com/26465327/172351763-69758961-5047-4e44-a05d-34dc4943bdbb.mp4

Changelog

:cl: refactor: Refactored parallax background to use a subsystem for updates. balance: Telekenisis can now pick up closets with mobs inside them (The reason for this change is due to the code misusing an internal variable that no longer exists). /:cl:

[PowerfulBacon]

https://user-images.githubusercontent.com/26465327/172372518-0d3a2fe4-31b5-45d7-9888-b78f73b9b965.mp4

[PowerfulBacon]

This doesn't seem to be working. Runtime which may be causing slowdown

[PowerfulBacon]

In fairness TiDi is really low and overtime is low, however this is extremely high on self CPU and has a lot of calls still. The amount of times this is being called needs to be compared with the old version to see if its being called more due to some issue

[PowerfulBacon]

Ok that helped a lot, but on high pop it might be worth just disabling the animations and lowering everyone's parallax quality forcibly

[PowerfulBacon]

Going to make it 10 ticks before the parallax update is free to skip the queue, since despite being the proc itself being more performant, the amount of times it is being called is increasing.

[PowerfulBacon]

I have absolutely no idea if this is working or not and it honestly might be worth just removing the instant update entirely

[PowerfulBacon]

It might also be worth making it not constantly update screen_loc

[PowerfulBacon]

lavaland sometimes goes invisible

[PowerfulBacon]

Registering and unregistering signals on one or more locs every time a client moves appears to be worse than what we had before performance wise

Signals are only registered and unregistered when the client changes loc, not when they move. If a mob moves from a turf to another turf, an extra signal gets sent and no registering happens at all (And the extra signal only happens on mobs with clients). The impact of this should be negligable compared to the rest of the movement. It could be related to signal handling, but without more information I can't look into this effectively. Another thing to support this not being more performant is the previous implementation of SSparallax. This code ran 5 times a second (Assuming no tick overrun). I am almost certain that this alone would be more than the signals.

In a short profile with 32 players (Around a minute or more), the signal handling proc parent_moved was inposition 328/8229 for self CPU usage. This additional cost is balanced by:

• Less updates of the parallax. (Even though parallax is still somewhat intense and a big strain on the server, it is less than it used to be)
• More efficient handling of calculating the client's contained position.

An incredibly inaccruate analysis

This analysis will be super inaccurate and isn't scientific at all. The numbers in this should not be used for or aganist this PR.

Taking scrubbers to scrub at a constant rate (This will depend on tick overrun, which low-pop)

After: Parallax cost is 1.349 at 32 pop with 200000 scrubs. Before: Parallax cost is 1.976 at 64 pop with 31000 scrubs.

Before we have a CPU usage rate of 996 nanocpuusesperpopperscrubs After we have a CPU usage rate of 210 nanocpuusesperpopperscrubs

Could you send more details about the performance impacts of this PR so I can investigate them and address the issue?

[PowerfulBacon]

Updated and tested

[PowerfulBacon]

The lavaland planet never seems to go away on shuttles if throttling is enabled

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[PowerfulBacon]

These reviews seem kind of nitpicky and inconsequential at this point.

There is, one less var on this object even if it's super minor improvement I see no point in not making this a define

Making a define for a variable that is literally only used once is not a good idea. It would spread the code across multiple files, only to have a define with the exact same name as the variable. If this number was used more than once, then yes, it would make sense; however this number is only used here and isn't compared anywhere else, meaning that there are no magic numbers anywhere. The variable and comment very clearly defines what the variable does and so, a define is not needed.

I mean that you are checking len of GLOB.clients twice

I am, the value I am comparing against is different in both. Storing and reusing the value of length(GLOB.clients) would provide a couple of nanoseconds of speed improvements at best, however would also require a variable assignation. The optimisations gained from storing length(GLOB.clients) is basically 0 and not worth the slightly more messy code that would come from creating a new variable and using that instead of length(GLOB.clients).

imo changing len instead of clearing the list seems at least weird and probably shouldn't be allowed if we had good language. I have no evidence for Cut() being faster or anything but it seems cleaner approach to me. If you want to change it go ahead, click the commit, if not just resolve this.

To maintain consistency with all other subsystems, I'll keep using .len = 0

Old code being bad isn't real excuse. In the long run making classes not partial is nice.

While I generally dislike making partial classes, having the parallax code for clients inside the parallax file allows these changes to be more modularised which assists with it being ported to other codebases. /client already has multiple different spread out uses. If we are going to put them all into one place, then I think its best we do it all at once, rather than make just parallax code stored in the giant /client class definition.

Unless you sleep somewhere or run async client won't get deleted here, so one check is enough

Clients can become null without sleeping.

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.