[MC-114030] Structure blocks take and apply NBT data of entities and tile entities directly instead of a copy when loading and saving structures

The bug

Structure blocks do not make a copy of the NBT data of an entity or tile entity when they save it in a structure or when they load it from the structure. This results in multiple bugs:

Loading a structure multiple times results in multiple entities or tile entities "sharing" the same NBT data
Modifying an entity or tile entity which was saved as part of a structure might modify the NBT data of the respective entity or tile entity in the structure data as well
Loading a structure with entities and tile entities and modifying them might modify the NBT data of the entity or tile entity which was saved as part of the structure

How to reproduce

Place a structure block

Place a mob spawner on top of it and save it as structure

/setblock ~ ~ ~ mob_spawner default replace {SpawnData:{id:"armor_stand"},SpawnCount:0s,SpawnRange:0s,MinSpawnDelay:0s,MaxSpawnDelay:0s,MaxNearbyEntities:1s,RequiredPlayerRange:16s,SpawnPotentials:[{Weight:1,Entity:{id:"bat"}}]}

Load the structure at multiple positions in the world
Right click one of the loaded spawners with for example a creeper spawn egg
Go to the other spawners and check their NBT data (they won't update visually)
```
/blockdata ~ ~-1 ~ {}
```
→ You will notice that id tag in the SpawnData tag has now the value "minecraft:creeper" (= case 1)
Load the structure again
→ Even the newly loaded mob spawners will spawn creepers despite the fact that you did not overwrite the structure (= case 2)
Set the time to night and use /blockdata on one of the loaded spawners to make it spawn the SpawnData entity and switch to the SpawnPotentials entry
```
/time set 14000
```
```
/blockdata ~ ~-1 ~ {SpawnCount:1b}
```
Right click the spawner with for example a chicken spawn egg
Use /blockdata on the spawner you saved in the structure
```
/blockdata ~ ~-1 ~ {}
```
→ You will notice that its SpawnPotentials entry changed as well. Its value is now "minecraft:chicken" instead of "minecraft:bat" (= case 3)

Code analysis

Based on 1.11.2 decompiled using MCP 9.35 rc1

The following methods do not make a copy of the NBT data taken from the world

net.minecraft.world.gen.structure.template.Template.takeEntitiesFromWorld(World, BlockPos, BlockPos)
net.minecraft.world.gen.structure.template.Template.takeBlocksFromWorld(World, BlockPos, BlockPos, boolean, Block)

The following methods do not make a copy when loading entities respectively tile entities in the world

net.minecraft.world.gen.structure.template.Template.addEntitiesToWorld(World, BlockPos, Mirror, Rotation, StructureBoundingBox)
net.minecraft.world.gen.structure.template.Template.addBlocksToWorld(World, BlockPos, ITemplateProcessor, PlacementSettings, int)

Comments 3

marcono1234 2017-02-20T19:59:00Z

The problem seems to be that in general structure blocks apply the NBT data stored in the structure file without creating a copy. This means all entities and tile entities created by a structure block basically "share" the same NBT data.

I am going to rewrite the description a little bit, I hope you are fine with that.

Daniel Burnett 2017-02-21T03:17:09Z

@Marcono1234 Sure, that's fine; but I'm having trouble locating that specific code you mentioned. Perhaps Forge has already fixed this bug in its source, but looking through the 1.11.2 source that Forge provides, the takeBlocksFromWorld() method does seem to create a new NBT tag, which it copies the tile entity data to, and then adds that copy of the NBT to a new BlockInfo instance, which is in turn added to the template's block list. So where exactly is it failing to make a copy? (Or, again, perhaps this is something that Forge's developers already fixed and just failed to comment on?)

marcono1234 2017-02-21T12:14:56Z

It is true that it writes the NBT data to a new NBTTagCompound, but the method overriding net.minecraft.tileentity.TileEntity.writeToNBT(NBTTagCompound) do not necessarily copy the NBT data they add to this compound and they probably should not do it anyways because that would only slow down saving a world.

This can for example be seen with banners (net.minecraft.tileentity.TileEntityBanner.writeToNBT(NBTTagCompound)) as they directly set their patterns NBTTagList.