-
-
Notifications
You must be signed in to change notification settings - Fork 682
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
Refactors ghost-role sentience #11897
base: master
Are you sure you want to change the base?
Refactors ghost-role sentience #11897
Conversation
Does this also affect sentience baloons? |
No, it affects everything listed in the PR body and those things only |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
return TRUE | ||
if(href_list["activate"]) | ||
var/mob/dead/observer/ghost = usr | ||
if(istype(ghost)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(istype(ghost)) | |
if(isdead(ghost)) |
istype is not sufficient to prevent href exploit.
var/mob/living/parent_mob = target | ||
if (istype(parent_mob)) | ||
if (can_become_role(player.client, parent_mob) != GHOST_SPAWN_ABLE) | ||
return | ||
else | ||
if (can_become_role(player.client, null) != GHOST_SPAWN_ABLE) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you check istype()
.
does it mean there can be null or path?
/// If set | ||
/// Have we alerted already? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two lines, mistake? also, instead of questioning sentence, please describe normally.
/// If set, implies the spawner is alerted already
/// mobs for that type (A unique alert will not appear if another exists at the | ||
/// same time, which prevents spamming) | ||
var/unique = FALSE | ||
/// If we are using a spawned type, should we delete the parent after spawning? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// If we are using a spawned type, should we delete the parent after spawning? | |
/// Decides if the parent should be deleted after spawning (When we use a spawned type) |
Questioning tone is bad to read.
/datum/component/ghost_spawner/UnregisterFromParent() | ||
var/mob/living/parent_mob = parent | ||
parent_mob.RemoveElement(/datum/element/point_of_interest) | ||
remove_from_spawner_menu() | ||
UnregisterSignal(parent_mob, COMSIG_ATOM_ATTACK_GHOST) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some missing unregister signals:
- COMSIG_LIVING_DEATH,
- COMSIG_MOB_GHOSTIZE
I am not certain if it's not necessary.
/// When the mob dies, remove from the spawner menu but don't remove this | ||
/// component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the component is missing of on_mob_revival()
proc that reenables a mob's ghost spawner that reverts the effect of on_mob_death()
/// If set, we will become bound to this mob upon spawning | ||
var/datum/mind/master = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be weakref
ah, also
The first one makes sense to send signals to make things restricted, but the second one doing restrictions isn't fair. |
I encountered an issue where diona nymphs could bypass the ghost-role cooldown, allowing for you to have infinite lives in combat scenarios. Simple fix right, wrong. This uncovered one hell of a mob spawning rabbit hole that goes deeper than you could have ever imagined.
About The Pull Request
Things are now standardised:
There are 2 types of ghost sentience options:
Makes the spawn message when you enter a body more clear:
Also ghosting will now give you a ghost role cooldown.
The following sentience spawns are affected:
Other changes:
Note that this doesn't quite convert everything, only the stuff that was previously going through the sentience system. Convertnig mob_spawner would be a lot of work.
Why It's Good For The Game
Things being standard is good, it means that any issues will be shared across all ghost spawners making them much easier to fix.
Testing Photographs and Procedure
Nymphs
1 alert for all nymphs
Clicking on a specific message gives you that specific mob
Spawner menu:
Pyro Slime
Ghosting:
Spiders
Changelog
🆑
fix: Nymphs now appear in the ghost menu and respect respawn delay.
add: Adds a message for ghost role spawning
fix: Standardises ghost sentience spawning and ensures that enslaving works for all spawned mobs.
/:cl: