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

Remove cyborg antagonist roles on death #17215

Merged
merged 6 commits into from Dec 6, 2023

Conversation

DasBrain
Copy link
Contributor

@DasBrain DasBrain commented Dec 4, 2023

[silicons] [bug]

Fixes #17170, ##16113, #13569

About the PR

Cyborg antagonist roles (emagged/syndicate cyborg) are now (hopefully) reliably removed from cyborg antagonists.

Why's this needed?

Currently the roles are not removed if the head part is removed.
This leads to a bunch of bugs.

Changelog

(u)DasBrain
(+)Cyborg antagonist roles are now successfully removed when the cyborg is turned into a ghost.

* Roles persist after the death of a cyborg - good for the crew credits.
* Roles are removed if the new body is not emagged/syndicate.
@keywordlabeler keywordlabeler bot added A-Silicons Deals with the angry metal robots C-Bug A bug that impacts usage of a feature labels Dec 4, 2023
@github-actions github-actions bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 4, 2023
Copy link
Contributor

@TobleroneSwordfish TobleroneSwordfish left a comment

Choose a reason for hiding this comment

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

If only it was that simple, this will lead to any syndie/emagged borg resurrected as a human through cloning or SR being classed as an antagonist. Roles being removed on death isn't really an issue for crew credits since they should use the historic antagonist list anyway (not sure why they don't)
I think the ideal handling of this is that emagged and syndie antag statuses should always be removed on death and re-added on assembly, since the antagonist status is inherently tied to the frame itself.

@DasBrain

This comment was marked as outdated.

@DasBrain DasBrain marked this pull request as draft December 5, 2023 00:20
@DasBrain DasBrain marked this pull request as ready for review December 5, 2023 00:20
Copy link
Contributor

@TobleroneSwordfish TobleroneSwordfish left a comment

Choose a reason for hiding this comment

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

You're going to need to explain beyond "No." and "It is.", you haven't actually answered the review.

@DasBrain
Copy link
Contributor Author

DasBrain commented Dec 5, 2023

So, I... removing the antag status of cyborgs when they are destroyed/brain pulled...
Makes my head hurt.

In theory... an emagged cyborg should have their antag status removed - because remove_on_death = TRUE.
But it is not. Emagged cyborgs keep their antag status.

So, the next best thing is: remove the antag status when they are put back into a body.
Cloning should not be a problem - remove_on_clone = TRUE.

That works - but... some people prefer antag removal on death.

Ok, let's try that again:

  • There are two ways a cyborg can "die": death(), and eject_brain().
  • eject_brain does use ghostize.
  • eject_brain Currently it also removes the syndicate role - making the body non-syndicate. ARGH.
  • eject_brain manually calls on_death for all antag roles - so emagged (which has remove_on_death = true) should be removed. It is not.
  • death calls eject_brain. So - ghosts with emagged cyborg antag role should not be possible. Yet here we are.

Of course, it is bad if the antagonist role is not removed - as they do not get the "equipment" a second time.
For an emagged cyborg that was moved to a fresh frame this means:

  • They still have the emagged cyborg role.
  • They are connected to the law rack.

And if they are then emagged again, we have the following situation:

  • They are not notified that they have been emagged (again).
  • They are still connected to the law rack.
  • They have the emagged cyborg antagonist role.
  • And they spark.

Which is the worst thing that can happen. Also see #17170.

Cloning is not an issue - the antag role is set to be removed on cloning. (Which I hope works better than remove_on_death)
It is also very common to have cyborgs with unrelated antagonist roles - when a humanoid antagonist is borged.


I am sorry for my last response - you did not deserve this.
I tried long to figure out why antag removal on death does not work for cyborgs - and I can not.
And then you suggested to just do that - which made me snap. Sorry.

@github-actions github-actions bot added the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Dec 5, 2023
@TobleroneSwordfish
Copy link
Contributor

I've managed to replicate and fix the syndicate cyborg brain removal issue, it's now handled the same as emag status (which yes may have bugs but at least it doesn't just delete itself on brain removal now)

I can't replicate any of the emagged bugs locally, the antag status has been removed and re-added fine every way I can test. This likely means either there's a major replication step left out of all the bug reports or it's somehow a race condition causing procs to be called in the wrong order due to server lag.

The paranoia borged check in this PR is a good workaround in the meantime, but please keep the status set to remove_on_death = TRUE for both emagged and syndicate statuses since that's what should be happening (and I think is, most of the time)

Also apology accepted, code can be frustrating :)

…nto antagcyborgs

# Conflicts:
#	code/mob/living/silicon/robot.dm
#	code/modules/antagonists/syndicate_cyborg/syndicate_cyborg.dm
@github-actions github-actions bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2023
@DasBrain
Copy link
Contributor Author

DasBrain commented Dec 5, 2023

I've managed to replicate and fix the syndicate cyborg brain removal issue, it's now handled the same as emag status (which yes may have bugs but at least it doesn't just delete itself on brain removal now)

Yes, I have seen b56dfdc.
Unfortunately, I suspect destroyed syndicate borgs will drop normal frames now. (The emagged, `freemodule and syndicate variables need to be transfered over to the frame.)

I can't replicate any of the emagged bugs locally, the antag status has been removed and re-added fine every way I can test. This likely means either there's a major replication step left out of all the bug reports or it's somehow a race condition causing procs to be called in the wrong order due to server lag.

It is a very common thing, at least on RP.
I do not know what causes this - which is frustrating.
I can send out an ahelp next time it happens - maybe it helps to reproduce it.

The paranoia borged check in this PR is a good workaround in the meantime, but please keep the status set to remove_on_death = TRUE for both emagged and syndicate statuses since that's what should be happening (and I think is, most of the time)

I think this is a good compromise. I will update this PR to do that and synchronize the syndicate flag again.

Thank you for accepting my apology.

Giving out the equipment again should link to the right rack in (hopefully) all cases.
@DasBrain DasBrain marked this pull request as draft December 5, 2023 23:41
@DasBrain
Copy link
Contributor Author

DasBrain commented Dec 5, 2023

Heureka! I found it!

Reproducer:

  • Be an emagged/syndicate cyborg
  • Have your head removed (you can do it yourself)

This will remove the head, call update_apperance, which will call death, which in turn calls eject_brain, but eject_brain checks at the beginning if there is even a head unit and returns.

In that cases the antag status is not lost.


Now I just need to fix THAT.

@github-actions github-actions bot removed the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Dec 6, 2023
@DasBrain DasBrain marked this pull request as ready for review December 6, 2023 01:26
@DasBrain DasBrain changed the title Uniform handling of cyborg antagonist roles Remove cyborg antagonist roles on death Dec 6, 2023
@TobleroneSwordfish TobleroneSwordfish merged commit 425bb60 into goonstation:master Dec 6, 2023
21 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Silicons Deals with the angry metal robots C-Bug A bug that impacts usage of a feature size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cyborg Emag Issue
2 participants