Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom stalactites #50

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Gordon-Frohman
Copy link

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 Dream-Master requested a review from a team March 8, 2024 17:21
@Dream-Master
Copy link
Member

@Gordon-Frohman need to run spotless

@YannickMG
Copy link

YannickMG commented Mar 9, 2024

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 #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.

Copy link

@YannickMG YannickMG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment.

@Gordon-Frohman
Copy link
Author

Gordon-Frohman commented Mar 9, 2024

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
Copy link

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
Copy link
Author

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
Copy link

YannickMG commented Mar 9, 2024

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.

  • Right now the game will crash if one of the hill stalactite lists is empty. For the sake of completion you might want to allow these lists to be empty if you're allowing for the removal of stalactites.
  • Instead of adding stalactites and removing them, you could add a config to not add them or check for the presence of Dreamcraft. It's not a big impact but in principle it's preferable to avoid doing work rather than undo it.

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

@Gordon-Frohman
Copy link
Author

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
Copy link

YannickMG commented Mar 9, 2024

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
Copy link
Author

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

@Gordon-Frohman
Copy link
Author

@OneEyeMaker, is this alright?

@YannickMG
Copy link

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
Copy link
Author

2024-03-10_23 31 29

Lol, WTF?)
It looks like something went wrong

@Gordon-Frohman
Copy link
Author

Gordon-Frohman commented Mar 10, 2024

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
Copy link
Author

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

@Dream-Master Dream-Master requested a review from a team March 11, 2024 09:25
@Dream-Master Dream-Master marked this pull request as draft March 11, 2024 09:40
@Dream-Master Dream-Master added the ongoing freeze - don't merge Not just a bug fix and thus affected by a current freeze for a upcoming version label Mar 11, 2024
@Dream-Master Dream-Master removed the ongoing freeze - don't merge Not just a bug fix and thus affected by a current freeze for a upcoming version label May 3, 2024
@Dream-Master Dream-Master marked this pull request as ready for review May 3, 2024 17:55
@Dream-Master Dream-Master marked this pull request as draft August 17, 2024 20:31
@boubou19
Copy link
Member

any update here?

@Dream-Master
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants