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

(code bounty) refactors all uses of Crossed() and Uncrossed() into signals sent to loc, tracked by connect_loc #58340

Merged
merged 103 commits into from May 7, 2021

Conversation

Kylerace
Copy link
Contributor

@Kylerace Kylerace commented Apr 12, 2021

About The Pull Request

this pr is for a code bounty by @optimumtact

original pr here #58210, can be kept as a backup in case all the extra elements cause ooms or something (probably ridiculously unlikely now that i think of it)

the for loops in doMove and Move that called Crossed() and Uncrossed() on every movable in loc and oldloc no longer exist, now it simply sends a signal to the relevant turf, and if anything on that turf wants to listen to COMSIG_MOVABLE_CROSSED or the uncrossed equivalent, it uses the connect_loc element to listen to the turf. this way you're not calling Crossed() and Uncrossed ~n times each every movement over a tile with n objects in contents.

also changes connect_loc to now register to a new signal, COMSIG_MOVABLE_LOCATION_CHANGE. its emitted right after the moving atoms loc changes in Move() and doMove(). because the crossed and uncrossed signals are emitted before Moved() is called, there are times where a proc registered to crossed can qdel or move the crossing atom, meaning that connect_loc can try to access the targets list with the targets current position which was never put into the targets list because Moved wasnt called yet in the process.

NOTE: for future people using git blame to find this pr, it uses the Moved() signal again not LOCATION_CHANGE

squashable is a component now, with how crossed is changed theres no way for an element to get a reference to its target in any signal handler registered for crossed, since the crossed and uncrossed signals are sent to the turf that its target is on

Why It's Good For The Game

Massive performance improvement when theres many moving objects in a few tiles,

this is how fast the tram moves on master with three part replacers worth of objects on it
https://user-images.githubusercontent.com/15794172/114290043-77dd5100-9a31-11eb-869d-3927582ba064.mp4

this is how fast the tram moves with these changes, same number of objects on it in the same spots
https://user-images.githubusercontent.com/15794172/114290056-95121f80-9a31-11eb-910f-d28a24cb17a8.mp4

Changelog

🆑
refactor: tram is marginally faster
/:cl:

Mothblocks and others added 30 commits April 6, 2021 00:34
Co-authored-by: Emmett Gaines <ninjanomnom@gmail.com>
As in, the listener and tracked objects can be different things
@Mothblocks Mothblocks added the Test Merge Candidate You're our unpaid test team label May 4, 2021
Copy link
Member

@Mothblocks Mothblocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TM'd on Terry, will merge if no complications (or if they're extremely minor).

@github-actions github-actions bot added the Merge Conflict Adding upstream files to your repo via drag and drop won't resolve conflicts label May 4, 2021
@optimumtact
Copy link
Member

bot says no lol

@Mothblocks
Copy link
Member

et tu, brute?

@tgstation-server tgstation-server removed Merge Conflict Adding upstream files to your repo via drag and drop won't resolve conflicts Test Merge Candidate You're our unpaid test team labels May 4, 2021
@81Denton 81Denton added the Test Merge Candidate You're our unpaid test team label May 4, 2021
@rdragan
Copy link
Contributor

rdragan commented May 5, 2021

This seems to have broken at least teleporters, wormholes(from the wormhole event), wormhole jaunters and hand tele portals.

@ATH1909
Copy link
Contributor

ATH1909 commented May 5, 2021

This seems to have broken at least teleporters, wormholes(from the wormhole event), wormhole jaunters and hand tele portals.

we should probably check to see if it breaks monkey diving and mulebots as well.

@KathyRyals
Copy link
Contributor

Can confirm this broke portals.

@tgstation-server tgstation-server removed the Test Merge Candidate You're our unpaid test team label May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Uses the 32-bit address space and slow interpreter more effectively Refactor Makes the code harder to read
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants