Incompatibility with several mods due to grave creation time

[RedPxnda]

I’m the developer of Respawn Obelisks, a relatively small mod. A user of mine has reported incompatibility with this mod. My mod attempts to intercept player item drops and keep them, but not necessarily with a soul bound enchantment. I do this during the drop method of LivingEntity, whereas you create graves and handle items during onDeath of ServerPlayerEntity. At first, I was just going to make my execution happen before yours, but I quickly found out that breaks my compatibility with another mod: Revive. Revive attempts to stop the player from dropping their items when they die, and it turns out that doesn't work with this mod either, since it tries to redirect the LivingEntity drop call(after you create a grave). I believe the best way to fix this issue is for this mod to instead similarly prevent the player from dropping their items when they do actually drop them, which should fix problems with both of the aforementioned mods.

TLDR; Execution of YIGD's grave creation happens quite early, and breaks support with several other mods that too try to intercept item drops.

I am happy to make a PR to move execution to be on the actually dropping of the items, but I wanted to first clarify with you that that would actually be helpful.

[RedPxnda]

Another thing to add is that Trinkets' TrinketDropCallback fires during LivingEntity#dropInventory, meaning that also never gets called with this mod. Said event is used to change the drop rule of each dropped item.

[B1n-ry]

I am aware of this, but it's actually necessary for me to have my execution time early. It turns out that if I move execution to when the player drop all their items, other things can break. For example, if I would have my graves generated when the player drops all of their inventory, trinkets will already have dropped their items, and I can't store them in my graves. This will allow me to store items in the graves from a wide variety of mods, but yes in some instances this will happen that some other mod who also wants to change item drops is prevented from doing so. I like to manually provide compat with these mods. In my opinion it makes it easier for both me and players using the mod with the other mods. I will for example store a snapshot of the inventory as it was before everything was dropped, and then depending on a ruleset, give some back when the grave is looted, when the player respawn, drop them on the ground, or just treat them as "vanished" or "destroyed". I'm aware of the trinkets specific drop rules, and I'm checking them manually through my compat implementation, to make sure they are followed. If there's any edge-case however you have found that I do not follow, do let me know about it. Finally when it comes to revive, (I haven't looked at the mod recently, so I'm not sure exactly what they do), it would make sense if they had their code injected earlier, but I'm not sure how our mods would play along in that case. It might be possible to just redirect the specific onDeath call from LivingEntity#damage, and then call the function separately, but idk maybe there's a better reason why the code is put where it is. There's also a chance I have this opinion just because I don't want to move my stuff, as that could possibly have unforeseen consequences, and I want to provide an as stable experience as possible (since if something goes wrong, players might lose all their items randomly). I'd be interested if there's anything I should be aware of to implement compatibility on my end. Also do let me know if you have any counter-arguments you'd like to mention. I'd appreciate the feedback!

[RedPxnda]

Where later in specific did you inject? Don’t inject into LivingEntity#dropInventory, and instead into LivingEntity#drop. Trinkets does it into LivingEntity#dropInventory, and LivingEntity#drop just calls dropInventory. You can always change your mixin priority to make your stuff fire when you want. I've been using LivingEntity#drop with Trinkets and it's been working perfectly fine, it executes before Trinkets drops. I've looked at other grave mods(specifically UniversalGraves) and they inject into LivingEntity#drop while still maintaining compat with many of the same mods you do. I can attempt to get it to work and make a PR, if you'd like. Getting compat to work with my mod is one thing, but I think with the current execution location it will be pretty difficult to get it to work with Revive. Revive tries to stop all drops(Redirect on its call in ServerPlayerEntity), and instead recall the drop method later if it needs to drop the player's items. If you inject into the drop method instead, Revive's behavior will work correctly; items won't be dropped at all(no grave spawned, like intended), until Revive calls drop itself, and then a grave IS spawned like intended.

[B1n-ry]

I guess I could try to move the injection point to LivingEntity#drop. I did have an issue in the past that fixed itself by moving from that injection point to ServerPlayerEntity#onDeath (which is the same injection point that DeathLog uses) from LivingEntity#drop. I've previously used PlayerEntity#dropInventory, but I'm not even sure that's in a released version, meaning it's long ago I did that. After that (and the aforementioned issue) I've concluded that putting the code execution earlier will prevent other mods from messing things up for me. I will try to change the injection point though, and I'll see what happens. There are quite a number of mods now that add their own inventory implementation (I provide compat for all I know, which there are 7 of, which I believe is more than universal graves does), and there's a chance some compat might break. I'm still interested in providing compat with respawn obelisks though! There are a lot of rules that can be applied to the inventory storage mechanisms in my mod, so people could in that case likely decide on their own how they would want inventories to be handled

[RedPxnda]

Alright well, keep me updated! Respawn Obelisks is a bit of a challenging case, as it only conditionally saves items, and that condition often has to be tested after/when the player respawns rather than when the player drops their items. (If they respawn at a Respawn Obelisk, only then should I keep the items, but it is possible to choose where to respawn in the death screen.) Right now, I just test the condition when the player dies, store the items that need to be kept(and remove them from player's inventory), and then drop them if the player chooses to not spawn at the obelisk. Meaning, for the potentially kept items, they are still dropped rather than being put in a grave. Even with direct compat, I don’t know how this could be fixed.

[B1n-ry]

Should be mostly doable to implement, as long as there's something to check an obelisk was used when the player has respawned, and if yigd still takes priority with its mixins. The problem with spawning the items where the player died on respawn, instead of on death, is something that will not be as straight-forward to implement, but it's definitely possible to do. I'll let you know how it goes

[RedPxnda]

The player clone event(I’m an architectury dev, fabric probably has some equivalent- if not then the clone method is called somewhere in PlayerManager#respawn) provides access to both the old(dead) player and the new(respawning) player. That will likely prove useful.

[B1n-ry]

Yeah I already have systems in place for respawning. Both for soulbound and other stuff. So adding/removing items from the grave shouldn't be a problem, only dropped items, but as I stated that can probably be implemented somehow anyway.

Most people think of YiGD as a grave mod, when in reality it's evolved into more of a death management mod, and graves are just one of the many features it brings (be it quite a bigger feature than most other features in the mod)

[RedPxnda]

Alright.

As far as checking if the player respawned at an obelisk, you should just be able to check their current respawn point. As long as you don’t fire after I do(in player cloning), it should generally be the most recent point.(I sometimes(config dependent) sort the player's respawn points based on predefined priority values)

[B1n-ry]

Based on my very limited testing, it seems to work well enough having the injection point at LivingEntity#drop. It might still cause issues with other mods, like mods implementing soulbound traits to items, but that remains to be seen. I'll take a better look at compatibility tomorrow though

[RedPxnda]

Alright. Traditionally, I would imagine soul bound enchantments to be implemented either in that method or the dropInventory one. Either way, as long as you have a lower mixin priority(default is 1000), you should be able to deal with that.

[B1n-ry]

Hello again! It's been a few months, and most of that time I've just simply procrastinated modding, and focused on other things. I just pushed a new version with compatibility though. I think I've gotten everything down the way your mod has it, although it might get a little bit weird on certain XP keep configurations on both our mods. For items I've built a more robust system though, mostly because XP is basically just a number that increases or decreases, while items are ... more complicated. Just to mention the soulbound thing, yes. Most mods would traditionally implement it after LivingEntity#drop, but some mods use pre-defined events (for example twilight used ServerLivingEntityEvents#ALLOW_DEATH in their 1.19 fabric versions). These might trigger before LivingEntity#drop. As twilight has not updated yet to 1.20, it might not be a problem anymore though.

I'll mark the issue as closed, as the initial issue has been fixed, and there is now compatibility with respawn obelisks. Let me know if you would happen to find anything that I can improve on, or if something breaks!