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

Mirror: StrippableSystem doafter overhaul #205

Conversation

SimpleStation14
Copy link
Member

Mirror of PR #25994: StrippableSystem doafter overhaul from space-wizards space-wizards/space-station-14

41ca8f3dfcb986432e1e509247bf239cac137836

PR opened by Krunklehorn at 2024-03-11 12:36:28 UTC


PR changed 7 files with 465 additions and 305 deletions.

The PR had the following labels:

  • Status: Needs Review

Original Body

About the PR

Refactors Strippable DoAfter events to make them synchronous and organized.

Technical details

Strippable System & Component

  • Synchronous DoAfters
  • Made use of TimeSpan, GetStripTimeModifiers() and ByRefEvent
  • Reorganized checks, removed some redundant ones
  • Resolve pattern where useful
  • Added more asserts
  • Lots of cleanup

The DoAfters were grouped under one event to avoid copy-pasting eight separate cancel checks, asserts and function signatures.

Let me know if this is bad for performance and I'll roll them out instead.

Media

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

TimeSpans

ThievingComponent, InventoryTemplatePrototype and ToggleableClothingSystem use TimeSpan in places where they intersect with StrippableComponent.

Changelog

N/A

@SimpleStation14 SimpleStation14 added the Pull Request Mirror Mirrors a PR from another Repo. Automatically applied by mirror bot label Apr 22, 2024
Copy link
Contributor

@DangerRevolution DangerRevolution left a comment

Choose a reason for hiding this comment

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

needs to be checked with Felinid changes (not sure if merged / reverted here)

@SimpleStation14 SimpleStation14 marked this pull request as draft May 4, 2024 21:13
@github-actions github-actions bot added the Status: Merge Conflict FIX YOUR PR AAAGH label May 4, 2024
Copy link
Contributor

github-actions bot commented May 4, 2024

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

@VMSolidus VMSolidus marked this pull request as ready for review May 6, 2024 19:12
@github-actions github-actions bot added Changes: C# Changes any cs files and removed Status: Merge Conflict FIX YOUR PR AAAGH labels May 29, 2024
@github-actions github-actions bot added the Status: Needs Review Someone please review this label Jun 14, 2024
@DangerRevolution DangerRevolution added Size: 3-Medium For medium issues/PRs Priority: 3-Medium Needs to be resolved at some point Type: Cleanup Like Rework but small labels Jun 14, 2024
Copy link
Member

@VMSolidus VMSolidus left a comment

Choose a reason for hiding this comment

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

Going to approve this since we have both the fix PR, as well as a new PR reworking the thieving system to go with it.

@VMSolidus VMSolidus merged commit 89a6bb3 into Simple-Station:master Jul 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Priority: 3-Medium Needs to be resolved at some point Pull Request Mirror Mirrors a PR from another Repo. Automatically applied by mirror bot Size: 3-Medium For medium issues/PRs Status: Needs Review Someone please review this Type: Cleanup Like Rework but small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants