neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.2k stars 177 forks source link

Fake Player are able to mount entities, afterwards causing them to despawn when server stops. #1320

Closed MayaqqDev closed 3 months ago

MayaqqDev commented 3 months ago

Minecraft Version: 1.20.1 and up (and lower mayhaps)

NeoForge Version: 47.1.106

Logs: https://mclo.gs/f9BSXNw not needed, but here anyway.

Steps to Reproduce:

  1. Setup a fake player with the right click action (In this example it is provided by the create mod deployer)
  2. Spawn a ride-able entity it is right clicking and let it mount the said entity
  3. Disconnect from the world
  4. Join back and observe the rid-able entity no longer existing

Description of issue: The Fake Player mounts the entity, which by itself shouldn't be a thing, as it functions in pretty unintended ways, but it also does mean that when the server stops, and the Fake Player "disconnects", it casues the entity to go with it, and because the fake player never "reconnects", it causes the mount to be basically deleted.

I managed to solve this issue by using the "EntityMountEvent", where I cancelled it when a FakePlayer is passed into it.

    @SubscribeEvent
    public static void onEntityMount(EntityMountEvent event) {
        if (event.getEntity() instanceof FakePlayer) {
            event.setCanceled(true);
        }
    }
IThundxr commented 3 months ago

Ideally the Entity patch is modified so in startRiding it returns false if this instanceof FakePlayer

KnightMiner commented 3 months ago

With the amount of places that do such instanceof checks, would it be worth just patching in a isFakePlayer boolean getter into the player class? have it return false by default, true in the FakePlayer override. The virtual function table lookup should be way faster than an inheritance check (not that this case is performance sensitive code, but other cases might be and it just reads nicer). Plus, it would allow people to make their own Player subclass be fake should the need arise (not sure that has a usecase but its just better API design to support)

IThundxr commented 3 months ago

With the amount of places that do such instanceof checks, would it be worth just patching in a isFakePlayer boolean getter into the player class? have it return false by default, true in the FakePlayer override. The virtual function table lookup should be way faster than an inheritance check (not that this case is performance sensitive code, but other cases might be and it just reads nicer)

That would be a good idea yeah, would be nicer to read as well