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

Canonical Movement #82085

Merged
merged 5 commits into from Mar 23, 2024
Merged

Conversation

Kapu1178
Copy link
Contributor

@Kapu1178 Kapu1178 commented Mar 18, 2024

About The Pull Request

  • Canonizes Movement DaedalusDock/daedalusdock#892
    Currently, TG's movement chain is not canonical, meaning movement can resolve in an order that doesn't actually represent what it going on. For example, if something inside of an Entered() call would move the currently moving object, it resolves in this order:
  1. Original Move
  2. Intercepted Move
  3. Intercepted Moved
  4. Original Moved

This makes Moved() unreliable at tracking things like spatial grid locations. This is a massive problem.

This PR introduces active_movement, a list containing arguments for Moved(). At the start of Move() and doMove(), if the list is present, it will call Moved(), concluding the original movement, before proceeding. The original Move() call will not end in Moved(), due to active_movement being null.

This is touching some of the most important code in the game and needs extensive testing.

@san7890 san7890 added Refactor Makes the code harder to read Unit Tests Make sure the stuff that's broken is still broken 📌 Test Merge Candidate This version will not be removed by actions when the PR is updated labels Mar 19, 2024
Copy link
Contributor

github-actions bot commented Mar 19, 2024

This pull request was test merged in 259 round(s).

Round list

sybil

terry

@san7890
Copy link
Member

san7890 commented Mar 19, 2024

people were complaining about a bug that would "lock people up" transiently for a few seconds on Sybil Round 225755- I pulled the test-merge and demanded they make an issue report regarding it (some maintainers were on, hopefully they can collect good data i hope)

@JohnFulpWillard
Copy link
Member

JohnFulpWillard commented Mar 19, 2024

It's been testmerged on sybil for 2 rounds and both times apparently movement just seemed to stop. Even observing I find myself unable to move, trying to move instead just types in my chat bar and tabbing doesn't swap to move mode. arrow keys also don't work and instead just moves where I'm typing.
Vid (me using WASD to move, trying to tab out, trying to click my screen, trying to use arrow keys):

8mb.video-8oL-9THp18BN.mp4

Runtimes that might be related (ID 225755):
image
image
image
image
image

Kyler also noticed errors about divisions by 0 in FOV, screenshot stolen from him on discord
image

@Kapu1178
Copy link
Contributor Author

I have changed the code such that active_movement is nulled before performing the Moved() call, as Kyler's theory is that it's caused by a Moved() subscriber causing another movement.

The mob_movement.dm runtimes Willard posted are unrelated, that's just shitcode. Check out the lines and you should immediately notice what the issue is with that.

@san7890
Copy link
Member

san7890 commented Mar 21, 2024

No complaints about this over the last two days of dual-server test merges, you think it's ready to ship?

@Kapu1178
Copy link
Contributor Author

No complaints about this over the last two days of dual-server test merges, you think it's ready to ship?

@Kylerace

@Kylerace Kylerace merged commit 848c4f9 into tgstation:master Mar 23, 2024
19 checks passed
github-actions bot added a commit that referenced this pull request Mar 23, 2024
This was referenced Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Makes the code harder to read 📌 Test Merge Candidate This version will not be removed by actions when the PR is updated Unit Tests Make sure the stuff that's broken is still broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants