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

block() syntax refactor for 515 #6265

Merged
merged 2 commits into from
May 11, 2024

Conversation

Doubleumc
Copy link
Contributor

@Doubleumc Doubleumc commented May 9, 2024

About the pull request

image
https://www.byond.com/docs/ref/#/proc/block

With the move to 515, updates the turf.dm defines to make use of the new block() syntax, removing unnecessary locate() calls. Removed the out-of-bounds guards as Lummox's post implies it is no longer necessary and there were no issues in testing.

Reviewed existing use of block() or the defines. Removed locate() anywhere it was no longer necessary due to the new syntax. Switched to the defines where helpful. Any for loop using the turf list was changed to var/turf/... as anything... if there would be no side effects.

Explain why it's good for the game

Making full use of new functionality. More consistent use of block(). Fewer calls to locate() and more use of as anything can't hurt.

Testing Photographs and Procedure

Boots without obvious issue. Checked Almayer and LV624, both look generated correctly. Shuttles launch and land correctly. Vehicles collide correctly.

Changelog

No player-facing changes.

@github-actions github-actions bot added the Missing Changelog Maintainers always document their changes. label May 9, 2024
Copy link
Contributor

@Zonespace27 Zonespace27 left a comment

Choose a reason for hiding this comment

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

Code in general seems alright but a short TM period would probably be nice since this touches a fair bit

@cm13-github cm13-github added the Merge Conflict PR can't be merged because it touched too much code label May 10, 2024
@cm13-github
Copy link
Contributor

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

@Drulikar Drulikar added 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) labels May 10, 2024
@Drulikar Drulikar marked this pull request as draft May 10, 2024 05:28
@cm13-github cm13-github removed the Merge Conflict PR can't be merged because it touched too much code label May 10, 2024
@cm13-github
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Drulikar Drulikar marked this pull request as ready for review May 10, 2024 05:30
cm13-github added a commit that referenced this pull request May 10, 2024
cm13-github added a commit that referenced this pull request May 10, 2024
cm13-github added a commit that referenced this pull request May 10, 2024
cm13-github added a commit that referenced this pull request May 10, 2024
@Drulikar Drulikar added this pull request to the merge queue May 11, 2024
@Drulikar Drulikar added Code Improvement Make the code longer and removed Needs Testing Need to test it on the guinea pigs (production server) labels May 11, 2024
Merged via the queue into cmss13-devs:master with commit d57a276 May 11, 2024
27 checks passed
@Doubleumc Doubleumc deleted the block-conversion branch May 11, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Improvement Make the code longer Missing Changelog Maintainers always document their changes. Testmerge Candidate we'll test this while you're asleep and the server has 10 players
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants