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

[1.21.1] Sync remaining c tags with NeoForge #4161

Open
wants to merge 37 commits into
base: 1.21.1
Choose a base branch
from

Conversation

TelepathicGrunt
Copy link
Contributor

@TelepathicGrunt TelepathicGrunt commented Oct 13, 2024

In the original unify tag PR, the aim was to sync majority of tags but leave out the super niche tags in order to not bug down the unify work with too much nitpicking. I think it is now a good time to work on discussing these niche tags on which to fully sync between Fabric and Neoforge and which might be time to retire in 1.22 instead of adding to the other loader.

I ran a small Python script and collected all the tags that are in NeoForge but not Fabric and vice versa. I made this PR to NeoForge to add c tags to it that Fabric had: neoforged/NeoForge#1593

This current PR is for c tags that are in Neoforge but not Fabric. HOWEVER, I will point out, there is quite a handful of tags added and I honestly don't expect them all to be liked. Please take a look and give feedback which tag looks good, or missing entries, or is inconsistent, or you feel is too niche or pointless.

Neo side is willing to discuss this. Like I am yeeting the c:is_modified biome tag out of Neo in 1.22 (marking deprecated now) instead of syncing it because that tag is outdated and lost all meaning with today's worldgen systems.

I did split every tag into a separate commit to help reviewers out. It should be easier to look through all the tags in this PR than the original unify PR that lagged GitHub lol. Also ran the game locally to make sure that there is no tag logspam.

Again, no rush on this PR. Lets give it time needed for review. This PR should be easy to cherry pick into 1.21.2 as well in theory. I would like to get it in for both 1.21.1 and future versions since a lot of modpacks are gonna to settle on 1.21.1 for a while.

Side note: Looks like the c:is_cold tag was missing c:is_cold/end tag. Fixed now in this PR.

getOrCreateTagBuilder(ConventionalBiomeTags.IS_LUSH)
.add(BiomeKeys.LUSH_CAVES);
getOrCreateTagBuilder(ConventionalBiomeTags.IS_MAGICAL);
getOrCreateTagBuilder(ConventionalBiomeTags.IS_RARE)
Copy link
Member

Choose a reason for hiding this comment

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

Im not sure about the rare biomes or the ore rates, as they have nothing to do with the tags. You could have a another datapack that invalidates these. Whats a use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much like storage block tags that can be invalidated by datapacks, it is on the modpack maker to adjust any relevant tags when they are customizing their modpacks so personally, that angle, I think is fine.

For biome rarity, some mods seems to target these biomes for uncommon ores or mobs. Not many but a few. Like Alex's Mobs seems to add Mungus to these rare tagged biomes. There's Create Arcane Engineering that adds Aurum, Ferrum, and other ores to rare biomes. Resourceful Bees spawns their easter egg Oreo Bee in rare biomes. Ice and Fire spawns their Pixie Village in rare biomes as well. So there's some use cases for targeting biome that are infrequent/difficult to find by default. And then if pack makers make a biome more or less common, they would adjust the is_rare tag to reflect the new change,

For ore_rates, I had the same thought as you of who even uses this. Turns out Tinker's uses it quite a bit. I'll ping @KnightMiner to come talk about his usages for that. RCXcrafter said he should be using ore rates tag but haven't done so yet. If packs changes the rates, they should adjust the tags too.

@@ -166,6 +195,46 @@ private ConventionalBlockTags() {
*/
public static final TagKey<Block> HIDDEN_FROM_RECIPE_VIEWERS = register("hidden_from_recipe_viewers");

/**
* Blocks which are often replaced by deepslate ores, i.e. the ores in the tag {@link #ORES_IN_GROUND_DEEPSLATE}, during world generation
Copy link
Member

Choose a reason for hiding this comment

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

Question: Why is a sperate tag needed for these?

Copy link
Contributor Author

@TelepathicGrunt TelepathicGrunt Oct 14, 2024

Choose a reason for hiding this comment

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

This is more of the actual ground block itself (deepslate, stone, netherrack) rather than the ores themselves. Twilight Forest uses these tags for their Ore Magnet. Not sure how they use it but I can ask them to come comment here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original source of the ore_bearing_ground tags came from here. Pinging @SuperMartijn642 if they want to elaborate more about it. Does seem like mods make use of it. Not many but some do.
image
MinecraftForge/MinecraftForge#7891 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

The reasoning behind these tags was that some mods generated ore related recipes from the ore tags and that some machines, for example the pulverizer from Thermal Expansion, may drop the block the ore is in (so stone up to 1.17) as a byproduct.
In 1.17, deepslate variants of ores were added to vanilla, hence there was a need to differentiate between ores in stone and ores in deepslate. As there are mods which add additional ore bearing blocks, the idea was that if there's a way to identify the ground block the ore is in, recipes for those ores could be generated automatically as well.

So, for example, a mod may add a marble block and a marble coal ore block.
Then the marble coal ore block would go in ore_in_ground/marble and the marble block would go in ore_bearing_ground/marble.
Some mod which may want to find all coal ores and their corresponding ground block could then get all coal ore blocks from ores/coal. Then for each, check in which ore_in_ground/x tag the ore block is and then obtain the ground block from ore_bearing_ground/x.

I personally haven't used these tags and I am not sure whether these tags have actually been adopted by mods since they were added to NeoForge.

Choose a reason for hiding this comment

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

I have an item that I have yet to port for my embers rekindled port called metallurgic dust.
When used on an ore vein it transmutes the entire vein into a random different ore from the same stone type, each ore block also has a chance of just turning into stone.
Having these tags would be great for making this item work out of the box with modded ores.

Choose a reason for hiding this comment

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

Twilight Forest uses these tags for their Ore Magnet. Not sure how they use it but I can ask them to come comment here

For clarity, it is used for procedurally determining the block that we replace at an Ore's location when translocating the Ore block via the Ore Magnet / Tree of Mining. Thanks to the tags, there is zero hardcoding for the relationships between "source blocks" and the actual ore blocks

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Oct 14, 2024

Just saw this issue report: #4150

This PR would close that issue report

@modmuss50
Copy link
Member

Seems to be crashing when running:

Caused by: java.lang.AssertionError: Expected field ConventionalBiomeTags.IS_VEGETATION_SPARSE_NETHER to match one of [IS_SPARSE_VEGETATION_NETHER, NETHER_IS_SPARSE_VEGETATION] for tag TagKey[minecraft:worldgen/biome / c:is_sparse_vegetation/nether]

@modmuss50 modmuss50 added the enhancement New feature or request label Oct 23, 2024
@TelepathicGrunt
Copy link
Contributor Author

@modmuss50 yes. I asked a few times in discord what to do about the naming of the biome fields but got no feedback from others. Basically, do we change the field name of all the biome fields which is a breaking change, change just this biome tag field to be inconsistent with the other fields, or find a way to exclude this field like how the others are excluded from the check

@modmuss50
Copy link
Member

in discord what to do about the naming of the biome fields but got no feed

Ah sorry, I am just catching up on things now. IS_VEGETATION_SPARSE_NETHER seems fine to me as it matches the existing IS_VEGETATION_SPARSE_OVERWORLD I personally dont feel storngly enough about this to change it feel free to go ahead and special case it.

The PR looks good, as I have said before im happy for you to take the lead on this stuff and do what you feel is right.

@TelepathicGrunt
Copy link
Contributor Author

Thanks! Through I am not sure where the special casing is done. I’ll make a 1.21.3 pr in a bit to have this work be on there as well but I won’t be able to do the new wood stuff because that requires special handling with feature flags that’s outside my skill set

@modmuss50
Copy link
Member

I won’t be able to do the new wood stuff because that requires special handling with feature flags that’s outside my skill set

I can maybe help with this, im not too sure what needs doing hopefully nothing too extreame.

@TelepathicGrunt TelepathicGrunt changed the title Sync remaining c tags with NeoForge [1.21.1] Sync remaining c tags with NeoForge Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants