PlayerRevive

PlayerRevive

5M Downloads

Compatibility issue with Vampirism - DeathEvent

maxanier opened this issue · 18 comments

commented

My mod Vampirism provides a similar feature to this mod.
Vampires do not immediately die from certain damage sources. Instead they become DBNO (down but not out), lay on the ground and are unable to move/interact. They can choose to wait for some time after which they are able to simply resurrect, or give up and actually die.
We also listen for the LivingDeathEvent on highest priority and cancel it if appropriate
https://github.com/TeamLapen/Vampirism/blob/a3c786abbdb287bafa0d4a3d7de802aa492ecfdf/src/main/java/de/teamlapen/vampirism/player/ModPlayerEventHandler.java#L311
https://github.com/TeamLapen/Vampirism/blob/a3c786abbdb287bafa0d4a3d7de802aa492ecfdf/src/main/java/de/teamlapen/vampirism/player/vampire/VampirePlayer.java#L619

This leads to an issue together with PlayerRevive: TeamLapen/Vampirism#1105
From my interpretation it seems like PlayerRevive kicks in first. Once the time frame for a revive is over and the player is killed "again" with the old damage source. Vampirism takes over, which is unexpected by PlayerRevive. Somehow leading to the reported result.
I did not test debug this yet, so it might be different.

There are two questions now:

  1. How to fix this soft lock
  2. What would be the most suitable behavior with both mods installed

Bug

My current assumption is the PlayerRevive user down state and interface is not sufficiently "cleaned" because a player respawn is expected. Hence, when the player does not actually die afterwards, something is not correct.

Compatibility

It probably does not make too much sense for vampire players to have two different resurrection/revive abilities consecutively.
Ideas
a) The PlayerRevive functionality is disabled for vampire players (but not for the non-vampire players). This would likely require some small optional API dependency in PlayerRevive on Vampirism, (maybe it possible to do this via the scoreboard which should contain information about the player vampire status. This would require any code dependency)
b) The DBNO functionality in Vampirism is disabled when PlayerRevive is installed (Simple ModList check)
c) Both things are combined. During vampire DBNO state, so while players is waiting to be able to resurrect on their own, another player could revive them. This is the most complex solution and would likely require changes in both mods. Not sure if this is worth it.

What do you think?

commented

Sorry for taking even longer to respond :D

Thank you for fixing the problem for 1.19. I can confirm it works fine now.
Are you still supporting 1.18 or even 1.16 and include the fix in a future release?

Regarding compatibility:

I agree that this is the best (and probably only worthwhile) option.
After testing it a bit, I came up with the following idea:
Vampirism's revive system only kicks in after either PlayerRevive timeout or when the player gives up.
We could simply take the time which the player spent in "PlayerRevive down" state before and subtract this from the resurrection waiting time.
This would mean Vampirism just needs a way to retrieve that value. No need for any GUI or player interaction changes.

Implementation

Regarding the implementation my idea is the following rather ugly, but simple system:
You provide a static method, whose signature preferably does not change in future versions, that returns the ticks spent in PlayerRevive down state for a given player.
Vampirism could then try to retrieve that value if PlayerRevive is installed via reflection (with sufficient try-catch) at the beginning of the DBNO system.
This would be easy to implement and maintain (at least on our side) and the reflection/try-catch should not have a performance impact as it is only used very rarely.

What do you think? Is this possible, or do you have another solution.

commented

But the benefit of this solution would only be a shorter down time right? Not sure if that really solves the situation. I would rather got for my c) solution. I still think it's easy to implement also using some static methods (which should not change).

Furthermore I don't know why people think reflections are slow. It's just one or two extra method calls. I don't think you will ever notice a difference. The performance heavy part is to locate a class, method or field, but with a simple cache it's only done once.

commented

Sorry for not responding earlier. Thanks for all the detailed information.

Source of issue
PlayerRevive adds a capability to the player, which does not get reset if the player is forced to die (instead a new one will be created once the player respawns). This issue can easily be fixed and will be in the newest version for 1.19.

