Cry of the Eternal Soul Ritual non-functional

[sam-kirby]

Issue Description:

The Cry of the Eternal Soul ritual does not function.

Further, since its structure was changed in the refactor, the ritual diviner lists it as requiring more ritual stones than it actually uses. It currently consumes 73 ritual stones, though the diviner still lists the old requirement of 76. This is due to it requiring 3 fewer fire runes. It is possible this is due to the order of arguments on this line being wrong, with offset and y reversed:
https://github.com/WayofTime/BloodMagic/blob/1.12/src/main/java/WayofTime/bloodmagic/ritual/types/RitualEternalSoul.java#L89

What happens:

The cry of the eternal soul ritual activates when right clicked with an activation crystal, but does not do anything further. The nearby blood altar is not filled.

Structure is changed from previous versions, count of ritual stones is not accurately reflected in the diviner.

What you expected to happen:

The nearby blood altar to be filled.

Ritual diviner to accurately specify requirements or structure to be the same as previous versions.

Steps to reproduce:

1. /bloodmagic ritual create eteranal_soul just below a blood altar
2. Activate with LP in network - "Rush of energy..."
3. Observe lack of function.

---

Affected Versions (Do not use "latest"):

• BloodMagic: 2.4.1-103
• Minecraft: 1.12.2
• Forge: 14.23.5.2838

Further details

The following line can never evaluate to false, thus the method is always terminated early:
https://github.com/WayofTime/BloodMagic/blob/1.12/src/main/java/WayofTime/bloodmagic/ritual/types/RitualEternalSoul.java#L48

Should be obtaining the BloodAltar from the tile's capability and using that.

I attach below a working fix, but as I am fairly unfamiliar at this point with Forge's capabilities I have not submitted a PR as it can likely be improved upon.

diff --git a/src/main/java/WayofTime/bloodmagic/ritual/types/RitualEternalSoul.java b/src/main/java/WayofTime/bloodmagic/ritual/types/RitualEternalSoul.java
index 01ff6e08..07bc735e 100644
--- a/src/main/java/WayofTime/bloodmagic/ritual/types/RitualEternalSoul.java
+++ b/src/main/java/WayofTime/bloodmagic/ritual/types/RitualEternalSoul.java
@@ -1,6 +1,7 @@
 package WayofTime.bloodmagic.ritual.types;
 
 import WayofTime.bloodmagic.BloodMagic;
+import WayofTime.bloodmagic.altar.BloodAltar;
 import WayofTime.bloodmagic.altar.IBloodAltar;
 import WayofTime.bloodmagic.block.BlockLifeEssence;
 import WayofTime.bloodmagic.ritual.*;
@@ -11,6 +12,7 @@ import net.minecraft.util.math.AxisAlignedBB;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.world.World;
 import net.minecraftforge.fluids.FluidStack;
+import net.minecraftforge.fluids.capability.CapabilityFluidHandler;
 import net.minecraftforge.fluids.capability.IFluidHandler;
 
 import java.util.List;
@@ -21,7 +23,7 @@ import java.util.function.Consumer;
 public class RitualEternalSoul extends Ritual {
 
 
-    private IBloodAltar altar = null;
+    private BloodAltar altar = null;
 
     public RitualEternalSoul() {
         super("ritualEternalSoul", 2, 2000000, "ritual." + BloodMagic.MODID + ".eternalSoulRitual");
@@ -39,7 +41,7 @@ public class RitualEternalSoul extends Ritual {
                 for (int j = -5; j <= 5; j++) {
                     for (int k = -10; k <= 10; k++) {
                         if (world.getTileEntity(new BlockPos(pos.getX() + i, pos.getY() + j, pos.getZ() + k)) instanceof IBloodAltar) {
-                            this.altar = (IBloodAltar) world.getTileEntity(new BlockPos(pos.getX() + i, pos.getY() + j, pos.getZ() + k));
+                            this.altar = (BloodAltar) world.getTileEntity(new BlockPos(pos.getX() + i, pos.getY() + j, pos.getZ() + k)).getCapability(CapabilityFluidHandler.FLUID_HANDLER_CAPABILITY, null);
                         }
                     }
                 }
@@ -62,9 +64,9 @@ public class RitualEternalSoul extends Ritual {
                 entityOwner = player;
         }
 
-        int fillAmount = Math.min(currentEssence / 2, ((IFluidHandler) this.altar).fill(new FluidStack(BlockLifeEssence.getLifeEssence(), 10000), false));
+        int fillAmount = Math.min(currentEssence / 2, this.altar.fill(new FluidStack(BlockLifeEssence.getLifeEssence(), 10000), false));
 
-        ((IFluidHandler) this.altar).fill(new FluidStack(BlockLifeEssence.getLifeEssence(), fillAmount), true);
+        this.altar.fill(new FluidStack(BlockLifeEssence.getLifeEssence(), fillAmount), true);
 
         if (entityOwner != null && entityOwner.getHealth() > 2.0f && fillAmount != 0)
             entityOwner.setHealth(2.0f);
@@ -86,7 +88,7 @@ public class RitualEternalSoul extends Ritual {
 
     @Override
     public void gatherComponents(Consumer<RitualComponent> components) {
-        addCornerRunes(components, 0, 1, EnumRuneType.FIRE);
+        addCornerRunes(components, 1, 0, EnumRuneType.FIRE);
 
         for (int i = 0; i < 4; i++) {
             addCornerRunes(components, 2, i, EnumRuneType.AIR);

[keraldi]

Huh? The rune layout should be exactly the same. Furthermore, the diviner tooltip is incapable of showing a different amount of runes than are used in the ritual because the tooltip is built from the component description in the ritual class. I'll take a look at it.

[sam-kirby]

If addCornerRunes puts 4 runes in the same place that could explain it no?

[keraldi]

addCornerRunes adds 4 runes in each |x|,|x| offset.
I'll take a look at it later but if your PR keeps the original layout and works, I don't see a reason why yours shouldn't be merged.

[sam-kirby]

Above is the original layout. Central fire runes added by addCornerRunes(offset: 1, y: 0)

Above is current layout. Fire rune on top is specified by addCornerRunes(offset: 0, y: 1). 4 in the same position due to zero offset.

[keraldi]

Obvious mistake on my part then, thanks for finding and fixing it!

[sam-kirby]

No worries, more concerned by the ritual not working at all no matter which structure though! Could you have a look at the patch I posted above (can turn it into a PR if that's easier for you) and let me know if I'm doing anything silly with capabilities? It's not something I'm overly familiar with.

[keraldi]

I'm busy right now, I'll look at it in a bit.

[keraldi]

I'll change a couple things, I seem to have requested a merge before finishing it (obviously, as it is not working), it's not using a couple mechanics that are new in BM2 to deal with areas and altar locations.

[sam-kirby]

I think I discovered this for myself, see #1633