Skip to content

Comments

More balance settings and GT Ore integration#36

Closed
Minepolz320 wants to merge 11 commits intoGTNewHorizons:masterfrom
Minepolz320:master
Closed

More balance settings and GT Ore integration#36
Minepolz320 wants to merge 11 commits intoGTNewHorizons:masterfrom
Minepolz320:master

Conversation

@Minepolz320
Copy link

@Minepolz320 Minepolz320 commented Jan 14, 2024

-Ability to adjust the density of ore in stalactites
-Ability to disable certain ore in stalactites
-Ability to replace the type and ore in stalactites using ore metadata from GT

-Ability to customize the frequency of dungeon generation and landscape features

I immediately apologize for the low quality of the code
I can’t decide what to do:
it’s better to add the required method to the gregtech api or just use reflection
however, that’s why I opened it as a Draft

image

50%
image

image

@Dream-Master
Copy link
Member

Make a config that we can add every gt ore not only this one you hard coded @Minepolz320

@Minepolz320
Copy link
Author

@Dream-Master yes, you can simply specify a different ore meta for a specific stalactite

gt_oremapping {
I:GT_coalOreMeta=535
I:GT_diamondOreMeta=500
I:GT_emeraldOreMeta=501
I:GT_goldOreMeta=86
I:GT_ironOreMeta=32
I:GT_lapisOreMeta=526
I:GT_redstoneOreMeta=810
}

@Minepolz320 Minepolz320 marked this pull request as ready for review January 16, 2024 08:39
@Dream-Master Dream-Master requested a review from a team January 16, 2024 17:14
@Caedis
Copy link
Member

Caedis commented Jan 16, 2024

I would prefer if this was handled in GT and TF exposes an API that can be called

@Minepolz320
Copy link
Author

@Caedis Yes, but on the other hand, you don’t need a dependency to build a mod for such a minor change

I myself can’t decide whether it’s worth writing an api in the greg

@YannickMG
Copy link

We really shouldn't be using reflection for new code unless we have to.

@Minepolz320
Copy link
Author

@YannickMG unfortunately I can’t rewrite the code now, maybe later or I can cancel the request for now or just mark it as a draft

@Minepolz320
Copy link
Author

It even seems to work with the non-GTNH version of gregtech

@Minepolz320
Copy link
Author

Up?

@YannickMG
Copy link

Will be reviewing this evening.

@YannickMG YannickMG self-requested a review January 31, 2024 23:07
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.

Overall great and very welcome addition! I've made a few comments and suggestions.

Also, while this is in no way a blocker, I will mention that while we're adding the ability to replace TF ores I wonder why we're only looking at GT5U integration. For instance GT++ has Koboldite which it would be very nice to use here. If you want to make your solution more generic I suggest you simply expose an API that accepts a functional interface for the signature void setBlock(World world, int x, int y, int z, Block block) and let the caller decide exactly what ore block they want to place down. In that case integration with GT would be done in code in NewHorizonsCoreMod.

@@ -378,6 +378,15 @@ public static TFFeature generateFeatureForOldMapGen(int chunkX, int chunkZ, Worl
}

public static TFFeature generateFeatureFor1Point7(int chunkX, int chunkZ, World world) {
Copy link

@YannickMG YannickMG Jan 31, 2024

Choose a reason for hiding this comment

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

It appears this function isn't only used when generating a feature (as its name would suggest) but also when looking up which feature should exist in a given region.

Changing the configurated change post-worldgen affects functionality that check against existing features such as the magic map.

Is this intended? If so you might want to document the caveat that this config isn't meant to be modified on an existing world.
image

Copy link
Author

Choose a reason for hiding this comment

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

yes, definitely need a warning, I was worried that this would happen

if (ry < 75) {
TFGenerator rf = randomFeature(randomGenerator);
rf.generate(currentWorld, randomGenerator, rx, ry, rz);
// now with chance

Choose a reason for hiding this comment

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

The "now" part of "now with chance" isn't going to be very helpful to future readers of the code, for which that comment will always have been there.

If the intent of the comment is to indicate that there's a chance of the feature not happening then you might want to encode that knowledge about the code's intent into a well named function instead.

Comment on lines 31 to 33
// Greg?
public boolean doOreGen;

Choose a reason for hiding this comment

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

The // Greg? comment appears to be misleading since this can be used without GregTech. Also you should generally not expose fields publicly so this should be private.

Comment on lines +1 to +12
package twilightforest.gt_integration;

import net.minecraft.block.Block;
import net.minecraft.init.Blocks;
import net.minecraft.world.World;

import gregtech.common.blocks.GT_TileEntity_Ores;
import twilightforest.TwilightForestMod;

public class GT_OrePlacer {

public int getMappedGTMetaForOre(Block mOre) {

Choose a reason for hiding this comment

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

getMappedGTMetaForOre should be private, and if you remove GT_Integration_Utils::getOrePlacer then all of GT_OrePlacer can be package private instead of public to hide your implementation details.

Comment on lines 1 to 31
package twilightforest.gt_integration;

import net.minecraft.block.Block;
import net.minecraft.world.World;

public class GT_Integration_Utils {

static GT_OrePlacer orePlacer;

public static void init() {
// Create worker obj
orePlacer = new GT_OrePlacer();

}

public static boolean isInit() {
return orePlacer != null;
}

public static boolean doPlaceGTOre(World aWorld, int aX, int aY, int aZ, Block mcOreBlock) {
if (!isInit()) {
return false;
}
return orePlacer.placeGTOre(aWorld, aX, aY, aZ, mcOreBlock);
}

public static GT_OrePlacer getOrePlacer() {
return orePlacer;
}

}

Choose a reason for hiding this comment

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

You should reduce the visibility of this class's members.

orePlacer and isInit can be private, getOrePlacer is not used and should be removed (or used).

Comment on lines 162 to 170
if (TFFeature.getRandom(randomGenerator, TwilightForestMod.minorFeatureGenChance)) {
if (randomGenerator.nextInt(6) == 0) {
int rx = chunk_X + randomGenerator.nextInt(16) + 8;
int rz = chunk_Z + randomGenerator.nextInt(16) + 8;
int ry = currentWorld.getHeightValue(rx, rz);
if (ry < 75) {
TFGenerator rf = randomFeature(randomGenerator);
rf.generate(currentWorld, randomGenerator, rx, ry, rz);
}

Choose a reason for hiding this comment

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

This new minorFeatureGenChance config option now compounds with an existing 1 in 7 chance of generating a minor feature. This makes the config option a bit misleading and limits our ability to do things such as making minor features more common.

Would it be possible to combine these random chances, such as by using 1/7 (~14.25%) as the default value of the config option?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I think it’s logical to combine and leave the default 14% in the config

@Minepolz320
Copy link
Author

@YannickMG Tanks for amazing review
I'll try to fix everything

@Minepolz320
Copy link
Author

Minepolz320 commented Feb 1, 2024

Overall great and very welcome addition! I've made a few comments and suggestions.

Also, while this is in no way a blocker, I will mention that while we're adding the ability to replace TF ores I wonder why we're only looking at GT5U integration. For instance GT++ has Koboldite which it would be very nice to use here. If you want to make your solution more generic I suggest you simply expose an API that accepts a functional interface for the signature void setBlock(World world, int x, int y, int z, Block block) and let the caller decide exactly what ore block they want to place down. In that case integration with GT would be done in code in NewHorizonsCoreMod.

ok i try to do this and make it possible to switch the mode in the config

сan i look some examples where is this done, for example in our project

@boubou19
Copy link
Member

superseded by #50

@boubou19 boubou19 closed this Mar 10, 2024
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