Compatibility
a) I could easily add support for it, but this would make it impossible for them to be revived.
b) Then it would not be possible for Vampires to resurrect on their own.
c) Is obviously more work but I think the best solution. If being able to resurrect yourself is the only feature of the vampire DBNO, I think it would be easier to use the system of PlayerRevive. This would require solution b) + implement the resurrection feature for PlayerRevive. Not sure how your code works, but maybe you can keep almost all the same except of the detecting a player being downed and the actual revive call. I could add hooks for both of them.

Let me know what you think. For now the issue is at least solved.

commented

The idea is that vampire players can be revived by a friend during the PlayerRevive timeframe or alternatively, if there is nobody nearby or the time runs out, choose to resurrect via Vampirism after waiting Vampirism's DBNO period. That period would then be the same regardless of how long they stay in the PlayerRevive screen. Of course this means they have too manually skip that screen or wait the timer to be able to resurrect (after the waiting time).

Your c) suggestion is to combine this into one screen, where the player can wait for a teammate, wait for the cooldown and resurrect or give up, right?
This is a bit better, because the player does not have to skip the PlayerRevive screen to resurrect, but the waiting time is unchanged. But it is also more work, probably on your end, because your system kicks in first and maybe more error prone.

If we go with your c), I could provide a static method that takes DamageSource and Player object and then returns the waiting period (or e.g. -1, if not applicable) and you introduce another control to your screen that allows the player to resurrect after the waiting period has passed.

commented

It seems like I got no idea what Vampirism does. I thought a vampire was in a DBNO and could use the time to resurrect. So my idea was to remove this DBNO entirely (if PlayerRevive is installed) and add the ability to resurrect while being downed in PlayerRevive. But it seems like that is not how it works.

In theory a player can also skip the PlayerRevive waiting time, so maybe it is fine already?

commented

Looks good. DownedTime is what I am interested in.
I will build a PlayerRevive version and test it with Vampirism this weekend.

commented

Uploaded a new versions which contain the helper methods.

commented

Perfect, thank you.
I implemented the part on our side and it works as intended:
TeamLapen/Vampirism@838f9ed

One last issue I noticed while testing (which occurs regardless of this integration):
When the player skips (or waits) the PlayerRevive period and then chooses to give up (in Vampirism's DBNO state) they return to a (new) PlayerRevive screen. Vampirism attacks the player with DamageSource.GENERIC 10000 when they give up.

Two options to avoid this:

commented

Ah, sorry, was busy with work and am now on vacation, will get back to this in a couple of days.

commented

@maxanier still there?

commented

Oh, then I failed to properly explain Vampirism's DBNO feature, sorry.
Yes, I agree, with the earlier implemented fix, everything is working fine.
Some small improvement is still possible though:
Right now the player has to decide whether they want to wait for a friend to come by and, if nobody comes by, wait for the DBNO timeout or alternatively skip the PlayerRevive time and go to DBNO right away (which means even if somebody comes by, they cannot do anything).
With the implementation proposed here #97 (comment), the player could just wait until either a friend comes by or the DBNO timeout is over.

But this is only a small QoL improvement. If you say, that is not worth the effort, I'm perfectly fine with that.

commented

Now I was on vacation, so I could not respond earlier.

I still do not understood fully how things work, but it does not matter anyway. If all you need are a few static helper methods I'm totally fine with it.

Do these fit your needs? https://github.com/CreativeMD/PlayerRevive/blob/1.19/src/main/java/team/creative/playerrevive/server/PlayerReviveServer.java#L23-L33
I have not released them, first wanted to check if that works for you.

commented

Hi guys how are you, special thanks to you both for fix this bug. I have seen the [Vampirism-1.18.2-1.8.7] release that already implements this fix, i would like to know if you are going to implement it to the 1.16.5 too, because i tried the [Vampirism-1.16.5-1.9.0-beta.1] update that you uploaded the same day as the 1.18.2 version but the fix is not implemented yet, at least the bug still happening to me.
Thanks to you anyways for this 2 fantastic mods!
Regards

commented

@maxanier I'm sorry, but I don't support 1.16.5 anymore.

commented

I think the second option is better. Added it 983e645

commented

Released it on curseforge. Was a pleasure to work with you! Glad we could solve it.

commented

Thank you very much. Works perfectly now.
Thank you for making this possible

commented

@mathi0101 I might add a workaround to 1.16, which disables DBNO if PlayerRevive is active to avoid getting stuck