[1.17.1] Player capabilities are invalidated on death before they can be copied

[Danny and Son]

When a player dies, and I attempt to copy my custom capabilities data from the original player object to the new player object in the PlayerEvent.Clone event, the getOriginal().getCapability method is not giving me the capability. Upon further inspection in the Debugger, the capability is marked as invalid. Specifically, event.original.capabilities.caps[0].mycapabilityLazyOptional.isValid = false. This is unique to 1.17.1. In 1.16.5, with the same code isValid = true, and the data is copied as expected.

All other functionality seems to be working fine in 1.17.1. Data is being saved when a player leaves and rejoins, and can be found in their player data file NBT. It can be queried by other methods and classes while the player is alive. It is only on death that the capability data of the original player object can not be retrieved.

Do we need to take a different approach in 1.17.1 or is something not working as expected?

[Danny and Son]

Thanks! I think I'm not fully understanding how to implement this, though. Calling reviveCaps() on the original player object does not validate the capabilities. It looks like I need to call reviveCaps() on the provider, but I'm not sure how to do that in this context.

[Danny and Son]

Ah, ok. Good to know it's not just me being stupid. It looks like LivingEntity is overriding reviveCaps(). Perhaps it should be calling super.

[Danny and Son]

Oh right. It's the capability provider for Entity, not for attached capabilities.

[Danny and Son]

It appears players are losing capability data on dimension change as well (tested in 37.0.25 and  37.0.30).

[LexManos]

Yes this is intentional.
The system we had before is no longer valid for the way Mojang designed the code in 1.17.
Modders now have to explicitly revive and then invalidate their entities as designed in my commit.

[Danny and Son]

That doesn't seem to work unless I'm doing it incorrectly:

    @SubscribeEvent
    public void onPlayerDeath(PlayerEvent.Clone event)
    {
        if (event.getEntity() instanceof Player playerEntity) {

            event.getOriginal().reviveCaps();
            event.getOriginal().getCapability(CapabilityNutritionalBalancePlayer.HEALTHY_DIET_PLAYER_CAPABILITY).ifPresent(originalInutritionalbalancePlayer -> {

                playerEntity.getCapability(CapabilityNutritionalBalancePlayer.HEALTHY_DIET_PLAYER_CAPABILITY).ifPresent(newInutritionalbalancePlayer -> {


                  . . . .

                });

            });
            event.getOriginal().invalidateCaps();
        }

    }

After running event.getOriginal().reviveCaps(), the caps are still invalid, so the lambda code is never executed.

Similarly for dimension changes:

    @SubscribeEvent
    public void onDimensionChange (PlayerEvent.PlayerChangedDimensionEvent event){
        event.getEntity().reviveCaps();
    }

The caps are still invalid after executing this.

Am I doing something wrong?

[LexManos]

Then your provider is not returning a valid cap. Use your IDE's debugging tools to figure out why your code is not returning the value you think it is.

[Danny and Son]

My provider is returning a valid cap in every other circumstance though. Just not during cloning or dimension changes. Data is saved on leave and rejoin. GUI elements that query the caps are working as expected. Other events that are taking actions based on values within the caps are working.

In the debugger, I can see all the data in my cap in the 2 above scenarios. It is exactly what it's supposed to be, but
event.entity.capabilities.caps[0].nutritionalBalanceLazyOptional.isValid = false
and it remains so after calling reviveCaps().

Other than that, the debugger is showing that all the other data is exactly what it's supposed to be. What would cause isValid to be false?

[Danny and Son]

It might help to add that when I check the player object at any other point, such as when opening the mod's GUI, nutritionalBalanceLazyOptional.isValid = true. So, I'm guessing it's being invalidated by the death and dimension change, and reviveCaps() is not changing that. Based on my understanding of the commit you referenced above, reviveCaps() is supposed to change that to true. Right?

[Danny and Son]

