MCTCP / TerrainControl

Minecraft Terrain Generator for SpigotMC and Forge
https://www.spigotmc.org/threads/terraincontrol.37980/
MIT License
231 stars 161 forks source link

ReplaceBlocks isn't working correctly with spawned resources #355

Open Pilvinen opened 9 years ago

Pilvinen commented 9 years ago

Latest TC grabbed from dev builds today.

ReplacedBlocks: (LEAVES,AIR) turns into -> ReplacedBlocks: (LEAVES:0,AIR) when you start the server.

ReplacedBlocks: (LEAVES:1,AIR) turns into -> ReplacedBlocks: (LEAVES:0,AIR) when you start the server.

Timethor commented 9 years ago

TC currently does not support block data on the replaceFrom block. Furthermore, there is no way to implement this at the moment due to the data we have access to at population time.

Please note your replaceTo block may have block data, just not the replaceFrom block. see here

Pilvinen commented 9 years ago

That seem to be a bit besides the point.

If I do: ReplacedBlocks: (LEAVES,AIR),(LEAVES_2,AIR)

I get a world where only leaves with data value 0 are replaced. So I have a broken world with leaves here and there, everywhere.

So, ok. Data values are not supported, fine - but the whole thing is broken and need fixing since right now you can't so anything useful with this.

Pilvinen commented 9 years ago

Also if data in replacefrom is not supported then why does TC change my config from (LEAVES,AIR) to -> (LEAVES:0,AIR) when I start the server.

What I want to do is to remove 100% all leaves from my world. Now only way to do that seems to be by removing 100% all the trees from resource queue which sucks big time.

Timethor commented 9 years ago

When I was testing this I was seeing all leaves replaced with my target block. It is interesting that you are seeing otherwise. Which leads me to ask: What version of TC is it that you are using? and What version of Forge/Spigot/Bukkit are you using.

As to your question regarding the block data getting added, I assume the intent of the person who wrote the line of code that does this wanted to remove the notion that you can add a data value from anyone's mind by always setting the data value to 0. I agree that this is weird behavior, if you don't initially set a data value for your "from" block, you shouldn't be presented with a data value when TC writes back to the file. Again, if you provide me with better information about the version/server type you are using I can better assist you and pinpoint a possible fix.

Pilvinen commented 9 years ago

This server is running CraftBukkit version git-PaperSpigot-5be55c3-18fbb24 (MC: 1.8.8) (Implementing API version 1.8.8-R0.1-SNAPSHOT)

TerrainControl dev build #97 (8.9.2015 3:33:06)

Example from Forest.bc: ReplacedBlocks: (WEB,AIR),(SPONGE,LAPIS_BLOCK),(SEA_LANTERN,GLOWSTONE),(PRISMARINE,SMOOTH_BRICK),(LEAVES:0,AIR),(LEAVES_2:0,AIR),(SNOW,AIR),(LONG_GRASS,AIR)

This removes SOME but NOT all leaves. You will see trees that have leaves here and there when you generate the world.

Timethor commented 9 years ago

Hmm, I did a little more digging and it seems that the replacement process isn't respecting spawned entities... so the replacement is happening but it isn't catching things that end up spawning outside of the chunk "spawn area". This is hard to describe without a picture or two, but this is definitely a bug with block replace for spawned resources. I'll see what I can do to get it fixed :smile:

rutgerkok commented 9 years ago

As far as I can see, there are two bugs here:

@Timethor If you want, I can try to fix the first one. I wouldn't know how to fix the second one.

Timethor commented 9 years ago

@rutgerkok agreed. Second if definitely the bigger of the two tasks and I've already started looking at it... If you want to go ahead and take the first that would be great! It seems to be on line 47 of ReplaceBlocksMatrix: from = TerrainControl.readMaterial(values[0]).withBlockData(0);

Edit: @rutgerkok do you know a lot about the replace block code? When I started looking into it, I was seeing that most leaves were set to data value 8, which is Oak Leaves (check decay)... yet I had a bunch of jungle trees in world. Setting the data value to 8 removed just as many leaves as when I didn't include a data value. Is this a definite limitation on Bukkit or was I doing things wrong?

rutgerkok commented 9 years ago

@Timethor Thanks! I created a new withDefaultBlockData method on LocalMaterialData, so that line 47 is now removing the block data correctly.

About the replace block code: we would need to somehow change the code to also change blocks placed in already modified chunks. Changing our LocalWorld.setBlock code could work, but not all terrain generating code uses that method: trees, dungeons and structures are handled by Minecraft's code.

This issue has been around for years (see #151), and as far as I know it's not easily fixable. You would need to find something really creative to solve it.

JfromHolland29 commented 9 years ago

@Rutgerkok @Timethor I think stuff like that "Objects spawned in a new chunk, but extend to already replaced chunks don't have their blocks replaced in the existing chunk." happens with lava lakes in snow biomes. Often half of them is lava, and half of them is a mix of ice, water, lava, and obsidian...

Timethor commented 9 years ago

@rutgerkok It looks like just expanding the area of the replacement to ALL of the 4 chunks from XWorld.loadFourChunks it fixes the issue in a naive way.

2015-11-09_22 39 01

I think the real way for trees would be to additionally override that part of the MC code and not delegate... but that seems overkill atm. Like you said, the setBlock solution would work for all things we spawn, but nothing MC handles. Plus with that we would still have to run through the traditional replaceBlocks method to eliminate the ChunkProviderTC (pre-population) blocks. That's a pretty big downside considering we would be repeating work. I might do some more testing and push this naive version up to fix the immediate issue and then keep this in the back of my mind for a more proper fix later... what do you think?

rutgerkok commented 9 years ago

Nice work!

Based on your description, some blocks will be replaced twice. Consider ReplacedBlocks: (STONE, GRAVEL), (GRAVEL, GLASS), if blocks are replaced twice then stone will be converted to glass.

For the base terrain there's also a setBlock method: com.khorn.terraincontrol.generator.ChunkBuffer.setBlock.ChunkBuffer is quite new; ChunkBuffer replaces the byte[] array that was in use previously.

Timethor commented 9 years ago

Right, this is a jungle with a config of: ReplacedBlocks: (LOG,DIAMOND_ORE),(DIAMOND_ORE,IRON_ORE): 2015-11-10_12 47 32

As you can see, some trees are diamond and some are iron...

I didn't realize that setBlock was there! So yea, everything that we handle will be able to be replaced, but MC stuff would either have to be overridden or we would have to come up with a smart solution ;)

Timethor commented 9 years ago

This has been fixed in a naive way but introduced another bug illustrated in the image above. For now this will remain open until a more proper solution is reached.

bloodmc commented 7 years ago

@Pilvinen Is this still an issue in latest snapshot?

Pilvinen commented 7 years ago

@bloodmc Oh, wow. It's been several years. I didn't know this bug is still open. I've moved on from Minecraft to develop my own voxel game (Rituals of the old), I'm unable to confirm if this is still an issue.

bloodmc commented 7 years ago

@Pilvinen Ah nice! Good to hear more voxel games coming to life =) I wish you the best in your new game. I'll keep this issue open until someone else can confirm it is fixed. Thanks for the response.