Fix soul beacon and amulet mirror - #106
Conversation
…pped and a grave is created from OpenBlocks. Warning: it doesn't support tconstruct.armor.player.TPlayerHandler.playerDrops. tconstruct.armor.player.TPlayerHandler.playerDrops attempts to handle null. (Why did those developers forget to add a null check?) No other errors. Tested on GTNH 2.8.4.
…e grave if there are no items left. Tested on Baubles slots, the off-hand, the Adventure Backpack (as an item), Tinker's Construct slots, and Galacticraft slots. Tested on GTNH 2.8.4.
|
I'll get to review this tonight, there where some changes made to gravestones that should be tested with this change in dailies |
|
I took a quick look at the changes to the grave creation logic. And I was hooked by the issue with extracting from pendingSnapshots; it will be null. But no exceptions are expected; there's a null check. |
thehoblit
left a comment
thehoblit
left a comment
There was a problem hiding this comment.
This does look good, I do have to ask why the two tiered event handler for the amulet? It seems like it introduces unnecessary fragility unless you want to somehow pass the amulet's location through to the next event with a thread or something.
Unless there is an expected behavior for changing the players inventory between the High and normal priority PlayerDropsEvent, I can't see why we wouldn't just handle the amulet when we see it.
IMO put it in a single handler and keep it at HIGH if need be.
|
This follows the original logic established by the author. The amulet should only activate when it's in a Buable slot. Simply activating it would make it unclear which inventory it came from, meaning it would always activate, no matter where it came from. |
|
|
||
| public void onEquipped(final ItemStack arg0, final EntityLivingBase arg1) {} | ||
| public void onEquipped(final ItemStack amulet, final EntityLivingBase arg1) { | ||
| amulet.stackTagCompound.setBoolean("isActivateTransport", true); |
There was a problem hiding this comment.
Only thing left is I think you should check the tags exist first before accessing like in the handler, just in case something absurd happens
There was a problem hiding this comment.
I'll check it in Forge now and add null to stackTagCompound if possible.
And you should probably check out this: https://github.com/GTNewHorizons/OpenBlocks/blob/397eb6179c413b06fa498b19b160ef1ec54ea85d/src/main/java/openblocks/common/PlayerDeathHandler.java#L370
There was a problem hiding this comment.
So as I understand it, with the same priority they will be competing based off of I believe registration/load order.
How would you like to proceed? It think an argument could be made for making graves lowest priority, all other mechanics should come before them as the default vanilla replacement.
It would also I think be worth bringing up in the discord #github-discussion to get some more experienced eyes on it. There very well may be more listeners I don't know about.
There was a problem hiding this comment.
Done, I've mimicked the original file's naming style a bit more. The main object itself can be null if we haven't bound it to the MirrorTile yet.
There was a problem hiding this comment.
I'll try running through the mods from 2.8.4 now.
There was a problem hiding this comment.
If you are going to be doing more testing, give a daily a go to get the new gravestone mechanics too
https://github.com/GTNewHorizons/DreamAssemblerXXL/actions/workflows/daily-modpack-build.yml
There was a problem hiding this comment.
I haven't looked into this mechanism yet, so I don't even know where to begin.
That's all I found.
adventurebackpack PlayerEventHandler (Normal)
backpack EventHandlerBackpack (Normal)
Baubles EventHandlerEntity (Normal)
Botania ItemKeepIvy (Normal?)
EnderIO EnchantmentSoulBound (Highest)
extrautilities EventHandlerServer (Normal)
GalactiCraft EventHandlerGC (Normal)
HEE VoidChestEvents (Lowest)
OpenBlocks PlayerDeathHandler (Low)
RandomThings RTEventHandler (Highest Charges the stone?)
TConstruct TPlayerHandler (Normal)
TConstruct TinkerToolEvents (Normal)
ThaumicHorizons EventHandlerEntity (Low)
witchery IHandlePlayerDrops (WitcheryPotions.EventHooks)
witchery PotionKeepInventory (IHandlePlayerDrops)
witchery WitcheryPotions (Highest)
witchery GenericEvents (High For vampire)
witchery RitePriorIncarnation (Normal)
WitchingGadgets EventHandler (Highest Soul seal?)
I'll continue tomorrow.
There was a problem hiding this comment.
It would be quite the PR farm to organize all these lol
thehoblit
left a comment
thehoblit
left a comment
There was a problem hiding this comment.
This is a good fix, I think the organization of death event priorities is a bigger change than just this PR
@Viptunbeqwfwew Are you still looking at other mods events for this, or are you happy with how it is now?
|
No, I'm not happy with this, but I won't be able to work on the fix for another month and a half. Yes, this is a major fix, as a lot needs to be redone. I'm focusing on the death protection mechanics and the inventory saving mechanics. And they're mixed together. For example, Witchery and TwilightForest save inventory in LivingDeathEvent. Meanwhile, ThaumicHorizons, before my fixes, handled item transfer in LivingHurtEvent. In general, I plan to use only the existing dependencies without adding new ones. I also need to solve the issue with the lack of a standard priority pool. I'm more than confident that 5 won't be enough. I can easily use my own solution https://github.com/Viptunbeqwfwew/DeterministicEvents, but I'm not sure it will work with the entire build. I'm fixing this for myself, as I play GTNH on hardcore (one life per world). It's crucial for me that this works correctly. |
4 participants
Fixed item drops when the Soul Beacon is active and item capture with the Mirror Amulet.
Please see the commit comment.