GTNewHorizons / twilightforest

Twilight Forest repository
GNU Lesser General Public License v2.1
14 stars 20 forks source link

Custom stalactites #50

Open Gordon-Frohman opened 8 months ago

Gordon-Frohman commented 8 months ago

Using the addNewStalactite method, you can add stalactites made of various blocks without affecting vanilla ones. You can also adjust:

  1. Level of the hill these stalactites would generate in
  2. Their min/max/relative size
  3. Chance of them generating compared to the others

For example I used it to generate these huge fur stalactites in hills of any level. But, as you can see, no other stalactites are affected by this:

2024-03-01_00 13 59 2024-03-01_00 16 40 2024-03-01_00 16 51 2024-03-01_00 16 57

You can also remove all the vanilla ores from lists if you happen to need it, using the removeAllStalactites method

Dream-Master commented 8 months ago

@Gordon-Frohman need to run spotless

YannickMG commented 8 months ago

I've previously explained I believed this method would not work for the purposes of allowing replacement of stalactites by GT ores, but now I've looked into it a bit further so I can explain why that is.

The code that needs to run to set a block to be a GT Ore is here.

As you can see, due to the nature of GT_TileEntity_Ores as a TileEntity, it needs you to set some additional fields after the call to World::setBlock.

Since you never know exactly what kind of processing a mod needs to perform in order to do their worldgen, my approach in https://github.com/GTNewHorizons/twilightforest/pull/41 was to give the API user mod the ability to just call whatever code they liked in order to place the block. This enables the placing of GT ores.

Now in order to move forward feel free to adapt my code or a similar solution into your PR. If not I would most likely adapt your "add the entire stalactite" model into my PR once @Dream-Master comes back with a solid plan on what exactly he wants these stalactites to be made of, and in what distribution.

In order to make your PR acceptable to merge you should at least test adding GT ores with it, and demonstrate that you have done so. You could do so by temporarily adding Gregtech as a dependency while testing in this mod, or by asking @Dream-Master to tag your PR so you can use that version of TF in another mod that already has a GregTech dependency like the core mod.

Gordon-Frohman commented 8 months ago

Do you remember what @Caedis said about GT ore generation? TileEntities will be replaced with extended metadata after the materials rewrite So I thought that there's no need to write a complex ore placer, and we just need an option to set up blocks metadata, which I did

YannickMG commented 8 months ago

Unfortunately that extended metadata rewrite isn't completed. This PR could be merged as-is once it is completed, but I'm not aware of a specific ETA for it.

For now it needs to be immediately useful after the merge so we can use it with the GT ore blocks we do have today.

Gordon-Frohman commented 8 months ago

Let's just use your PR then It already has everything GT needs, and it shouldn't conflict with mine I would prefer this PR to be merged anyway, since it's functionality can still be used by mods other then GT Or we can keep it unmerged until the materials rewrite is complete

YannickMG commented 8 months ago

Now you have me thinking of a third way. One could use your PR to make a new subclass of TFGenCaveStalactite, in this case maybe GTGenCaveStalactite, and just overrride the setBlockAndMetadata method themselves. In that case we wouldn't need the addNewStalactite methods, just the new addStalactite method.

This would work with GT ores today and moving forward as well.

Would you like to test that approach?

edit: If we're moving forward that way, some additional optional feedback.

Neither of these points are deal breakers, just nice to haves.

Gordon-Frohman commented 8 months ago

Hmm, that sounds like an interesting idea, but I don't quite understand how should it work. You want to Override the makeSpike method, do I get it correctly? Also addNewStalactite is just a nicer version of addStalactite method, where you don't have to put a constructor inside of your function. Maybe I should just rename it Also, hill lists shouldn't actually ever be empty. I can technically add a way of working with empty lists, but then we should have, like, a message in giant letters saying "YOU CAN DO IT, BUT YOU SHOULDN'T!!!"

YannickMG commented 8 months ago

Yeah I'm down with saying you probably shouldn't purposefully empty out hills. Worst case scenario one can add air stalactites.

Hmm, that sounds like an interesting idea, but I don't quite understand how should it work. You want to Override the makeSpike method, do I get it correctly?

Not quite, the subclass would just need to override the setBlockAndMetadata method which is called at the end of makeSpike. It would just need to have this code (ore more likely just a direct call to GT_TileEntity_Ores::setOreBlock). It could override more but that wouldn't be needed if you only want to use a different type of ore block. This would let all of the GT specific logic be hidden inside that subclass of TFGenCaveStalactite

Also addNewStalactite is just a nicer version of addStalactite method, where you don't have to put a constructor inside of your function. Maybe I should just rename it

I would actually argue that addNewStalactite is a worse version of addStalactite. Whenever possible you want to limit the number of methods you make publicly available and limit the number of arguments those take. Having only the single addStalactite method be visible is perfect since it does everything the others do.

Gordon-Frohman commented 8 months ago

@YannickMG, will this do? (We'll put GTGenCaveStalactite into core mod of course)

Gordon-Frohman commented 8 months ago

@OneEyeMaker, is this alright?

YannickMG commented 8 months ago

As a next step why don't you try to add some GT stalactites yourself and see how well that works? Still as a temporary commit.

You can get the right meta like so, I believe: Materials.Gallium.mMetaItemSubID

Gordon-Frohman commented 8 months ago

2024-03-10_23 31 29

Lol, WTF?) It looks like something went wrong

Gordon-Frohman commented 8 months ago

2024-03-11_01 27 46

Okay, it got worse I guess GT generates ores based on a level or something And we'll have to disable it somehow

Gordon-Frohman commented 8 months ago

Ok, I was able to fix the last problem It was caused by an improper generator setting But the first one still remains And I don't know if we can fix it without modifying GT code I'm also know almost nothing about GT, so I need some help from someone more knowledgeable

boubou19 commented 2 months ago

any update here?

Dream-Master commented 2 months ago

yea its a reminder for me to write down some concept how this need to be added. After that Yannik said he will re code it.