Can someone confirm that Player.reviveCaps() does, indeed, restore capabilities attached to the player? This does not appear to be happening in my case. I have thoroughly debugged my code (otherwise I would not have opened this thread), and the only issue appears to be that LazyOptional.isValid becomes false on death and dimension change, and this is not changed to true when calling reviveCaps().

In short, reviveCaps is not doing what I think it's supposed to do. So, either it's not working correctly, or I am misunderstanding how it's supposed to work.

[Liahim]

Hello!
Tell me, did anyone find a solution to this problem?
I have the same.

[Danny and Son]

As far as I know, no solution has been found. I might need to open an issue on the MinecraftForge GitHub.

I have a couple questions for you just to make sure we're both dealing with the same issue:

Are your capabilities working when the player leaves and rejoins?
Have you also tried using reviveCaps() with no success?
Is your code posted publicly or can you share an excerpt of your PlayerEvent.Clone event?

[DaqEm]

The reviveCaps() worked for me. I'm using forge 37.0.67.

My Clone Event:

public static void onDeath(PlayerEvent.Clone event) {
    if (event.isWasDeath()) {
        event.getOriginal().reviveCaps();
        event.getOriginal().getCapability(ModCapabilityImpl.MOD_CAPABILITY).ifPresent(oldStore -> {
            event.getEntity().getCapability(ModCapabilityImpl.MOD_CAPABILITY).ifPresent(newStore -> {
               newStore.copyForRespawn((ModCapabilityImpl) oldStore);
            });
        });
        event.getOriginal().invalidateCaps();
    }
}

In my IModCapability:

void copyForRespawn(ModCapabilityImpl oldStore);

In my ModCapabilityImpl:

@Override
public void copyForRespawn(ModCapabilityImpl oldStore) {
	this.value = oldStore.value;
}

Hope this helps!

[Danny and Son]

On 10/6/2021 at 4:20 PM, DaqEm said:

The reviveCaps() worked for me. I'm using forge 37.0.67.

Interesting. Thanks for sharing that. I haven't tested it since 37.0.30. I ended up changing over to SaveData. It's much simpler to work with, gives me a lot more control, and I don't have to worry as much about it breaking every time Minecraft updates. The only drawback is that the data will persist even if player data files are deleted. They would have to delete the custom data file instead. I don't think that's a big issue, especially given that the tradeoff is that the data also persist through death, dimension changes or whatever without having to play any dirty tricks.

[Liahim]

On 9/21/2021 at 4:17 PM, Danny and Son said:

As far as I know, no solution has been found. I might need to open an issue on the MinecraftForge GitHub.

I have a couple questions for you just to make sure we're both dealing with the same issue:

Are your capabilities working when the player leaves and rejoins?
Have you also tried using reviveCaps() with no success?
Is your code posted publicly or can you share an excerpt of your PlayerEvent.Clone event?

The information is saved if I add listeners in AttachCapabilitiesEvent:

event.addCapability(key, provider);
event.addListener(provider.capOptional::invalidate);

However, I get NullPointerException when going from dimension to dimension at this line in EntityJoinWorldEvent.

player.getCapability(MistCaps.CAPABILITY_MIST).orElseThrow(() -> new NullPointerException("Player has no mist capability"));

If I do not add listeners, then everything works fine when changing the dimension, but the information is not saved when I restart the game.

Of course I have tried using reviveCaps(). No result.

[Liahim]

But how can I save information when restarting the game?

I use this example:

https://github.com/VazkiiMods/Botania/blob/0c1138252901ea646f6f97f9427f62ccd258e9d3/src/main/java/vazkii/botania/common/capability/SimpleCapProvider.java#L42

public class SimpleCapProvider<C extends INBTSerializable<CompoundTag>> implements ICapabilityProvider, INBTSerializable<CompoundTag> {

	private final C instance;
	private final LazyOptional<C> capOptional;
	private final Capability<C> capa;
	
