Skip to content

Commit

Permalink
fix: improve biome setting to avoid writing directly to chunk
Browse files Browse the repository at this point in the history 
 - Removes possibility of writing to the LevelChunkSection biomes PalettedContainer whilst it is being read for sending packets
 - I believe this occured mostly on clipboard operations where blocks are written before biomes, so chunks are being sent whilst writing biomes
 - This would explain why the error reported in the below issue (and others) is/was so rare
 - Of course I could be completely wrong about all of this, but given the line in LevelChunkSection#write that the error seems to consistently occur on is when writing biomes to the packet, and that the only place I can find in FAWE where we write to a "live" PalettedContainer is for biomes, I am reasonably confident that this is the cause
 - Should address #2729
  • Loading branch information
@dordsor21
dordsor21 committed Jun 2, 2024
1 parent 7635eec commit 362e946
Show file tree
Hide file tree
Showing 10 changed files with 287 additions and 164 deletions.
49 changes: 24 additions & 25 deletions ...main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_19_R3/PaperweightGetBlocks.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,14 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
}
}
} else {
setBiomesToPalettedContainer(biomes, setSectionIndex, existingSection.getBiomes());
PalettedContainer<Holder<Biome>> paletteBiomes = setBiomesToPalettedContainer(
biomes,
setSectionIndex,
existingSection.getBiomes()
);
if (paletteBiomes != null) {
PaperweightPlatformAdapter.setBiomesToChunkSection(existingSection, paletteBiomes);
}
}
}
}
Expand Down Expand Up @@ -553,11 +560,7 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
if (existingSection == null) {
PalettedContainer<Holder<Biome>> biomeData = biomes == null ? new PalettedContainer<>(
biomeHolderIdMap,
biomeHolderIdMap.byIdOrThrow(WorldEditPlugin
.getInstance()
.getBukkitImplAdapter()
.getInternalBiomeId(
BiomeTypes.PLAINS)),
biomeHolderIdMap.byIdOrThrow(adapter.getInternalBiomeId(BiomeTypes.PLAINS)),
PalettedContainer.Strategy.SECTION_BIOMES,
null
) : PaperweightPlatformAdapter.getBiomePalettedContainer(biomes[setSectionIndex], biomeHolderIdMap);
Expand Down Expand Up @@ -625,15 +628,14 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
existingSection.getBiomes()
);

newSection =
PaperweightPlatformAdapter.newChunkSection(
layerNo,
this::loadPrivately,
setArr,
adapter,
biomeRegistry,
biomeData
);
newSection = PaperweightPlatformAdapter.newChunkSection(
layerNo,
this::loadPrivately,
setArr,
adapter,
biomeRegistry,
biomeData != null ? biomeData : (PalettedContainer<Holder<Biome>>) existingSection.getBiomes()
);
if (!PaperweightPlatformAdapter.setSectionAtomic(
levelChunkSections,
existingSection,
Expand Down Expand Up @@ -845,7 +847,7 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
}
if (callback == null) {
if (finalizer != null) {
finalizer.run();
queueHandler.async(finalizer, null);
}
return null;
} else {
Expand Down Expand Up @@ -1103,9 +1105,13 @@ private PalettedContainer<Holder<Biome>> setBiomesToPalettedContainer(
final int sectionIndex,
final PalettedContainerRO<Holder<Biome>> data
) {
BiomeType[] sectionBiomes;
if (biomes == null || (sectionBiomes = biomes[sectionIndex]) == null) {
return null;
}
PalettedContainer<Holder<Biome>> biomeData;
if (data instanceof PalettedContainer<Holder<Biome>> palettedContainer) {
biomeData = palettedContainer;
biomeData = palettedContainer.copy();
} else {
LOGGER.warn(
"Cannot correctly set biomes to world, existing biomes may be lost. Expected class " +
Expand All @@ -1115,10 +1121,6 @@ private PalettedContainer<Holder<Biome>> setBiomesToPalettedContainer(
);
biomeData = data.recreate();
}
BiomeType[] sectionBiomes;
if (biomes == null || (sectionBiomes = biomes[sectionIndex]) == null) {
return biomeData;
}
for (int y = 0, index = 0; y < 4; y++) {
for (int z = 0; z < 4; z++) {
for (int x = 0; x < 4; x++, index++) {
Expand All @@ -1130,10 +1132,7 @@ private PalettedContainer<Holder<Biome>> setBiomesToPalettedContainer(
x,
y,
z,
biomeHolderIdMap.byIdOrThrow(WorldEditPlugin
.getInstance()
.getBukkitImplAdapter()
.getInternalBiomeId(biomeType))
biomeHolderIdMap.byIdOrThrow(adapter.getInternalBiomeId(biomeType))
);
}
}
Expand Down
21 changes: 18 additions & 3 deletions ...ava/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_19_R3/PaperweightPlatformAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public final class PaperweightPlatformAdapter extends NMSAdapter {

private static final Field fieldTickingFluidCount;
private static final Field fieldTickingBlockCount;
private static final Field fieldNonEmptyBlockCount;
private static final Field fieldBiomes;

private static final MethodHandle methodGetVisibleChunk;

Expand Down Expand Up @@ -138,8 +138,15 @@ public final class PaperweightPlatformAdapter extends NMSAdapter {
fieldTickingFluidCount.setAccessible(true);
fieldTickingBlockCount = LevelChunkSection.class.getDeclaredField(Refraction.pickName("tickingBlockCount", "g"));
fieldTickingBlockCount.setAccessible(true);
fieldNonEmptyBlockCount = LevelChunkSection.class.getDeclaredField(Refraction.pickName("nonEmptyBlockCount", "f"));
fieldNonEmptyBlockCount.setAccessible(true);
Field tmpFieldBiomes;
try {
// It seems to actually be biomes, but is apparently obfuscated to "j"
tmpFieldBiomes = LevelChunkSection.class.getDeclaredField("biomes");
} catch (NoSuchFieldException ignored) {
tmpFieldBiomes = LevelChunkSection.class.getDeclaredField("j");
}
fieldBiomes = tmpFieldBiomes;
fieldBiomes.setAccessible(true);

Method getVisibleChunkIfPresent = ChunkMap.class.getDeclaredMethod(Refraction.pickName(
"getVisibleChunkIfPresent",
Expand Down Expand Up @@ -497,6 +504,14 @@ private static LevelChunkSection newChunkSection(
return new LevelChunkSection(layer, dataPaletteBlocks, biomes);
}

public static void setBiomesToChunkSection(LevelChunkSection section, PalettedContainer<Holder<Biome>> biomes) {
try {
fieldBiomes.set(section, biomes);
} catch (IllegalAccessException e) {
LOGGER.error("Could not set biomes to chunk section", e);
}
}

/**
* Create a new {@link PalettedContainer<Biome>}. Should only be used if no biome container existed beforehand.
*/
Expand Down
49 changes: 24 additions & 25 deletions ...main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R1/PaperweightGetBlocks.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,14 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
}
}
} else {
setBiomesToPalettedContainer(biomes, setSectionIndex, existingSection.getBiomes());
PalettedContainer<Holder<Biome>> paletteBiomes = setBiomesToPalettedContainer(
biomes,
setSectionIndex,
existingSection.getBiomes()
);
if (paletteBiomes != null) {
PaperweightPlatformAdapter.setBiomesToChunkSection(existingSection, paletteBiomes);
}
}
}
}
Expand Down Expand Up @@ -552,11 +559,7 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
if (existingSection == null) {
PalettedContainer<Holder<Biome>> biomeData = biomes == null ? new PalettedContainer<>(
biomeHolderIdMap,
biomeHolderIdMap.byIdOrThrow(WorldEditPlugin
.getInstance()
.getBukkitImplAdapter()
.getInternalBiomeId(
BiomeTypes.PLAINS)),
biomeHolderIdMap.byIdOrThrow(adapter.getInternalBiomeId(BiomeTypes.PLAINS)),
PalettedContainer.Strategy.SECTION_BIOMES
) : PaperweightPlatformAdapter.getBiomePalettedContainer(biomes[setSectionIndex], biomeHolderIdMap);
newSection = PaperweightPlatformAdapter.newChunkSection(
Expand Down Expand Up @@ -623,15 +626,14 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
existingSection.getBiomes()
);

newSection =
PaperweightPlatformAdapter.newChunkSection(
layerNo,
this::loadPrivately,
setArr,
adapter,
biomeRegistry,
biomeData
);
newSection = PaperweightPlatformAdapter.newChunkSection(
layerNo,
this::loadPrivately,
setArr,
adapter,
biomeRegistry,
biomeData != null ? biomeData : (PalettedContainer<Holder<Biome>>) existingSection.getBiomes()
);
if (!PaperweightPlatformAdapter.setSectionAtomic(
levelChunkSections,
existingSection,
Expand Down Expand Up @@ -843,7 +845,7 @@ public synchronized <T extends Future<T>> T call(IChunkSet set, Runnable finaliz
}
if (callback == null) {
if (finalizer != null) {
finalizer.run();
queueHandler.async(finalizer, null);
}
return null;
} else {
Expand Down Expand Up @@ -1100,9 +1102,13 @@ private PalettedContainer<Holder<Biome>> setBiomesToPalettedContainer(
final int sectionIndex,
final PalettedContainerRO<Holder<Biome>> data
) {
BiomeType[] sectionBiomes;
if (biomes == null || (sectionBiomes = biomes[sectionIndex]) == null) {
return null;
}
PalettedContainer<Holder<Biome>> biomeData;
if (data instanceof PalettedContainer<Holder<Biome>> palettedContainer) {
biomeData = palettedContainer;
biomeData = palettedContainer.copy();
} else {
LOGGER.warn(
"Cannot correctly set biomes to world, existing biomes may be lost. Expected class " +
Expand All @@ -1112,10 +1118,6 @@ private PalettedContainer<Holder<Biome>> setBiomesToPalettedContainer(
);
biomeData = data.recreate();
}
BiomeType[] sectionBiomes;
if (biomes == null || (sectionBiomes = biomes[sectionIndex]) == null) {
return biomeData;
}
for (int y = 0, index = 0; y < 4; y++) {
for (int z = 0; z < 4; z++) {
for (int x = 0; x < 4; x++, index++) {
Expand All @@ -1127,10 +1129,7 @@ private PalettedContainer<Holder<Biome>> setBiomesToPalettedContainer(
x,
y,
z,
biomeHolderIdMap.byIdOrThrow(WorldEditPlugin
.getInstance()
.getBukkitImplAdapter()
.getInternalBiomeId(biomeType))
biomeHolderIdMap.byIdOrThrow(adapter.getInternalBiomeId(biomeType))
);
}
}
Expand Down
25 changes: 19 additions & 6 deletions ...ava/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R1/PaperweightPlatformAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
import org.apache.logging.log4j.Logger;
import org.bukkit.Bukkit;
import org.bukkit.craftbukkit.v1_20_R1.CraftChunk;
import sun.misc.Unsafe;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -101,7 +100,6 @@ public final class PaperweightPlatformAdapter extends NMSAdapter {

private static final Field fieldTickingFluidCount;
private static final Field fieldTickingBlockCount;
private static final Field fieldNonEmptyBlockCount;

private static final MethodHandle methodGetVisibleChunk;

Expand All @@ -110,6 +108,7 @@ public final class PaperweightPlatformAdapter extends NMSAdapter {

private static final MethodHandle methodRemoveGameEventListener;
private static final MethodHandle methodremoveTickingBlockEntity;
private static final Field fieldBiomes;

private static final Field fieldOffers;
private static final MerchantOffers OFFERS = new MerchantOffers();
Expand Down Expand Up @@ -148,8 +147,15 @@ public final class PaperweightPlatformAdapter extends NMSAdapter {
fieldTickingFluidCount.setAccessible(true);
fieldTickingBlockCount = LevelChunkSection.class.getDeclaredField(Refraction.pickName("tickingBlockCount", "f"));
fieldTickingBlockCount.setAccessible(true);
fieldNonEmptyBlockCount = LevelChunkSection.class.getDeclaredField(Refraction.pickName("nonEmptyBlockCount", "e"));
fieldNonEmptyBlockCount.setAccessible(true);
Field tmpFieldBiomes;
try {
// It seems to actually be biomes, but is apparently obfuscated to "i"
tmpFieldBiomes = LevelChunkSection.class.getDeclaredField("biomes");
} catch (NoSuchFieldException ignored) {
tmpFieldBiomes = LevelChunkSection.class.getDeclaredField("i");
}
fieldBiomes = tmpFieldBiomes;
fieldBiomes.setAccessible(true);

Method getVisibleChunkIfPresent = ChunkMap.class.getDeclaredMethod(Refraction.pickName(
"getVisibleChunkIfPresent",
Expand Down Expand Up @@ -412,7 +418,7 @@ public static LevelChunkSection newChunkSection(
@Nullable PalettedContainer<Holder<Biome>> biomes
) {
if (set == null) {
return newChunkSection(layer, biomeRegistry, biomes);
return newChunkSection(biomeRegistry, biomes);
}
final int[] blockToPalette = FaweCache.INSTANCE.BLOCK_TO_PALETTE.get();
final int[] paletteToBlock = FaweCache.INSTANCE.PALETTE_TO_BLOCK.get();
Expand Down Expand Up @@ -502,7 +508,6 @@ public static LevelChunkSection newChunkSection(

@SuppressWarnings("deprecation") // Only deprecated in paper
private static LevelChunkSection newChunkSection(
int layer,
Registry<Biome> biomeRegistry,
@Nullable PalettedContainer<Holder<Biome>> biomes
) {
Expand All @@ -517,6 +522,14 @@ private static LevelChunkSection newChunkSection(
return new LevelChunkSection(dataPaletteBlocks, biomes);
}

public static void setBiomesToChunkSection(LevelChunkSection section, PalettedContainer<Holder<Biome>> biomes) {
try {
fieldBiomes.set(section, biomes);
} catch (IllegalAccessException e) {
LOGGER.error("Could not set biomes to chunk section", e);
}
}

/**
* Create a new {@link PalettedContainer<Biome>}. Should only be used if no biome container existed beforehand.
*/
Expand Down
Loading

0 comments on commit 362e946

Please sign in to comment.