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.

Sign up for GitHub

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

Jump to bottom

Converting many grab attackby() blocks into grab_attack(). #4466

Merged
merged 6 commits into from
Sep 24, 2024
Merged

Converting many grab attackby() blocks into grab_attack(). #4466

merged 6 commits into from
Sep 24, 2024

Conversation

MistakeNot4892
Copy link
Contributor

Description of changes

Breaks various blocks out of attackby() into grab_attack().

Why and what will this PR improve

Allows grab attack overrides to function properly without wonky proc flow like structure base grab interactions talking precedence over attackby().

Authorship

Myself.

Changelog

Nothing player-facing.

@MistakeNot4892 MistakeNot4892 added the work in progress This PR is under development and shouldn't be merged. label Sep 16, 2024
@MistakeNot4892 MistakeNot4892 added the has dependencies This PR should not be merged prior to any PRs linked in body or comments. label Sep 17, 2024
@MistakeNot4892
Copy link
Contributor Author

Depends on #4462 railing changes.

@MistakeNot4892 MistakeNot4892 force-pushed the tweak/grabattack branch 2 times, most recently from 1dd93c5 to 7915fc8 Compare September 21, 2024 01:50
@MistakeNot4892 MistakeNot4892 added ready for review This PR is ready for review and merge. and removed has dependencies This PR should not be merged prior to any PRs linked in body or comments. work in progress This PR is under development and shouldn't be merged. labels Sep 21, 2024
@MistakeNot4892
Copy link
Contributor Author

All appears to be working.

out-of-phaze
out-of-phaze previously approved these changes Sep 21, 2024
View reviewed changes
Copy link
Member

@out-of-phaze out-of-phaze left a comment

Choose a reason for hiding this comment

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

a little worried about the reversed arg order (user then item instead of item then user) causing issues down the line/when migrating legacy code, but it'll probably be fine. it's something typemaker will hopefully be able to detect eventually, at least

@MistakeNot4892
Copy link
Contributor Author

a little worried about the reversed arg order (user then item instead of item then user) causing issues down the line/when migrating legacy code, but it'll probably be fine. it's something typemaker will hopefully be able to detect eventually, at least

Easy enough to tweak it.

out-of-phaze
out-of-phaze previously approved these changes Sep 22, 2024
View reviewed changes
@out-of-phaze out-of-phaze merged commit 8a5ff01 into NebulaSS13:dev Sep 24, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@out-of-phaze out-of-phaze out-of-phaze approved these changes

Assignees
No one assigned
Labels
ready for review This PR is ready for review and merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MistakeNot4892 @out-of-phaze