	public SimpleCapProvider(Capability<C> capa, NonNullSupplier<C> instance) {
		this.capa = capa;
		this.instance = instance.get();
		this.capOptional = LazyOptional.of(instance);
	}

	@Nonnull
	@Override
	public <T> LazyOptional<T> getCapability(@Nonnull Capability<T> cap, @Nullable Direction side) {
		return capa.orEmpty(cap, capOptional);
	}

	@Override
	public CompoundTag serializeNBT() {
		return this.instance.serializeNBT();
	}

	@Override
	public void deserializeNBT(CompoundTag nbt) {
		this.instance.deserializeNBT(nbt);
	}

	public static <C extends INBTSerializable<CompoundTag>> void attach(AttachCapabilitiesEvent<?> event, ResourceLocation key, Capability<C> cap, NonNullSupplier<C> capInstance) {
		SimpleCapProvider<C> provider = new SimpleCapProvider<>(cap, capInstance);
		event.addCapability(key, provider);
		//event.addListener(provider.capOptional::invalidate); //TODO Capa invalidate
	}
}

And this is my capa registration

public class MistCaps {

	@CapabilityInject(IMistCapaHandler.class)
	public static Capability<IMistCapaHandler> CAPABILITY_MIST;

	@CapabilityInject(ISkillCapaHandler.class)
	public static Capability<ISkillCapaHandler> CAPABILITY_SKILL;

	@CapabilityInject(IFoodHandler.class)
	public static Capability<IFoodHandler> CAPABILITY_FOOD;

	private static final List<CapaEntry<? extends INBTSerializable<CompoundTag>>> capaList = Lists.newArrayList();

	public static void init(RegisterCapabilitiesEvent event) {
		register(event, "player_capa", IMistCapaHandler.class, () -> CAPABILITY_MIST, MistCapaHandler::new);
		register(event, "skill_capa", ISkillCapaHandler.class, () -> CAPABILITY_SKILL, SkillCapaHandler::new);
		register(event, "food_capa", IFoodHandler.class, () -> CAPABILITY_FOOD, FoodCapaHandler::new);
		Mist.LOGGER.info("Misty caps has been registered!");
	}

	//////////////////////////////////////////////////////// MAGIC ////////////////////////////////////////////////////////

	private static <C extends INBTSerializable<CompoundTag>> void register(RegisterCapabilitiesEvent event, String name, Class<C> clazz, NonNullSupplier<Capability<C>> capa, NonNullSupplier<C> instance) {
		event.register(clazz);
		capaList.add(new CapaEntry<C>(Mist.resLoc(name), capa, instance));
	}

	@SubscribeEvent
	public void attachCapabilitiesPlayer(AttachCapabilitiesEvent<Entity> event) {
		if (event.getObject() instanceof Player) capaList.forEach(entry -> attach(event, entry));
	}

	@SubscribeEvent
	public void cloneCapabilitiesEvent(PlayerEvent.Clone event) {
		capaList.forEach(entry -> clone(event, entry));
	}

	private static <C extends INBTSerializable<CompoundTag>> void attach(AttachCapabilitiesEvent<?> event, CapaEntry<C> entry) {
		SimpleCapProvider.attach(event, entry.res, entry.capa.get(), entry.instance);
		if (event.getCapabilities().get(entry.res) == null) {
			Mist.LOGGER.error("Player didn't attach [" + entry.capa.get().getName() + "] capa");
		} else Mist.LOGGER.info("Player has attached [" + entry.capa.get().getName() + "] capa");
	}

