GregTechCE/GregTech

[BUG] Rubber Tree WorldGen is implemented wrong

Opened this issue · 1 comments

Versions
GTCE: 1.14.1

Describe the bug
The way rubber tree world generation is implemented looks wrong to me and has a number of potential problems.

The worldgen should not be delegating to the sapling.generateTree() it is meant to be the other way around.

BlockGregSapling.generateTree() looks ok in that it uses the real world gen (not what is registered via forge).

But doing it this way has some bad consequences;

  1. The sapling is placed before the real world gen has had a chance to check if the tree can really be generated (this is the real bug that #1590 partially fixes)
  2. Block update notifications are being performed during worldgen which should not be happening

Some of this is kind of worked around by this code in BlockGregSapling:

        worldIn.setBlockState(pos, Blocks.AIR.getDefaultState(), 4);
        if (!worldgenerator.generate(worldIn, rand, pos)) {
            worldIn.setBlockState(pos, state, 4);
        }

which first removes the sapling and then puts it back if the generation fails.

I believe the correct fix should be something like the following.

Have 2 seperate world generators registered with forge. These are effectively the reals one that get used in BlockGregSapling.generateTree (except notifications would be off).
The big one would have 10x the weighting to reduce it chances.

And, they will have an additional method that does the world type/biome specific code that is in the current rubber tree world gen when called via forge (completely untested code):

public class WorldGenTreesCustom extends WorldGenTrees implements IWorldGenerator {
or 
public class WorldGenBigTreeCustom extends WorldGenBigTree implements IWorldGenerator {

    // Insert Relevant constructors here

    // This duplicated code could be implemented in a default method of an interface?
    @Override
    public void generate(Random random, int chunkX, int chunkZ, World world, IChunkGenerator chunkGenerator, IChunkProvider chunkProvider) {
        if (world.getWorldType() == WorldType.FLAT ||
            !world.provider.isSurfaceWorld()) {
            return; //do not generate in flat worlds, or in non-surface worlds
        }
        BlockPos randomPos = new BlockPos(chunkX * 16 + 8, 0, chunkZ * 16 + 8);
        Biome biome = world.getBiome(randomPos);

        if (BiomeDictionary.hasType(biome, Type.COLD) ||
            BiomeDictionary.hasType(biome, Type.HOT) ||
            BiomeDictionary.hasType(biome, Type.DRY) ||
            BiomeDictionary.hasType(biome, Type.DEAD) ||
            BiomeDictionary.hasType(biome, Type.SPOOKY))
            return; //do not generate in inappropriate biomes

        int rubberTreeChance = 6;
        if (BiomeDictionary.hasType(biome, Type.SWAMP) ||
            BiomeDictionary.hasType(biome, Type.WET))
            rubberTreeChance /= 2; //double chance of spawning in swamp or wet biomes

        if (random.nextInt(rubberTreeChance) == 0) {
              generate(world, random, randPos))
        }
    }

   // Include setBlockAndNotifyAdequately for the big tree version

Notice it invokes the real world gen directly allowing all the checks to take place and no hacks with sapling placing.

The BlockGregSapling.generateTree, would use these new world gen objects with notifications on, but it would not need to do any dance with removing and replacing the sapling.

I forgot to mention that these new WorldGen classes should also probably override this method

    @Override
    protected boolean canGrowInto(Block blockType)
    {
        return super.canGrowInto(blockType) || blockType == MetaBlocks.SAPLING;
    }