Add fix for handling removal of old structures. (#1100)

* Add fix for handling removal of old structures.

This fix is required or else worlds where a mod is removed WILL NOT SAVE.

* Only mark for saving if the chunk has a null key

* Add label for issue being fixed

* Actually fix the redirect lol

* Licenses...

* Use ThreadLocal boolean instead of nullable unit

* Checkstyle is picky

* remove unnecessary `containsKey`

fabric-structure-api-v1/src/main/java/net/fabricmc/fabric/mixin/structure/ChunkSerializerMixin.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.mixin.structure;
+
+import java.util.Map;
+
+import it.unimi.dsi.fastutil.longs.LongSet;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Unique;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Inject;
+import org.spongepowered.asm.mixin.injection.Redirect;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
+
+import net.minecraft.nbt.CompoundTag;
+import net.minecraft.util.math.ChunkPos;
+import net.minecraft.world.ChunkSerializer;
+import net.minecraft.world.chunk.Chunk;
+import net.minecraft.world.gen.feature.StructureFeature;
+
+// This is a bug fix, tracking issue: MC-194811
+@Mixin(ChunkSerializer.class)
+abstract class ChunkSerializerMixin {
+	@Unique
+	private static final ThreadLocal<Boolean> CHUNK_NEEDS_SAVING = ThreadLocal.withInitial(() -> false);
+
+	/**
+	 * Remove objects keyed by `null` in the map.
+	 * This data is likely bad since multiple missing structures will cause value mapped by `null` to change at least once.
+	 *
+	 * <p>If a null value is stored in this map, the chunk will fail to save, so we remove the value stored using null key.
+	 *
+	 * <p>Note that the chunk may continue to emit errors after being (un)loaded again.
+	 * This is because of the way minecraft handles chunk saving.
+	 * If the chunk is not modified, the game will keep the currently saved chunk on the disk.
+	 * In order to affect this change, we must mark the chunk to be save in order force the game to save the chunk without the errors.
+	 */
+	@Inject(method = "readStructureReferences", at = @At("TAIL"))
+	private static void removeNullKeys(ChunkPos pos, CompoundTag tag, CallbackInfoReturnable<Map<StructureFeature<?>, LongSet>> cir) {
+		if (cir.getReturnValue().remove(null) != null) {
+			ChunkSerializerMixin.CHUNK_NEEDS_SAVING.set(true);
+		}
+	}
+
+	@Redirect(method = "deserialize", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/chunk/Chunk;setStructureReferences(Ljava/util/Map;)V"))
+	private static void forceChunkSavingIfNullKeysExist(Chunk chunk, Map<StructureFeature<?>, LongSet> structureReferences) {
+		// Redirect is much cleaner than local capture. The local capture would be very long
+		if (ChunkSerializerMixin.CHUNK_NEEDS_SAVING.get()) {
+			ChunkSerializerMixin.CHUNK_NEEDS_SAVING.set(false);
+			// Make the chunk save as soon as possible
+			chunk.setShouldSave(true);
+		}
+
+		// Replicate vanilla logic
+		chunk.setStructureReferences(structureReferences);
+	}
+}

fabric-structure-api-v1/src/main/resources/fabric-structure-api-v1.mixins.json

@@ -4,6 +4,7 @@
   "compatibilityLevel": "JAVA_8",
   "mixins": [
     "BiomeAccessor",
+    "ChunkSerializerMixin",
     "FlatChunkGeneratorConfigAccessor",
     "MixinChunkGeneratorSettings",
     "MixinStructuresConfig",

fabric-structure-api-v1/src/testmod/java/net/fabricmc/fabric/test/structure/mixin/ChunkSerializerMixin.java

@@ -0,0 +1,40 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.test.structure.mixin;
+
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.injection.Constant;
+import org.spongepowered.asm.mixin.injection.ModifyConstant;
+
+import net.minecraft.nbt.CompoundTag;
+import net.minecraft.structure.StructureManager;
+import net.minecraft.world.ChunkSerializer;
+
+@Mixin(ChunkSerializer.class)
+abstract class ChunkSerializerMixin {
+	/**
+	 * @reason Changes the logging message for the `unknown structure start` to describe which chunk the missing structure is located in for debugging purposes.
+	 */
+	@ModifyConstant(method = "readStructureStarts", constant = @Constant(stringValue = "Unknown structure start: {}"))
+	private static String modifyErrorMessage(String original, StructureManager structureManager, CompoundTag tag, long worldSeed) {
+		// Use coordinates in tag to determine the position of the chunk
+		final int xPos = tag.getInt("xPos");
+		final int zPos = tag.getInt("zPos");
+
+		return String.format("Unknown structure start: {} at chunk [%s, %s]", xPos, zPos);
+	}
+}

fabric-structure-api-v1/src/testmod/resources/fabric-structure-api-v1-testmod.mixins.json

@@ -3,6 +3,7 @@
   "package": "net.fabricmc.fabric.test.structure.mixin",
   "compatibilityLevel": "JAVA_8",
   "mixins": [
+    "ChunkSerializerMixin",
     "MixinDefaultBiomeCreator"
   ],
   "client": [