Skip to content

Grenade GC improvement #19355

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

Merged

Conversation

GDNgit
Copy link
Contributor

@GDNgit GDNgit commented Oct 10, 2022

What Does This PR Do

Improves the GCing on grenades

Why It's Good For The Game

They use walk(), walk is not reset, ref is kept, the grenades will always fail to GC due to this. GCing is good.
This needs to be done for a boatload of other items as well

Testing

compiled, grenades GCed properly.

Changelog

NPFC

@ParadiseSS13-Bot ParadiseSS13-Bot added the -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally label Oct 10, 2022
@hal9000PR hal9000PR added the GC Related This PR optimises how a type GCs label Oct 11, 2022
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting review This PR is awaiting review from the review team and removed -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally labels Oct 11, 2022
@farie82
Copy link
Contributor

farie82 commented Oct 15, 2022

Can you show that this actually fixes a GC issue? I'm thinking that walk(src, null, null) works the same as walk(src, 0) due to null and 0 being both falsey

@GDNgit
Copy link
Contributor Author

GDNgit commented Oct 15, 2022

Can you show that this actually fixes a GC issue? I'm thinking that walk(src, null, null) works the same as walk(src, 0) due to null and 0 being both falsey

Mirroring from discord:
Grenades use walk_away in a few instances, but most notably in clusterbombs. As those clusterbombs use walk away they will fail to GC, and so will the grenades spawned from the clusterbomb segments.

Copy link
Contributor

@farie82 farie82 left a comment

Choose a reason for hiding this comment

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

LGTM besides the small styling thing

Co-authored-by: Farie82 <[email protected]>
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting merge This PR is ready for merge and removed -Status: Awaiting review This PR is awaiting review from the review team labels Oct 16, 2022
@farie82 farie82 merged commit f4a55d8 into ParadiseSS13:master Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Status: Awaiting merge This PR is ready for merge GC Related This PR optimises how a type GCs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants