WorldEdit Version

7.2.10-SNAPSHOT

Platform Version

git-Paper-235

Confirmations

• I am using the most recent Minecraft release.
• I am using a version of WorldEdit compatible with my Minecraft version.
• I am using the latest or recommended version of my platform software.
• I am NOT using a hybrid server, e.g. a server that combines Bukkit and Forge. Examples include Arclight, Mohist, and Cardboard.
• I am NOT using a fork of WorldEdit, such as FastAsyncWorldEdit (FAWE) or AsyncWorldEdit (AWE)

Bug Description

When copying/pasting/moving entities using the corresponding commands and the -e flag, the passengers of an entity are not saved nor loaded in the correct manner. Passengers of an entity are spawned not riding it and without any of the NBT data that they had when being stored. The handling of vehicles and passengers is incorrect at both the saving and loading state, both described below.

Saving
When converting entities to a BaseEntity, the com.sk89q.worldedit.bukkit.adapter.impl.v1_18_R1.PaperweightAdapter#getEntity uses the com.sk89q.worldedit.bukkit.adapter.impl.v1_18_R1.PaperweightAdapter#readEntityIntoTag, which calls the net.minecraft.world.entity.Entity#save method:

public boolean save(CompoundTag nbt) {
    return this.isPassenger() ? false : this.saveAsPassenger(nbt);
}

The usage of this method by itself is not incorrect, but the surrounding assumptions made when using it are wrong. This method goes on to call net.minecraft.world.entity.Entity#saveAsPassenger, which calls net.minecraft.world.entity.Entity#saveWithoutId. If the entity is a vehicle, this method saves all of the NBT data of its passengers into a ListTag Passengers.

if (this.isVehicle()) {
    nbttaglist = new ListTag();
    iterator = this.getPassengers().iterator();
    while (iterator.hasNext()) {
        Entity entity = (Entity) iterator.next();
        CompoundTag nbttagcompound1 = new CompoundTag();
        if (entity.saveAsPassenger(nbttagcompound1)) {
            nbttaglist.add(nbttagcompound1);
        }
    }
    if (!nbttaglist.isEmpty()) {
        nbt.put("Passengers", nbttaglist);
    }
}

This is why each individual passenger is not saved in the save method, as it is saved inside the NBT data of the vehicle it is riding. WorldEdit fails to account for this and attempts to save each entity individually, resulting in the passenger entities being saved without any corresponding NBT data. When accounting for the way how minecraft stores entities, worldguard should replicate the behaviour and never store entities for which net.minecraft.world.entity.Entity#isPassenger returns true, because those entities will be saved inside the vehicle entity they are riding.
Because of how the save method works, WorldEdit does in-fact save the correct passenger NBT data inside the vehicle, but it also duplicates the passenger entity types with missing NBT alongside the vehicle entity.

Now that we have established all the necessary data is actually being saved (and slightly duplicated) and not discarded, we can look at the loading part of the problem.

Loading
When loading entities, the above saved passengers from each vehicle are not loaded in. Only the blank entities without NBT, that were redundantly saved are spawned.

The class at com.sk89q.worldedit.bukkit.adapter.impl.v1_18_R1.PaperweightAdapter#createEntity uses the com.sk89q.worldedit.bukkit.adapter.impl.v1_18_R1.PaperweightAdapter#createEntityFromId method to first create an entity, then proceeds to load in the stored NBT TagCompound via the com.sk89q.worldedit.bukkit.adapter.impl.v1_18_R1.PaperweightAdapter#readTagIntoEntity which uses the net.minecraft.world.entity.Entity#load method. This works for non-vehicle entities, but when looking at the load method, it does not account or load passengers from the Passengers ListTag. Instead of manually spawning the entity then loading the NBT data, WorldEdit should spawn the stored entity using net.minecraft.world.entity.EntityType#loadEntityRecursive, which iterates through the passengers of the spawning entity, spawns them and sets them as riding the vehicle entity.

@Nullable
public static Entity loadEntityRecursive(CompoundTag nbt, Level world, Function<Entity, Entity> entityProcessor) {
    return (Entity) EntityType.loadStaticEntity(nbt, world).map(entityProcessor).map((entity) -> {
        if (nbt.contains("Passengers", 9)) {
            ListTag nbttaglist = nbt.getList("Passengers", 10);
            for (int i = 0; i < nbttaglist.size(); ++i) {
                Entity entity1 = EntityType.loadEntityRecursive(nbttaglist.getCompound(i), world, entityProcessor);
                if (entity1 != null) {
                    entity1.startRiding(entity, true);
                }
            }
        }
        return entity;
    }).orElse(null); // CraftBukkit - decompile error
}

An example of such usage is the Minecraft summon command net.minecraft.server.commands.SummonCommand#spawnEntity, which appends the entity id String to the CompoundTag, which the static method needs.

In conclusion, WorldEdit should not store passenger entities, as they are already stored in the vehicle NBT data, and should instead simply load those entities via the available methods for doing so, which correctly load in passengers.

Expected Behavior

WorldEdit should not store passenger entities, as they are already stored in the vehicle NBT data, and should instead simply load those entities via the available methods for doing so, which correctly load in passengers.

Reproduction Steps

1. Spawn any jockey using a command like /summon chicken ~ ~ ~ {CustomName:"\"Vehicle\"",CustomNameVisible:true,Passengers:[{id:chicken,CustomName:"\"Passenger\""}]}
2. Select a region around it.
3. //copy -e
4. //paste -e
5. Notice that Passenger chicken is not riding the Vehicle chicken, and that its NBT has not been preserved, visible by the missing custom name.

Anything Else?

I would open a pull request for this with the mentioned changes, but I am not familiar enough with the rest of the WorldEdit code to be comfortable doing that, so I did my best to provide the clear description of the underlying mechanics and a clear and simple solution that should be easily implementable, by the looks of it.

I have not tested this or explored it on any other platform than Paper, but it is possible the same mistake was made on them and should be also checked accordingly.

duplicate of #1763 tho the analysis here is useful

I missed the first report somehow, my bad. But I did a few hours of digging around and I am 99% sure this is the underlying issue.