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

Cluster grenade refactor and contra markings #31108

Merged
merged 7 commits into from
Dec 16, 2024
Merged

Cluster grenade refactor and contra markings #31108

merged 7 commits into from
Dec 16, 2024

Conversation

Plykiya
Copy link
Contributor

@Plykiya Plykiya commented Aug 16, 2024 •
edited
Loading

About the PR

Cluster grenades essentially have two categories.

  • Ones that shoot projectiles (projectile grenades)
  • Ones that scatter entities that may potentially trigger themselves (scattering grenades)

I split it into two distinct systems to better separate the logic and fixed a few bugs.

Why / Balance

Technical details

Clustergrenades only trigger as a result of their on-use timer. This means that when they were destroyed, nothing happened. I gave both new categories of grenades a damage trigger so that they can be properly chained like regular explosives.

More than half of the variables in the system were unused by projectile-type grenades, so separating the two into two different systems enables easier alteration of logic or behavior.

None of the actual behavior regarding how they function has changed, it's just been separated.

  • Scattering grenades and projectile grenades now have an abstract parent to make contra marking sane
  • Clusterbang, cluster banana peel, slipocalypse clustersoap, and foam dart grenade migrated to Scattering Grenades and proper contra marking
  • Stingers, incendiaries, and shrapnel grenades migrated to Projectile Grenades and proper contra marking

Media

2024-05-30.21-50-33.mp4
2024-05-30.21-51-08.mp4
2024-06-19.12-25-36.mp4

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

ClusterGrenadeComponent/System were removed, replaced by ProjectileGrenadeComponent/System and ScatteringGrenadeComponent/System.

Changelog

🆑

  • fix: Bombs like stingers and clustergrenades now trigger when destroyed due to damage.

@Plykiya
Copy link
Contributor Author

Plykiya commented Aug 16, 2024 •
edited
Loading

this is currently possible on live:

Base.Profile.2024.07.25.-.12.10.49.119.1.mp4

@Plykiya Plykiya changed the title Cluster grenade refactor Cluster grenade refactor and contra markings Aug 17, 2024
@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Oct 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 3, 2024

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

@Everturning
Copy link

I think the stinger hit the nukies boss

@Plykiya
Merge branch 'refs/heads/master' into ClusterGrenadeRefactoring
0a92af2 
# Conflicts:
#	Resources/Prototypes/Entities/Objects/Weapons/Throwable/clusterbang.yml
@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Oct 9, 2024
@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 the size/XL Denotes a PR that changes 1000+ lines. label Nov 19, 2024
@eoineoineoin eoineoineoin added T: Bugfix Type: Bugs and/or bugfixes P3: Standard Priority: Default priority for repository items. T: Refactor Type: Refactor of notable amount of codebase D2: Medium Difficulty: A good amount of codebase knowledge required. S: Needs Review Status: Requires additional reviews before being fully accepted 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 Nov 20, 2024
Aidenkrz
Aidenkrz approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@Aidenkrz Aidenkrz left a comment

Choose a reason for hiding this comment

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

For the love of god the original PR for this was almost 8 months ago, this bug has existed for a millennia at this post and nobody has reviewed this.

@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 3, 2024 •
edited
Loading

900 stingers in a single stinger grenade is probably capable of crashing a server

https://cdn.discordapp.com/attachments/1295923728793866332/1301611809282719824/stinger_grenade.mp4?ex=67509d76&is=674f4bf6&hm=2a6c24eab300c7d9c6287c667a6924fb059d199c2cceb569efdbe4b6b38fc5eb&

chromiumboy
chromiumboy requested changes Dec 7, 2024
View reviewed changes
Copy link
Contributor

@chromiumboy chromiumboy left a comment

Choose a reason for hiding this comment

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

Thanks for this! Just a few things from me

Also, during my tests I noted that when projectile grenades, like stingers, are triggered it causes a whole bunch of errors because the meta data component on the grenade can't be resolved. This is likely because it got deleted too early. So that needs to be addressed as well

Content.Server/Explosion/EntitySystems/ProjectileGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Server/Explosion/EntitySystems/ScatteringGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Server/Explosion/EntitySystems/ScatteringGrenadeSystem.cs Outdated Show resolved Hide resolved
@chromiumboy chromiumboy added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Dec 7, 2024
@chromiumboy chromiumboy self-assigned this Dec 7, 2024
@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 7, 2024

Thanks for this! Just a few things from me

Also, during my tests I noted that when projectile grenades, like stingers, are triggered it causes a whole bunch of errors because the meta data component on the grenade can't be resolved. This is likely because it got deleted too early. So that needs to be addressed as well

because the bullet considers the grenade shooting it the "gun" and the grenade gets deleted so there is technically no gun, yeah

@chromiumboy
Copy link
Contributor

Thanks for this! Just a few things from me
Also, during my tests I noted that when projectile grenades, like stingers, are triggered it causes a whole bunch of errors because the meta data component on the grenade can't be resolved. This is likely because it got deleted too early. So that needs to be addressed as well

because the bullet considers the grenade shooting it the "gun" and the grenade gets deleted so there is technically no gun, yeah

Are you able to fix it? All those errors cause noticeable lag

@slarticodefast
Copy link
Member

I'm pretty sure this is the same as this bug already happening on master
#32642

@chromiumboy
Copy link
Contributor

I'm pretty sure this is the same as this bug already happening on master #32642

Yeah, it's the same issue

@Plykiya Plykiya requested a review from DrSmugleaf as a code owner December 8, 2024 06:03
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Dec 8, 2024
@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 8, 2024

2024-12-07.22-27-16.mp4

@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 8, 2024

looks pretty good

@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 8, 2024

2024-12-07.23-01-31.mp4

chromiumboy
chromiumboy requested changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@chromiumboy chromiumboy left a comment

Choose a reason for hiding this comment

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

Just a few more things

Content.Shared/Explosion/EntitySystems/SharedScatteringGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Server/Explosion/EntitySystems/ScatteringGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Server/Explosion/EntitySystems/ScatteringGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Server/Explosion/EntitySystems/ScatteringGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Server/Explosion/EntitySystems/ScatteringGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Server/Explosion/EntitySystems/ProjectileGrenadeSystem.cs Outdated Show resolved Hide resolved
Content.Shared/Explosion/Components/ScatteringGrenadeComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Explosion/Components/ScatteringGrenadeComponent.cs Outdated Show resolved Hide resolved
@chromiumboy chromiumboy added the S: Awaiting Changes Status: Changes are required before another review can happen label Dec 12, 2024
@chromiumboy
Copy link
Contributor

It doesn't look like the changes to RequireProjectileTargetSystem.cs have stopped the errors when projectile grenades detonate, unfortunately. I had a go at fixing it, but I couldn't make any progress either

@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 12, 2024 •
edited
Loading

It doesn't look like the changes to RequireProjectileTargetSystem.cs have stopped the errors when projectile grenades detonate, unfortunately. I had a go at fixing it, but I couldn't make any progress either

it stopped the client-side errors that normally caused me lag/game freezing at the very least but I never solved the gun code part that throws errors on the server

@Plykiya Plykiya requested a review from chromiumboy December 14, 2024 22:45
@github-actions github-actions bot removed the S: Awaiting Changes Status: Changes are required before another review can happen label Dec 14, 2024
chromiumboy
chromiumboy approved these changes Dec 15, 2024
View reviewed changes
Copy link
Contributor

@chromiumboy chromiumboy left a comment

Choose a reason for hiding this comment

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

Looks good :)

@chromiumboy chromiumboy added the S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. label Dec 15, 2024
@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 15, 2024 •
edited
Loading

to anyone who wants to take on the error being thrown on Content.Server in the future

SharedGunSystem's ShootProjectile() sets the shooter to either the gun's EntityUid, or user EntityUid? using Projectiles.SetShooter(). This will get set to our projectile grenade's EntityUid since we don't know who the user is usually. The projectile grenade is deleted once triggered.

ProjectileComponent has an EntityUid? Shooter field that is AutoNetworked. This field is set to the now deleted projectile grenade's EntityUid, and that will cause the engine's attempt to automatically network the EntityUid to fail since it can't load the MetadataComponent from the now deleted EntityUid

The reason that ShootProjectile() sets the user is so that the spawned projectile doesn't end up hitting the user who shoots the gun. This shouldn't really be relevant in the case of a grenade, since the grenade deletes itself...

I've attempted to make gun code check for whether the grenade is TerminatingOrDeleted, but the check doesn't really seem to figure out that the grenade is being deleted since the check fails, even if you set QueueDel(grenade) before you start shooting the projectiles
image

@Plykiya Plykiya mentioned this pull request Dec 16, 2024
Closed
2 tasks
@SlamBamActionman SlamBamActionman merged commit a4d6f09 into space-wizards:master Dec 16, 2024
13 checks passed
@Avalon-Proto
Copy link

I got a question, I've been attempting to make a grenade launcher that fires timed grenades in another server. Think this will help me out?

@Plykiya
Copy link
Contributor Author

Plykiya commented Dec 16, 2024 •
edited
Loading

I got a question, I've been attempting to make a grenade launcher that fires timed grenades in another server. Think this will help me out?

that's exactly why I made this change, so I could make the china-lake fire different types of ammo like that lol

@Plykiya Plykiya deleted the ClusterGrenadeRefactoring branch December 16, 2024 19:04
@Avalon-Proto
Copy link

I got a question, I've been attempting to make a grenade launcher that fires timed grenades in another server. Think this will help me out?

that's exactly why I made this change, so I could make the china-lake fire different types of ammo like that lol

THANK YOU. I can now give Delta's secborgs teargas and stingers instead of a pack injector

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chromiumboy chromiumboy chromiumboy approved these changes

@Aidenkrz Aidenkrz Aidenkrz approved these changes

@DrSmugleaf DrSmugleaf Awaiting requested review from DrSmugleaf DrSmugleaf is a code owner

Assignees

@chromiumboy chromiumboy

Labels
A: Combat Area: Combat features and changes, balancing, feel D2: Medium Difficulty: A good amount of codebase knowledge required. P3: Standard Priority: Default priority for repository items. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Needs Review Status: Requires additional reviews before being fully accepted size/XL Denotes a PR that changes 1000+ lines. T: Bugfix Type: Bugs and/or bugfixes T: Refactor Type: Refactor of notable amount of codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clustergrenade can get destroyed before splitting and not explode properly
8 participants
@Plykiya @Everturning @chromiumboy @slarticodefast @Avalon-Proto @Aidenkrz @eoineoineoin @SlamBamActionman