Minecraftforge: [1.13.x] Entities moving between dimensions lose all their caps

[williewillus]

Teleporting between dimensions calls world.removeEntityDangerously, which calls Entity.remove (formerly setDead), which invalidates all caps.

Vanilla then sets removed (isDead) back to false immediately after, the same entity is then moved to the destination world, and then continues without any caps at all.

This happens in quite a few places in vanilla code when teleporting is involved.

It also happens when returning from the End, since end return is a special case of dying + respawning. PlayerCloneEvent is unable to help here since by the time it fires the original entity is already dead so its caps are gone.

Not sure how to resolve this, there needs to be a way to "revive" caps or otherwise reinitialize them, or not invalidate them when this occurs. First one seems hacky, second one requires lots of patches everywhere vanilla does this

[DreenDex]

Moving caps across world can be done by PlayerEvent.Clone event when event.isWasDeath() is false. From there you have access to old player getOriginal() and to a new player getEntityPlayer(). Than you can move data from old player's capability to new player's capability. Most likely you will have serialization so you can use it.
In result you need to do something like this:

@SubscribeEvent
public void onPlayerClone(PlayerEvent.Clone event) {
    if (!event.isWasDeath()) {
        MyCapability playerCap = getCapFromPlayer(event.getEntityPlayer());
        MyCapability oldPlayerCap = getCapFromPlayer(event.getOriginal());
        playerCap.deserializeNBT(oldPlayerCap.serializeNBT());
    }
}

More about capabilities you can find in forge docs.

[williewillus]

no, that does not apply in this case.

player entities are not cloned when they move between dimensions normally, such as with a nether portal.

the only time Clone event is fired is 1. respawn from death and 2. returning from end (which is a special case on top of death respawn).

[LexManos]

We would need to find as many cases as we can where this odd mechanic is there.
And also probably ass a 'resuscitated' function to entitites for this 'setDead(true) setDead(false)' chain.
All of the wrappers we add to vanilla things can easily be resuscitated, as they are usually just wrappers around vanilla data.

[williewillus]

yeah wrappers are easy to revive, but this also calls CapabilityDispatcher.invalidate which invalidates all mod caps and not sure how that would work

[williewillus]

I'm pretty busy in the coming weeks so if someone wants to give this a stab go ahead

[williewillus]

Any fix should be tested for all the following cases:

1. Death and respawn (caps should be visible in Clone Event)
2. End return (caps should be visible in Clone Event)
3. Normal interdim teleporting e.g. nether (caps should be exact same before and after teleport)

[LexManos]

So looking at it we have a few code paths:

1) Teleportation. They remove the entity {invalidate the caps} then create a new entity and copy the data over. Which would copy the caps if they wernt invalid. This solution is most likely just to either delay the removing from the source world until after we copy the data, or add a second 'removeForTransfer' function that doesn't call invalidate.

2) Nether/End Portals, and Forge's SetDim command. All drill down to Entity.changeDimension() Again, duplicates the entity using copyDataFromOld, which is invalid because it called world.remove(this) before hand. So solution to this would be to either change the remove invocation, or add the 'removeForTransfer' function.

3) Player.teleport()/PlayerList.changePlayerDimension(). Actually DOES keep the same instance, and resuscitates it in the new world. however calls world.removeEntityDangerously(player) which in turn calls entity.remove(). Solution: Add a 'removeEntityDangerouslyForTransfer' function.

Those are the only instances where the removed flag is changed to false. 1&2 are simple changes, 3 needs is a bit more invasive.

[LexManos]

Not a super fan of that implementation, as it feels hacky, and has an issue with revive() not reviving caps. But honestly, I think entities should be copied not moved. Players are the only exception as it's to invasive to re-write that.

[LexManos]

So people stop complaing about it, Yes I know that when an entity dies and onDeathUpdate is called, it calls remove on itself. Thus invalidating all caps.
AS IT SHOULD.
We need to have a better less crappy system for copying data.
Feel free to discuss ideas.

Looking into it more, your Caps are serialized to NBT, then the NBT is read/deserialized by the copy function.
So you guys should never need to access Caps in the Clone event.
The only potential issue is the code paths where copy is NOT called, and the only one I can find is Player.teleport/PlayerList.changePlayerDimension, which is fine as it DOESN'T call the raw remove().

[sargunv]

Looking into it more, your Caps are serialized to NBT, then the NBT is read/deserialized by the copy function.

I see what you say is being done in Entity.copyDataFromOld:

   // net.minecraft.Entity
   public void copyDataFromOld(Entity entityIn) {
      NBTTagCompound nbttagcompound = entityIn.writeWithoutTypeId(new NBTTagCompound());
      nbttagcompound.remove("Dimension");
      this.read(nbttagcompound);
      /* snip */
   }

However, PlayerList.recreatePlayerEntity does not call Entity.copyDataFromOld; it calls EntityPlayerMP.copyFrom. I see that PlayerList.recreatePlayerEntity is the code that handles respawning the player after death or returning them from the end (see NetHandlerPlayServer.processClientStatus), and it eventually fires the PlayerEvent.Clone event (through EntityPlayerMP.copyFrom).

In copyFrom, I only see the data under PERSISTED_NBT_TAG being copied, not the entire NBT data:

   // net.minecraft.entity.player.EntityPlayerMP
   public void copyFrom(EntityPlayerMP that, boolean keepEverything) {
      /* snip */
      //Copy over a section of the Entity Data from the old player.
      //Allows mods to specify data that persists after players respawn.
      NBTTagCompound old = that.getEntityData();
      if (old.contains(PERSISTED_NBT_TAG))
          getEntityData().put(PERSISTED_NBT_TAG, old.get(PERSISTED_NBT_TAG));
      /* snip */
   }

Since serialized capabilities are not stored in entityData, they're not copied when a player is cloned.

One possible solution to this problem would be to store the data that needs to be persisted into the player's entityData under key PERSISTED_NBT_TAG (and then under the capability's unique key below that), and pull it back into the new player's capability on PlayerEvent.Clone.

See my repo for an example of the above solution, which seems to be working well. I'd love to hear about any potential pitfalls with this approach.

[williewillus]

So you guys should never need to access Caps in the Clone event.

But the point of caps is that forge handles all serialization for us. Modders should not have to dig around in the ForgeCaps tag manually to find their cap data in the Clone event, since that would be really brittle to any format changes forge does. It should be available at the provider level of abstraction as it has been for years.

Anyways, from looking on discord looks like dimension changes work now, but death and end return are still not.

[RobertSkalko]

Hopefully this will be fixed in 1.14 at least because otherwise my mod won't even work. I used capabilties to save player levels which should stay through dying and teleporting (which worked fine in 1.12.2).