Role swap handling and a couple other fixes

[AaronMcKenney]

When I released the Cursed role, a Steam user reported an interesting issue that would occur if the Pharaoh placed an ankh, swapped with the Graverobber, and attempted to steal their own ankh. In my efforts to reproduce the issue I found a few more that would occur from role swapping. The current code does not seem to always account for these strange cases where the Graverobber and Pharaoh roles move around frequently. I went ahead and took a look at the Pharaoh code and one thing lead to another... I ended up changing many things for this role. Good things I hope, but let's get into it.

Firstly, here are the notes (not necessarily bugs) that I compiled during my debug sessions and visual inspection of the code:

1. Ankhs and their structures persist across role changes. A player can revive from these Ankhs even if they are no longer Pharaohs or Graverobbers.
2. A player can place multiple Ankhs down via role swaps, as long as they make sure to place down their ankh as a Pharaoh before each role swap. If they die only the last ankh they placed will revive them. However, they can pick up another ankh that they previously placed and put it down again to "reactivate" it.
3. A player can pick up an ankh they placed down many role swaps ago, provided that they are currently a Pharaoh/Graverobber.
4. The decal IDs for the ankhs aren't unique, which may lead to a rune_neutral decal suddenly changing if the entity ID for a destroyed ankh is reused
5. Each time a player places an ankh down, a Traitor becomes a Graverobber, which means that with many role swaps all Traitors will lose their original role.
6. The "Adversary System" seems to imply that each Pharaoh shall have one associated Graverobber, and that the Graverobber will only be able to steal that Pharaoh's ankh. This may become confusing for 2+ Graverobbers, due to a lack of visual indication over which ankh each Graverobber has been assigned to steal.
7. As mentioned prior, if the Graverobber and Pharaoh swap roles, then there will be confusion over who owns the placed ankh.
8. Ankh destruction logic does not elegantly handle 3rd party addons (ex. Prop Exploder). The decal will not update to rune_neutral. A slain Pharaoh will attempt to revive if their ankh is destroyed by a Prop Exploder (since it still thinks the ankh exists), but the revive will fail due to the ankh missing.
9. A player can instantly heal their ankh by placing it down and picking it up again.
10. A player's ankh will be instantly destroyed if they die while it is in their inventory. The Graverobber is not notified of this.
11. weapon_ttt_ankh/shared.lua appears twice in the file hierarchy (under both the gamemodes and lua folders)

Here's a quick summary of the changes I made and why, which affect Notes 2,5,6, and 7:

1. A series of checks have been added to ensure that any player (no matter what role they have) only has at most one ankh associated with them. They may not receive, place, or steal another Ankh while they own one. A player may acquire another Ankh if and only if their previous ankh is broken or used. To otherwise allow multiple ankhs to be acquired could lead to a game where ankh hoarding and/or griefing occurs.
2. A dismantling of the Adversary System. Instead of creating many Graverobbers, only one is made per game from placing down ankhs (technically more can pop up via Unknowns and other such roles). Graverobbers will be able to steal any ankh they come across as long as they don't already own an ankh. Only the robbed Pharaoh can steal it back. This was done to reduce edge cases and prevent Graverobbers from crowding out other special Traitor roles such as the Glutton and Mesmerist.
3. The introduction of the Dual Ownership System. In sh_pharaoh_handler.lua, ankhs are now maintained by keeping track of their Original Owner (the player who first placed the Ankh when they became a Pharaoh) and their Current Owner (the player who currently controls the Ankh). Every ankh is stored in a table called ANKHS, with the original owner's SteamID64() being the key. There are many methods of interacting with Ankhs under this new system, and I believe it to be more flexible than the previous Adversary System.
4. To account for Pharaohs and Graverobbers swapping roles, the meaning of their colors had to change. An ankh will now be yellow if the Original Owner happens to currently own it (ex. they just placed it down for the first time as a Pharaoh). An ankh will now be orange if it has been stolen (ex. The current owner is not necessarily a graverobber any more, but at some point they were and stole an ankh).

Finally, I decided that Notes 4,8,9, and 11 were all issues that I should fix. So I did that. I also fixed Issue #7 (Jester can damage the ankh) and also added similar support for the Marker and Cursed roles (who also can't damage other players).

Notes 1,3, and 10 are assumed to be intentional, and still occur with these changes.

These changes were tested with multiple players in multiple sessions in order to stomp out as many bugs that may have been introduced as possible. I do not believe these changes will alter standard gameplay (Wherein only one Pharaoh exists, and no role swaps occur) in any significant manner.

Apologies for dumping this on you! It probably would have been better to have a discussion before making these changes, or make them over the course of multiple smaller pull requests, but I couldn't help myself (writing the Dual Ownership System was fun). Let me know what you think!

