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

Clumsy system refactor #31147

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

Conversation

beck-thompson
Copy link
Contributor

@beck-thompson beck-thompson commented Aug 18, 2024

About the PR

The clumsy system was just hard coded into a lot of places. Now the clumsy system uses events that it can interact with instead!

Technical details

As above, I mostly just made events where there weren't before and made the clumsy system hook into them and listen and interact with them. Specifically, the gun system, defib system, hypospray system, and climb on thing system.

Media

2024-08-17.20-46-35.mp4
  • All chances are 50/50 I just got unlucky.

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

Breaking changes

If you hard coded clumsy stuff into important systems please trigger events and listen for them in the clumsy system now! You also might need to change some of the clumsy fields as well.

Changelog

🆑 Beck Thompson

  • tweak: Minor tweaks to clumsiness. Some of the timings and or noises have been changed slightly!

@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Aug 18, 2024
@beck-thompson beck-thompson changed the title First commit Clumsy system refactor Aug 18, 2024
@beck-thompson
Copy link
Contributor Author

OK looks like I forgot that tables made a special sound 😭
I'll fix that tomorrow because its gonna require a few annoying changes...

Content.Shared/Clumsy/ClumsyComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsySystem.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsySystem.cs Outdated Show resolved Hide resolved
Content.Shared/Climbing/Events/BeforeClimbEvents.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsySystem.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsySystem.cs Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Sep 8, 2024

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

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Sep 8, 2024
@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Sep 10, 2024
@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Sep 26, 2024
@beck-thompson
Copy link
Contributor Author

OK think I fixed all that! Let me know if you need anything else or if I missed something 🫡

Content.Shared/Chemistry/Events/HyposprayEvents.cs Outdated Show resolved Hide resolved
Content.Shared/Inventory/InventorySystem.Relay.cs Outdated Show resolved Hide resolved
if (!_random.Prob(ent.Comp.ClumsyDefaultCheck))
return;

_stun.TryParalyze(ent, ent.Comp.GunShootFailStunTime, true);
Copy link
Member

Choose a reason for hiding this comment

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

Where did TryChangeDamage go?
Also before it had a separate roll for each ammo used, now you only got one. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah both of those are intended!

1.) I didn't think it made much sense that the "explosion" did actual damage. The bullet is never actually fired so I don't really know what would be actually damaging the clown. The noise has the same issue but its funny and seems like something that a clown would do so I kept that! Balance wise the damage was low enough that I felt It wasn't necessary.

2.) This was now a couple months ago so I could be wrong, but I literally could not figure out what that did and every time tested it, there would only ever be one ammo type. Maybe it was for shotguns or something? I'll do some more testing tonight.

Copy link
Member

Choose a reason for hiding this comment

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

I always interpreted it as the clown shooting himself by accident and never noticed there was no ammo being used.
Still not sure if that means we should remove the damage though, that is kinda part of the balance for monkeys and holoclowns. And 12 damage per failed shot is quite a lot.

Copy link
Member

Choose a reason for hiding this comment

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

For the second part the ammo count seems to be used for semi auto or burst shots, where multiple shots are done at once. If there is only one clumsy roll there you might be able to shoot the whole magazine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always interpreted it as the clown shooting himself by accident and never noticed there was no ammo being used.
Still not sure if that means we should remove the damage though, that is kinda part of the balance for monkeys and holoclowns. And 12 damage per failed shot is quite a lot.

I honestly don't know about balance much, I personally would prefer to leave it without the damage but if you think that I should add it back just let me know!

For the ammo stuff, I'll work on that some more, I also want to make sure it works correctly with #31292!

Content.Shared/Medical/DefibrillatorEvents.cs Outdated Show resolved Hide resolved
Content.Shared/Medical/DefibrillatorEvents.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsyComponent.cs Show resolved Hide resolved
Content.Shared/Clumsy/ClumsySystem.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsyComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsyComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsyComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsyComponent.cs Outdated Show resolved Hide resolved
else
// Someone else slamed you onto the table.
_popup.PopupPredicted(Loc.GetString("bonkable-success-message-others", ("bonker", puttingOnTableName), ("victim", gettingPutOnTableName), ("bonkable", beingClimbedName)), ent, ent);
_popup.PopupPredicted(Loc.GetString("bonkable-success-message-others", ("bonker", Name(args.PuttingOnTable)), ("victim", Name(args.GettingPutOnTable)), ("bonkable", args.BeingClimbedOn)), ent, ent);
Copy link
Member

Choose a reason for hiding this comment

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

Wait, now you are ignoring identity again.
bonker and victim need it since they are players and can have their identity hidden.
bonkable does not need it since a table has no identity, only a name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm like 70% sure this is correct now... I tested a lot and it all seems to make sense?

@beck-thompson
Copy link
Contributor Author

I also tested this with the new burst mechanics and everything seems to work as expected!

Comment on lines +94 to +95
_audio.PlayPredicted(ent.Comp.ClumsySound, ent, ent);

Copy link
Member

Choose a reason for hiding this comment

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

I had a look at this again, and before it only played one of these two sounds. The admin smite is crazy loud right now due to that. How it should work:

  • The table bonk sound should be part of BonkableComponent, like it was before
  • Move playing the sound into HitHeadClumsy
  • play the default sound only if not overriden by the bonkablesound

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh forgot to reply here! I couldn't quite do it like that for various reasons but it now acts the same as it did before (Only the bonk noise, no clown horn).

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Nov 13, 2024
Copy link
Contributor

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

@SlamBamActionman SlamBamActionman added the S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. label Nov 14, 2024
@github-actions github-actions bot added size/L Denotes a PR that changes 100-1000 lines. and removed S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted labels Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Awaiting Changes Status: Changes are required before another review can happen S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/L Denotes a PR that changes 100-1000 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants