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

Add attack delay to weapon throwing #34903

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

SlamBamActionman
Copy link
Member

About the PR

This PR restricts melee throwing weapons after using them to melee attack, requiring the user to wait out the melee cooldown before being able to throw.

Unarmed attacks are exempted from this.

Why / Balance

Doing a melee swing and then throwing is a technique most commonly seen used in conjunction with stunbatons, as it reduces the normal 2 second stamcrit time to just 1 second (hit > wait > hit > throw). A 1 second stamcrit is of course too short, so this PR fixes that.

It was kinda annoying to not be able to throw immediately upon missing an unarmed attack, since you might've been trying to pick up the item and misclicked, so it makes an exception for that.

Technical details

At first I just checked the melee swing timer but that ends up being set when you pick up an item, so it added an annoying delay to picking things up from the ground and immediately throwing, which can be important for transporting stuff.

So instead we're subscribing to the MeleeAttackEvent and that sets the throw timer instead.

Media

Requirements

Breaking changes

Changelog

🆑

  • fix: Fixed being able to throw melee weapons immediately after attacking with them.

@github-actions github-actions bot added size/S Denotes a PR that changes 10-99 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 5, 2025
@slarticodefast
Copy link
Member

slarticodefast commented Feb 6, 2025

Code looks good and works as intended.
A problem with this I can see is the lack of communication to the player, which makes it feel like an unintended bug. You press the button and sometimes nothing happens, depending on your exact timing for the throw (and I imagine on a live server on your ping as well, due to throwing not being predicted), especially since this new cooldown is very short.

@ArtisticRoomba ArtisticRoomba added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. T: New Feature Type: New feature or content, or extending existing content S: Needs Review Status: Requires additional reviews before being fully accepted T: Balance Change Type: Balance changes through direct value changes, or changes to mechanics that affect it DB: Beginner Friendly Difficulty: Great for beginners. Unambiguous in scope, and explains how to achieve the result. A: Combat Area: Combat features and changes, balancing, feel and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 8, 2025
@ArtisticRoomba
Copy link
Contributor

A problem with this I can see is the lack of communication to the player, which makes it feel like an unintended bug. You press the button and sometimes nothing happens, depending on your exact timing for the throw (and I imagine on a live server on your ping as well, due to throwing not being predicted), especially since this new cooldown is very short.

I'm sure a simple "You can't throw this item yet!" or something along those lines would tell the player that the block is intended.

@slarticodefast
Copy link
Member

Having the melee weapon cooldown show up as a doafter circle in the hand slot would work as well, but I think the cooldown is currently using its own implementation for that.

@SlamBamActionman
Copy link
Member Author

It's worth noting there are fairly few weapons where you'd consider throwing after hitting, so any overarching attack cooldown changes are probably not necessary.

I will look into improving the UX though!

@FairlySadPanda
Copy link
Contributor

The do-after is representing the amount of time it really takes to complete an attack. So in-universe the reason you can't throw the weapon should be because you're still mid-swing.

Realistically a throw is also an attack, so it makes sense without additional UX, IMO.

Copy link
Member

@Tayrtahn Tayrtahn left a comment

Choose a reason for hiding this comment

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

Code looks good; change makes sense.

UX improvement would be nice. Could be added here or separately.

// Give it a l'il spin.
// Give it a li'l spin.
Copy link
Member

Choose a reason for hiding this comment

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

Absolutely vital change! 😄

@SlamBamActionman
Copy link
Member Author

I have looked into this further and there are some minor not-so-good feeling stuff with it. E.g. if you swing a crowbar there is a 1 second window before you can throw it, which just doesn't feel good. I tried a pop-up but that honestly didn't seem to solve anything. It's a crowbar, why shouldn't I be able to just throw it?

So I am pivoting with the code. Instead of having a general OnMeleeAttack event in the handcode, I am moving it to any component that should block throwing. This seems to be DamageOtherOnHit and StaminaDamageOnCollide. Still testing it but theoretically it would mean only objects where throwing after hitting matters have it blocked.

Then I'll add a general You are not ready to throw! pop-up to a failed throw caused by the throw cooldown, which will pop up either after a relevant melee swing, or if you try to spam the throw button (i.e. the normal throw cooldown).

@SlamBamActionman
Copy link
Member Author

SlamBamActionman commented Feb 8, 2025

It was a wee bit trickier than I would've liked (why is DamageOnCollide a server-only component?) but I settled on a solution that should be future-proof; ResetThrowCooldown can be used by other systems to reset/set a new throw cooldown, and the relevant systems do that now. Also, pop-up!

Note that I also fixed an imperceptible bug where disarm attempts would fail if the target was experiencing a throw cooldown. Since weapon swings now cause a throw cooldown, this was necessary to change - disarms simply bypass the throw timer check.

Copy link
Member

@Tayrtahn Tayrtahn left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -213,4 +216,23 @@ private void HandleExamined(EntityUid examinedUid, HandsComponent handsComp, Exa
args.PushMarkup(Loc.GetString(locKey, locUser, locItems));
}
}

/// <summary>
/// Resets the throw cooldown to allow for throwing, and optionally sets a new throw tim if given.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Resets the throw cooldown to allow for throwing, and optionally sets a new throw tim if given.
/// Resets the throw cooldown to allow for throwing, and optionally sets a new throw time if given.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Combat Area: Combat features and changes, balancing, feel DB: Beginner Friendly Difficulty: Great for beginners. Unambiguous in scope, and explains how to achieve the result. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Needs Review Status: Requires additional reviews before being fully accepted size/S Denotes a PR that changes 10-99 lines. T: Balance Change Type: Balance changes through direct value changes, or changes to mechanics that affect it T: New Feature Type: New feature or content, or extending existing content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants