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

fix scram, revert #26429 #31482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Ilya246
Copy link
Contributor

@Ilya246 Ilya246 commented Aug 26, 2024

About the PR

fixes #31480

why is this a revert? i believe that #26429 is a de-optimisation, since i presume it would be faster to check the 20 or so tiles the tries20 picks rather than the 31k or so tiles in range; i think tries20 is actually the efficient implementation for this

however, it's possible i am wrong regarding the above, so correct me if i am; in that case, i will make a PR that removes the range sqrt, as opposed to a revert

however, another issue is that when i tested a fix involving simply removing the sqrt, it tended to very often teleport you into space

additionally, tries20 will naturally prioritise larger grids near itself and be less likely to teleport you into space if you're far into the station

Why / Balance

this is a fix - irrelevant

Technical details

makes scram use tries20 to find a valid tile

Media

tested, works, consistently gives luckier (minimal TP into walls or into space) teleports than before even with raised range

Requirements

  • I have read and I am following the Pull Request Guidelines. I understand that not doing so may get my pr closed at maintainer’s discretion
  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Changelog
🆑

  • fix: Fixed the syndicate scram implanter incorrectly teleporting you only up to 10 tiles away. The range is now 100 tiles.

Copy link
Contributor

@whateverusername0 whateverusername0 left a comment

Choose a reason for hiding this comment

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

i approve of this

@Sadopeer
Copy link

Sadopeer commented Sep 9, 2024

This would be a much needed buff

Copy link
Contributor

@metalgearsloth metalgearsloth left a comment

Choose a reason for hiding this comment

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

Change the existing code to do what is preferred rather than leaving in poorly optimised code that teleports arbitrarily.

@metalgearsloth metalgearsloth added the Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. label Sep 10, 2024
@Ilya246 Ilya246 force-pushed the fixscram branch 2 times, most recently from 2e2f9d8 to d425dfb Compare September 10, 2024 07:23
@Ilya246
Copy link
Contributor Author

Ilya246 commented Sep 11, 2024

i believe i should mention this, but from testing, the tries20 method works 20 to 50 times faster than the one i am reverting

@github-actions github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Sep 12, 2024
Copy link
Contributor

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

@github-actions github-actions bot removed the Merge Conflict This PR currently has conflicts that need to be addressed. label Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scram implant's range is way lower than intended
4 participants