	private static <C extends INBTSerializable<CompoundTag>> void clone(PlayerEvent.Clone event, CapaEntry<C> entry) {
		try {
			//event.getOriginal().reviveCaps();
			C original = event.getOriginal().getCapability(entry.capa.get()).orElseThrow(NullPointerException::new);
			CompoundTag nbt = original.serializeNBT();
			C clone = event.getPlayer().getCapability(entry.capa.get()).orElseThrow(NullPointerException::new);
			clone.deserializeNBT(nbt);
			//event.getOriginal().invalidateCaps();
		} catch (Exception e) {
			Mist.LOGGER.error("Could not clone capability [" + entry.capa.get().getName() + "] when player [" + event.getOriginal().getName() + "] changing dimensions");
		}
	}

	private static class CapaEntry<C extends INBTSerializable<CompoundTag>> {

		private final ResourceLocation res;
		private final NonNullSupplier<C> instance;
		private final NonNullSupplier<Capability<C>> capa;
		
		public CapaEntry(ResourceLocation res, NonNullSupplier<Capability<C>> capa, NonNullSupplier<C> instance) {
			this.res = res;
			this.instance = instance;
			this.capa = capa;
		}
	}
}

 

Edited November 17, 2021 by Liahim

[Luis_ST]

1 hour ago, Liahim said:

But how can I save information when restarting the game?

you still save infos, in your SimpleCapProvider via the implementation of INBTSerializable (#serializeNBT and #deserializeNBT)

[Danny and Son]

Is your init function being called? It doesn't look like your subscribing to RegisterCapabilitiesEvent.

[Liahim]

8 hours ago, Luis_ST said:

you still save infos, in your SimpleCapProvider via the implementation of INBTSerializable (#serializeNBT and #deserializeNBT)

But capa didn't save during restarting the game.

7 hours ago, Danny and Son said:

Is your init function being called? It doesn't look like your subscribing to RegisterCapabilitiesEvent.

All events are subscribed in my main class. As I wrote in this message 

All methods work fine and information is saved when I use event.addListener(provider.capOptional::invalidate); 

But I am getting NullPointerException when going from dimension to dimension when trying to get a cap on the EntityJoinWorldEvent.

Edited November 18, 2021 by Liahim

[Liahim]

Yes, indeed. You're right.
But why can't I save the information? What am I doing wrong?

[Luis_ST]

23 hours ago, Liahim said:

		register(event, "player_capa", IMistCapaHandler.class, () -> CAPABILITY_MIST, MistCapaHandler::new);
		register(event, "skill_capa", ISkillCapaHandler.class, () -> CAPABILITY_SKILL, SkillCapaHandler::new);
		register(event, "food_capa", IFoodHandler.class, () -> CAPABILITY_FOOD, FoodCapaHandler::new);

show one of these CapaHandlers (the implementation not the interface)

[Liahim]

public class MistCapaHandler extends ItemStackHandler implements IMistCapaHandler {

	private Player player;
	private int pollution;
	private int toxic;

	public MistCapaHandler() {}

	// ...

	@Override
	public CompoundTag serializeNBT() {
		CompoundTag nbt = super.serializeNBT();
		nbt.putInt("Pollution", this.pollution);
		nbt.putInt("Toxic", this.toxic);
		return nbt;
	}

	@Override
	public void deserializeNBT(CompoundTag nbt) {
		super.deserializeNBT(nbt);
		this.pollution = nbt.getInt("Pollution");
		this.toxic = nbt.getInt("Toxic");
	}
 
    // ...
}

 

[Luis_ST]

looks okay, you could try to print the CompoundTag to the console to check whether it contains the data it should
respectively to check if the methods are called correctly

[Liahim]

I fixed it!
The point was that I worked with different instances of the cap in this place:

public SimpleCapProvider(Capability<C> capa, NonNullSupplier<C> instance) {
	this.capa = capa;
	this.instance = instance.get();
	this.capOptional = LazyOptional.of(instance); // <-----------
}

But this is correct:

public SimpleCapProvider(Capability<C> capa, NonNullSupplier<C> instance) {
	this.capa = capa;
	this.instance = instance.get();
	this.capOptional = LazyOptional.of(() -> this.instance); // <-----------
}