[ZenBre4ker]

While it's Tim's role I can atleast admit, that you put a lot of work into it At a first glance I agree with you, but I am wondering about two things, first there shouldnt be any graverobbers, when there are no pharaohs, so actually the ankhs should disappear accordingly. Second, I wonder if the TTT2 system shouldnt exclude such secondary roles as the graverobber, sidekick, zombie or infected from role swapping. Maybe via a little tag. Don't exactly know how unknown and doppelganger are handled for example. So yeah there should be only one graverobber and they should be converted back, when there is no pharaoh or not?

[AaronMcKenney]

While it's Tim's role I can atleast admit, that you out a lot of work into it At a first glance I agree with you, but I am wondering about two things, first there shouldnt be any graverobbers, when there are no pharaohs, so actually the ankhs should disappear accordingly. Second, I wonder if the TTT2 system shouldnt exclude such secondary roles as the graverobber, sidekick, zombie or infected from role swapping. Maybe via a little tag. Don't exactly know how unknown and doppelganger are handled for example. So yeah there should be only one graverobber and they should be converted back, when there is no pharaoh or not?

1. If I understand correctly, you are suggesting that if the Pharaoh changes their role their Ankh is destroyed and the Graverobber changes to their previous role. Something that would need to be addressed: What if it is the Graverobber who swaps roles, and the ankh is safe/stolen?
2. Regardless, while this solution would work, I'm not that big of a fan of it. Role swaps tend to be sudden things that take players by surprise. In this case, it would mean that the former Pharaoh would lose their ankh and the former Graverobber would lose the ability to steal the ankh. It just doesn't seem to be particularly nice to those two players, as they may have spent a large amount of time strategizing about the ankh only for it to get suddenly taken out of their hands. In addition, with regards to the Cursed which can cause rapid role changes to occur, the Pharaoh and Graverobber would be devalued as roles, as in many cases their ankh would just get yanked from them.
3. Off the top of my head, here are all of the roles that can swap and change roles: Amnesiac, Jester, Mimic, Doppelganger, Unknown, and Cursed. I don't think any of them make check for secondary roles. Personally I think it's a cop-out. The Unknown in particular would probably be a bit sad if they were killed and didn't respawn because their killer was a secondary role. Personally, I think it's more fun to allow for these kind of weird edge cases to happen in a graceful manner (It's why I made this pull request, as opposed to just having the Cursed fail to swap if it comes across a Pharaoh/Graverobber).

[ZenBre4ker]

1. If the graverobber swaps roles, he loses the ability to steal it therefore the ankh would either return to its owner (pharaoh) or would drop out of the inventory, as hes not able to possess it anymore. I would just make this whole play exclusive to pharaoh and graverobber.
2. Well not to be rude, but strategizing over something like this probably seldom happens, so it would be irrelevant in my opinion. Otherwise I think, that if roles change you need new strategies anyway, therefore this shouldnt be an issue, as the roles are not there anymore.
3. Just forbidding the roles would be a weak solution anyways. I rather mean, that you cant choose those. I dont know how they change roles, but excluding secondary roles directly for choosing would be my way of doing that. So that, ttt2 only gives you a list of morphable roles, that are primary for example.

[AaronMcKenney]

2. Well not to be rude, but strategizing over something like this probably seldom happens, so it would be irrelevant in my opinion. Otherwise I think, that if roles change you need new strategies anyway, therefore this shouldnt be an issue, as the roles are not there anymore.
3. Just forbidding the roles would be a weak solution anyways. I rather mean, that you cant choose those. I dont know how they change roles, but excluding secondary roles directly for choosing would be my way of doing that. So that, ttt2 only gives you a list of morphable roles, that are primary for example.

2. Well you will need new strategies if you change roles, but I don't see the reason for excluding the Ankh from one's strategies. If a player manages to place or steal an Ankh, I think the game will be more interesting if it isn't removed just because of a role change. They will have more options available to them because of their ankh. They will also still have to worry about a Graverobber, who could now be anyone. I don't understand what you mean about "the roles are not there anymore", they are still there, just given to different players.
3. I don't understand this point. How is "you can't choose those" different from forbidding roles? What is the in-game result of this proposed restriction for a role swap? For example, would an Unknown killed by a Graverobber respawn as a generic traitor or...?

[ZenBre4ker]

2. Well if roles change and they have new ones, they shouldnt have the option to be still in possession of an ankh I mean. It's their role that made that happen and therefore with a removal of that role, they should concentrate on their newly assigned role or not? A traitor that would get converted also loses it shop and only keeps his credits. His goal isnt anymore to kill all innocents, therefore a former pharaohs or graverobbers role shouldnt matter after a conversion. Somebody else takes on that purpose.
3. Thats a good point, didnt think of the roles that change via getting killed. My suggestion would be a overwritable function IsRoleAvailable() where the secondary role could decide if there is place for it. Otherwise TTT2 would handle it by giving him the innocent role as it is always free. If there are as many graverobbers as pharaohs, the role wouldnt be available.

[AaronMcKenney]

2. Well I hate to repeat myself, but I think it makes for more interesting gameplay if the Ankh still exists. There is also the edge case of a Graverobber losing their stolen Ankh and role, only to be made a Graverobber again after a hot minute due to the new Pharaoh placing down their Ankh, meaning that all of their work was just thrown out for nothing. Maybe if the Ankhs just don't offer revival if their current owner isn't a Pharaoh/Graverobber as opposed to being outright destroyed? I already have it that they can't pick up or place down Ankhs unless they are Pharaohs/Graverobbers. So it isn't that big of a step to make the Ankhs functionally inert to current owners who aren't Pharaohs/Graverobbers. I don't agree with your 2nd paragraph here. My Impostor role can place vents to create a teleportation network. Those vents don't suddenly disappear if the Impostor changes roles. The other traitors (and the new Impostor) can still use it. Heck, a Traitor can place down C4, but that doesn't suddenly disappear if the Traitor becomes Infected or a Sidekick or whatever. That C4 can still explode. Basically what I'm trying to argue here is that role-generated-entities should be divorced from the roles themselves (and it is only how they interact that changes). They still have the potential to impact the game if a role change occurs, and it is more interesting if they do.
3. I'll argue that this is largely a thing for the role addon developer to decide if they want or need to use such a restriction function. You seem to have taken issue with secondary roles swapping in general, and I'm not sure why? I don't understand the pushback. "Otherwise TTT2 would handle it by giving him the innocent role as it is always free." This would lead to easy Innocent victories if it negates secondary roles that are evil (not TEAM_INNOCENT nor TEAM_NONE).

[ZenBre4ker]

2. I get what you mean, but the vents are for traitors and the c4 just explodes, whereas the ankh only revives the one who placed it. Therefore that is role-specific only. But yeah I dont wanna repeat myself too and I gave my two cents here 😄
3. Thats why I stated to give an overwritable option, so that the addon developer can decide what happens in that case. My approach would be to give the role a tag if it is secondary. If it is, it can decide, whether the role can be chosen and if not, it could give an alternative for example, graverobber becomes former role or traitor. And if the role doesnt overwrite those, ttt2 decides to give default values, which in my opinion would be best to have not much impact, therefore giving them a simple innocent role ^^

[AaronMcKenney]

I don't want this pull request to be lost to the ages, so I'll summarize the discussion between Zen and I above, and ask something of Tim (the owner of this role):

There is a disagreement over how Ankhs should behave when Pharaohs and Graverobbers swap roles. I'll list out three general paths that could be taken:

1. Zen's idea. If The Pharaoh swaps roles, their Ankh is automatically destroyed and The Graverobber reverts back to their previous role. If The Graverobber swaps roles, the Ankh returns to its rightful owner if possible.
2. My idea (and the current implementation in this pull request). The Ankh is unchanged between swaps. The Ankh can still be activated by its current owner if they die, but it cannot be picked up or stolen by players who are no longer Pharaohs/Graverobbers.
3. Another idea, mentioned later on in the discussion. Similar to (2), except that if the current owner changes roles, the Ankh becomes inert, and won't activate if its current owner dies. It can still be stolen provided the stealer has the correct role, and can reactivate if the current owner changes back to the correct role.

Each idea has their own pros and cons. Tim, as the role owner, could you pick which one you prefer (or offer your own idea)? I will be willing to implement whichever one. While I have my own preferences, as long as the role is able to handle role swaps, I will ultimately be happy at the end of the day.

[TimGoll]

Sorry that it took that long. Life is busy and then I forgot about it. But I took the time, read everything and have a weak opinion on this. I can understand both of you though.

First off, I 100% agree that only pharaoh / graverobber should be able to interact with the ankh directly. Glad that you both agree with this. I generally agree with everything else that was said.

And I honestly can't say which solution to the ankh on rolechange I prefer. But since this solution is already done and tested, it is fine for me. I see why it is a good solution. However I also see why Svens solution is good.

Therefore I approve of this. Feel free to discuss this further with me.

Another idea I got while reading this: Should the graverobber receive their original role back, if the pharaoh respawned at their ankh? This would be a nice idea in my opinion. It is out of scope for this PR though.