From 0c5fdaa57be0b8c2fec282f71b40a44295b3adab Mon Sep 17 00:00:00 2001
From: Luc <89928798+lewcc@users.noreply.github.com>
Date: Wed, 24 Apr 2024 15:40:45 -0400
Subject: [PATCH] Updates contributing guide regarding signals, removes
SIGNAL_HANDLER_DOES_SLEEP (#24824)
* Updates contributing guide, removes DOES_SLEEP
* Update .github/CONTRIBUTING.md
Co-authored-by: DGamerL <108773801+DGamerL@users.noreply.github.com>
---------
Co-authored-by: DGamerL <108773801+DGamerL@users.noreply.github.com>
---
.github/CONTRIBUTING.md | 30 ++++++++++++++++++++++++++--
code/__DEFINES/dcs/dcs_helpers.dm | 4 ----
code/modules/surgery/organs/heart.dm | 13 ++++++------
3 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md
index f22c1134b10..ae777746daf 100644
--- a/.github/CONTRIBUTING.md
+++ b/.github/CONTRIBUTING.md
@@ -157,7 +157,7 @@ if(thing == TRUE)
return "bleh"
var/other_thing = pick(TRUE, FALSE)
if(other_thing == FALSE)
- return "meh"
+ return "meh"
// Good
var/thing = pick(TRUE, FALSE)
@@ -165,7 +165,7 @@ if(thing)
return "bleh"
var/other_thing = pick(TRUE, FALSE)
if(!other_thing)
- return "meh"
+ return "meh"
```
### Use `pick(x, y, z)`, not `pick(list(x, y, z))`
@@ -452,6 +452,32 @@ Look for code examples on how to properly use it.
addtimer(CALLBACK(target, PROC_REF(dothing), arg1, arg2, arg3), 5 SECONDS)
```
+### Signals
+
+Signals are a slightly more advanced topic, but are often useful for attaching external behavior to objects that should be triggered when a specific event occurs.
+
+When defining procs that should be called by signals, you must include `SIGNAL_HANDLER` after the proc header. This ensures that no sleeping code can be called from within a signal handler, as that can cause problems with the signal system.
+
+Since callbacks can be connected to many signals with `RegisterSignal`, it can be difficult to pin down the source that a callback is invoked from. Any new `SIGNAL_HANDLER` should be followed by a comment listing the signals that the proc is expected to be invoked for. If there are multiple signals to be handled, separate them with a `+`.
+
+```dm
+/atom/movable/proc/when_moved(atom/movable/A)
+ SIGNAL_HANDLER // COMSIG_MOVABLE_MOVED
+ do_something()
+
+/datum/component/foo/proc/on_enter(datum/source, atom/enterer)
+ SIGNAL_HANDLER // COMSIG_ATOM_ENTERED + COMSIG_ATOM_INITIALIZED_ON
+ do_something_else()
+```
+
+If your proc does have something that needs to sleep (such as a `do_after()`), do not simply omit the `SIGNAL_HANDLER`. Instead, call the sleeping code with `INVOKE_ASYNC` from within the signal handling function.
+
+```dm
+/atom/movable/proc/when_moved(atom/movable/A)
+ SIGNAL_HANDLER // COMSIG_MOVABLE_MOVED
+ INVOKE_ASYNC(src, PROC_REF(thing_that_sleeps), arg1)
+```
+
### Operators
#### Spacing of operators
diff --git a/code/__DEFINES/dcs/dcs_helpers.dm b/code/__DEFINES/dcs/dcs_helpers.dm
index ba2b9a704a3..c5c7e3c42dd 100644
--- a/code/__DEFINES/dcs/dcs_helpers.dm
+++ b/code/__DEFINES/dcs/dcs_helpers.dm
@@ -10,10 +10,6 @@
/// Every proc you pass to RegisterSignal must have this.
#define SIGNAL_HANDLER SHOULD_NOT_SLEEP(TRUE)
-/// Signifies that this proc is used to handle signals, but also sleeps.
-/// Do not use this for new work.
-#define SIGNAL_HANDLER_DOES_SLEEP
-
/// A wrapper for _AddElement that allows us to pretend we're using normal named arguments
#define AddElement(arguments...) _AddElement(list(##arguments))
/// A wrapper for _RemoveElement that allows us to pretend we're using normal named arguments
diff --git a/code/modules/surgery/organs/heart.dm b/code/modules/surgery/organs/heart.dm
index c2e847de6a9..9dac53823a4 100644
--- a/code/modules/surgery/organs/heart.dm
+++ b/code/modules/surgery/organs/heart.dm
@@ -292,7 +292,7 @@
/obj/item/organ/internal/heart/cybernetic/upgraded/proc/shock_heart(mob/living/carbon/human/source, intensity)
- SIGNAL_HANDLER_DOES_SLEEP
+ SIGNAL_HANDLER // COMSIG_LIVING_MINOR_SHOCK + COMSIG_LIVING_ELECTROCUTE_ACT
if(!ishuman(owner))
return
@@ -305,10 +305,11 @@
if(emagged && !(status & ORGAN_DEAD))
if(prob(numHigh))
to_chat(owner, "Your [name] spasms violently!")
- owner.adjustBruteLoss(numHigh)
+ // invoke asyncs here because this sleeps
+ INVOKE_ASYNC(owner, TYPE_PROC_REF(/mob/living/carbon/human, adjustBruteLoss), numHigh)
if(prob(numHigh))
to_chat(owner, "Your [name] shocks you painfully!")
- owner.adjustFireLoss(numHigh)
+ INVOKE_ASYNC(owner, TYPE_PROC_REF(/mob/living/carbon/human, adjustFireLoss), numHigh)
if(prob(numMid))
to_chat(owner, "Your [name] lurches awkwardly!")
owner.ForceContractDisease(new /datum/disease/critical/heart_failure(0))
@@ -318,14 +319,14 @@
heart_datum.change_beating(FALSE) // Rambunctious Crew - Stop My Fucking Heart
if(prob(numLow))
to_chat(owner, "Your [name] shuts down!")
- necrotize()
+ INVOKE_ASYNC(src, PROC_REF(necrotize))
else if(!emagged && !(status & ORGAN_DEAD))
if(prob(numMid))
to_chat(owner, "Your [name] spasms violently!")
- owner.adjustBruteLoss(numMid)
+ INVOKE_ASYNC(owner, TYPE_PROC_REF(/mob/living/carbon/human, adjustBruteLoss), numMid)
if(prob(numMid))
to_chat(owner, "Your [name] shocks you painfully!")
- owner.adjustFireLoss(numMid)
+ INVOKE_ASYNC(owner, TYPE_PROC_REF(/mob/living/carbon/human, adjustFireLoss), numMid)
if(prob(numLow))
to_chat(owner, "Your [name] lurches awkwardly!")
owner.ForceContractDisease(new /datum/disease/critical/heart_failure(0))