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

Use RANGE_TURFS #6692

Merged
merged 2 commits into from
Aug 7, 2024
Merged

Conversation

Doubleumc
Copy link
Contributor

@Doubleumc Doubleumc commented Jul 12, 2024

About the pull request

Switches several uses of for(var/turf/... in range()) over to RANGE_TURFS() instead. Doesn't switch when the expected radius is less than 5. Where possible, removes the loop entirely.

Explain why it's good for the game

For finding turfs, RANGE_TURFS shows a slight performance benefit over range. This performance benefit becomes more pronounced as the radius increases.

Testing Photographs and Procedure

Screenshots & Videos

Performance testing. All procs in question return a list of turfs.
The procs of interest:
image

Radius 1:
image

Radius 5:
image

Radius 10:
image

Radius 20:
image

Results show that when able to convert away from a loop (turftest_range_turf_direct) that is always the best choice. For results under a radius of 5 the standard range (turftest_range) was notably faster, for a radius of 5 or more RANGE_TURFS (turftest_range_turf) was slightly faster.

Changelog

No player facing changes.

@github-actions github-actions bot added the Missing Changelog Maintainers always document their changes. label Jul 12, 2024
@Doubleumc Doubleumc changed the title Use RANGE_TURFS, ORANGE_TURFS Use RANGE_TURFS Jul 15, 2024
Copy link
Contributor

This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself

@github-actions github-actions bot added the Stale beg a maintainer to review your PR label Jul 23, 2024
@Drulikar Drulikar added Stale Exempt PR can't go stale Testmerge Candidate we'll test this while you're asleep and the server has 10 players Needs Testing Need to test it on the guinea pigs (production server) and removed Stale beg a maintainer to review your PR labels Jul 29, 2024
cm13-github added a commit that referenced this pull request Aug 1, 2024
cm13-github added a commit that referenced this pull request Aug 1, 2024
cm13-github added a commit that referenced this pull request Aug 2, 2024
cm13-github added a commit that referenced this pull request Aug 2, 2024
cm13-github added a commit that referenced this pull request Aug 3, 2024
cm13-github added a commit that referenced this pull request Aug 4, 2024
cm13-github added a commit that referenced this pull request Aug 4, 2024
cm13-github added a commit that referenced this pull request Aug 5, 2024
cm13-github added a commit that referenced this pull request Aug 5, 2024
Copy link
Contributor

@Drulikar Drulikar left a comment

Choose a reason for hiding this comment

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

As far as I can tell code is fine. It has been TM'd for a while without any issues seemingly. I also tested some of the more obscure things changed such as teleporter, bluespace tomatoes, and doors bordering space (or not) without issue.

@Drulikar Drulikar added this pull request to the merge queue Aug 7, 2024
Merged via the queue into cmss13-devs:master with commit d3ca38d Aug 7, 2024
29 checks passed
@Doubleumc Doubleumc deleted the use-RANGE_TURFS-ORANGE_TURFS branch August 7, 2024 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing Changelog Maintainers always document their changes. Needs Testing Need to test it on the guinea pigs (production server) Stale Exempt PR can't go stale Testmerge Candidate we'll test this while you're asleep and the server has 10 players
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants