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

refactor: Movement cross/uncross implementation. #26762

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

warriorstar-orion
Copy link
Contributor

@warriorstar-orion warriorstar-orion commented Sep 12, 2024

What Does This PR Do

This PR refactors several aspects of movement code. It provides a replacement implementation for /atom/movable/Move which does not call the native implementation nor the various /Cross//Uncross native procs, instead relegating this behavior to signal handlers. It introduces the connect_loc component, which handles crossing-style signals for objects by reregistering the specified signals onto locations that the parent moves to, and connect_loc_behalf, which does the same thing as connect_loc except on behalf of another listener.

This PR is an amalgamation of several /tg/ PRs, including but not limited to:

Why It's Good For The Game

This implementation clears the way for #26637, another important refactor.

Many native BYOND procs are called millions of times a round for no reason. This implementation shortcircuits millions of these calls.

2024_09_05__15_11_20__Paradise Profile Viewer

2024_09_05__15_11_27__Paradise Profile Viewer

Overall this is a much cleaner implementation and easier to reason about, surprisingly.

Testing

In progorororereressss.

  • Test crossing terror spider webs.
  • Test bumps into doors/airlocks.
  • Test bumps into gravgen tiles.
  • Test crossover/crossing organics and exposed pulse demons.
  • Test crossing over mice for squeak noise.
  • Test crossing over peels.
  • Test passing flipped tables in correct directions.
  • Test projectiles passing shieldwalls and flipped tables.
  • Test mobs passing under tesla balls.
  • Test treadmill.
  • Test assemblies.
  • Test walking barefoot on syringes.
  • Test meteor gun.
  • Test necropolis doors.
  • Test bloody decals.
  • Test flamethrowers.
  • Test beartraps.
  • Test crossing over bluespace lockers.
  • Test railings.
  • Test teleport hubs.
  • Test border firedoors.
  • Test bonfires.
  • Test acid.
  • Test windoors.
  • Test mines.
  • Test snap-pops.
  • Test tar.
  • Test proximity sensors in assemblies, as well as assemblies placed in crates.
  • Test proximity detector for energy projectiles shot near a singulo.

Declaration

  • I confirm that I either do not require pre-approval for this PR, or I have obtained such approval and have included a screenshot to demonstrate this below.

Changelog

NPFC

@ParadiseSS13-Bot ParadiseSS13-Bot added the -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally label Sep 12, 2024
@BiancaWilkson BiancaWilkson added the Refactor This PR will clean up the code but have the same ingame outcome label Sep 13, 2024
Copy link
Contributor

@lewcc lewcc left a comment

Choose a reason for hiding this comment

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

The performance implications of this look sick. I gave some of it a quick look, but there is a LOT. I'll probably try to come back to this down the line.

The one thing that strikes me is that this does feel much less intuitive than simply overriding Crossed() as before. Do you think you could create some kind of developer documentation for this change that explains the difference between the old and new way of doing this? Maybe on the new documentation site.
I think it'd be worth listing out things like

  • which procs are now off limits (partially or entirely)
  • which signals should be used to replace them (possibly with their arguments), and in particular, how to interact with them, especially with the return types.
  • A good example or two of how you might now use it, maybe with a comparison of the old way and the new way in order to show how the concepts have transferred.

The more I look at the changes, the more straightforward it feels, but I also say this as someone who has a reasonable understanding of our ECS. I also really appreciate that you've put a lot into this work and don't want to put too much more onto your plate, but I think having this would do dividends.

code/__DEFINES/is_helpers.dm Show resolved Hide resolved
@@ -0,0 +1,21 @@
#define ACTIVE_MOVEMENT_OLDLOC 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Document these, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, don't need these in this PR. Removed.

@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Sep 14, 2024
@warriorstar-orion
Copy link
Contributor Author

The performance implications of this look sick. I gave some of it a quick look, but there is a LOT. I'll probably try to come back to this down the line.

The one thing that strikes me is that this does feel much less intuitive than simply overriding Crossed() as before. Do you think you could create some kind of developer documentation for this change that explains the difference between the old and new way of doing this? Maybe on the new documentation site. I think it'd be worth listing out things like

  • which procs are now off limits (partially or entirely)
  • which signals should be used to replace them (possibly with their arguments), and in particular, how to interact with them, especially with the return types.
  • A good example or two of how you might now use it, maybe with a comparison of the old way and the new way in order to show how the concepts have transferred.

The more I look at the changes, the more straightforward it feels, but I also say this as someone who has a reasonable understanding of our ECS. I also really appreciate that you've put a lot into this work and don't want to put too much more onto your plate, but I think having this would do dividends.

I've added a new page of documentation on the relevant signals and what to use when. PTAL and let me know it it needs any improvements.

@ParadiseSS13-Bot ParadiseSS13-Bot added the Testmerge Requested This PR has a pending testmerge request label Sep 15, 2024
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Sep 15, 2024
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting review This PR is awaiting review from the review team and removed -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally labels Sep 17, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Sep 18, 2024
@github-actions github-actions bot added Merge Conflict This PR is merge conflicted and removed Merge Conflict This PR is merge conflicted labels Sep 19, 2024
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Sep 19, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Sep 28, 2024
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Oct 1, 2024
Tested with sensors in crates, sensors in modsuits
Tested new proximity component with firing projectiles at singularity
Tested new proximity component with portable flashes
Tested new proximity component with facehuggers
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Oct 9, 2024
@@ -11,11 +11,19 @@

var/scanning = FALSE
var/timing = FALSE
var/time = 10
COOLDOWN_DECLARE(timing_cd)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't change its behavior, right? Was this original variable in seconds, ds, or processing ticks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did some extra testing. As far as I can tell, the original sensor cooldown is 10 ticks. I'm almost positive I ported the behavior correctly, both for the sensor cooldown and the arming timer. There's also "cooldown", which is not related to the timing countdown, meant to prevent spamming the sensor, which is part of the parent proc /obj/item/assembly/proc/process_cooldown(), and was already on a timer so the behavior there shouldn't have changed either.

@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Oct 10, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Oct 15, 2024
@github-actions github-actions bot added Merge Conflict This PR is merge conflicted and removed Merge Conflict This PR is merge conflicted labels Oct 15, 2024
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Status: Awaiting review This PR is awaiting review from the review team Refactor This PR will clean up the code but have the same ingame outcome Testmerge Requested This PR has a pending testmerge request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants