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

Unify tag namespace and conventions between Neoforge and Fabric for 1.20.5 MC #135

Merged
merged 10 commits into from
Apr 20, 2024

Conversation

TelepathicGrunt
Copy link
Contributor

@TelepathicGrunt TelepathicGrunt commented Sep 10, 2023

Background:

The modding community has been split between two tag namespaces. The c tag and the forge tag with one using folders and the other not. Now that Neoforge is a thing and willing to have discussions about switching namespaces for their tags, we are at a point were we can finally unify both loaders to the same namespace for tags and conventions!

Fabric PR: FabricMC/fabric#3310

Preface:

This large tag PR is not perfect but it is a great first steps toward a unified tag namespace and convention to ease the burden between both loader's ecosystems and allow for datapacks or other ecosystems to hop in as well to our namespace and conventions if they desire.

There is a ton of moving parts in this PR. Each of these parts I felt are interconnected and are best done all together in a single to help make this switch to the new namespace/conventions as best as possible. The sections below will help break down the PR and go into detail of each part and the reasons for each part.

If after reading the reasons, you still have concerns or suggestions, please voice them on this PR so we can have a discussion!

Goals:

  1. Unify both modloaders to the same namespace for tags which is common in this PR. EDIT: we are switching to using c namespace instead.

  1. Add or rename tags to better match each other on both loaders and to match the convention of using folders for better organization.

  1. All past tags have a modern equivalent in this PR with backwards compat using optional tag entries. No tags were entirely removed. (Quartz and amethyst were removed from storage blocks tag with no replacement because they are not storage blocks)

  1. Datagen translations for modloader common tags under the key format of tag.<registrypath>.c.<tagname> where slashes are turned into periods. Modded registries should specify their namespace like so so it doesn't conflict with vanilla registry: tag.<registrynamespace>.<registrypath>.c.<tagname>

  1. Log short warning by default to console if old tag namespace is detected in a modder's dev environment. Config exists to either turn it off or make it verbose to say all found legacy tags. (Short dev message on by default)

  1. Log short warning by default to console if untranslated modded item tags is detected in a modder's dev environment. Have config to either turn it off or make it verbose to say all found untranslated modded item tags. (off by default)

Details And Reasons Of Each Goal:

Namespace choice:

Edit: For better chance of acceptance, we are going to go with c namespace. The c namespace pollution is largely unique to Fabric ecosystem so it will be on them to work out how to clean it up on their side. On Neo's side, the c namespace is largely a blank slate so we should be ok.

There are a few reasons to go with common namespace as oppose to the currently established c namespace on Fabric.


First, the word common is much more clear to new modders or to players/modpack makers that this namespace is shared across many mods. c doesn't actually mean anything to people not previously familiar with it. Which can lead to confusion on what c is for. common appears best for conveying the meaning of the namespace to people. Not a powerful reason for this but it is a reason.

Second, the c namespace is in disaster in conventions at the moment. I'm not talking within Fabric API. I mean as Fabric ecosystem as a whole. To help show what I mean, here is a sheet of all tag usages from the top 1000 Fabric/Quilt mods on Modrinth as of August 4th, 2023: https://docs.google.com/spreadsheets/d/1KBn2hKOz3nnIYh3KAeqv5vezMwFSDO-tL8TUAKYTiBE/edit?usp=sharing

Third, the reason we are not sticking with forge namespace is because Neoforge is not Forge. And we should be using a modloader-agnostic tag namespace anyway so that if Neforge goes under for whatever reason, the new modloader can pick up the new namespace and not have to fight to change it. Makes it much easier on everyone. Keeping forge namespace is not an option.

In there, you can find some people are using folders under c. Some people are not. Some people are properly pluralizing their tag names. Some are not. Basically, it's a mess and I feel this was partially because some modders don't know where to find existing tags and some were not able to figure out what the conventions are when looking at Fabric API's tags. To help facilitate Goal 5, switching off of c to common makes that goal of notifying people of the new convention we are switching to a million times easier. More info about this logging is in the section below about the logging goal. But yes, this was a big reason for my choice of picking common. If we go back to c instead, please provide me an alternative solution for notifying people of the new conventions.


NOTE: I know people are going to point out that common is a valid modid unlike c and that this makes common bad. The truth is, that's actually a non-issue. Say if some random mod is made using common modid, the modding community will just ignore that mod anyway and it doesn't impact modpacks or anything. Modders will just tell the owner of that mod to change the modid and that'll be that. If it is a heavy concern still, modloaders themselves can just include a dummy common modid mod to reserve that namespace to prevent anyone else from using it. Super simple solution. I will leave the choice of reserving the common modid up to the modloaders themselves where they can make a PR after this one if we stick with common namespace.


Renaming and moving tags to same conventions

Fabric's c convention was to never use folders. Forge's convention was to use folders. And Minecraft started out with not using folders but then now does with its block mineable tags.

What it looks like to me is that Minecraft starts with no folders originally but as they grew, they started seeing the value of grouping similar tags together by folders. But they still keep the non-folder tags for backwards compatibility. Forge right away saw that trying to mimic vanilla would be an issue for scalability in a modded ecosystem so they went with folders. Fabric however, stuck with the legacy non-folder convention that vanilla did and this lead to a lot of messiness that lead to the current situation with the c namespace tag conventions: https://docs.google.com/spreadsheets/d/1KBn2hKOz3nnIYh3KAeqv5vezMwFSDO-tL8TUAKYTiBE/edit#gid=0

If you check what fabric currently does with dyes, all of them are in the format of c:<color>_dyes which just bloats the top level view of the c tags: https://github.com/FabricMC/fabric/tree/93d8cb82e85c3d5744716430516daef393cf5815/fabric-convention-tags-v1/src/generated/resources/data/c/tags/items

Instead, if we go with the folder convention, these tags would've be grouped under c:dyes/<color> which is a lot more organized and cleaner. It is also far more scalable for a modded ecosystem as more tags can be added to the folder without making the top level view harder to find certain tags. Example of what a folder dye tag looks like:
https://github.com/neoforged/NeoForge/tree/1.20.x/src/generated/resources/data/forge/tags/items/dyes

That is why the convention of this PR is plural + folders. Because of this, many tags from both c and forge were shuffled around so that tags that mean the same thing on both loaders now matches each other in the c namespace so multi-loader modders only need to pull from 1 tag instead of 2.


Backwards compat with old tags

I put a lot of work into making sure that all old tags from the modloader are added as an optional tag entry in the modern equivalent of the tag. This will allow built jars and datapacks using the old tag names to continue to have compatibility throughout 1.21 Minecraft. Then these backwards compatibility can be deleted in 1.22 Minecraft to clean up this remaining tech debt as everyone had an entire major Minecraft version to update their tags.

However, because of all the tag renaming and moving around, the tag fields in code were also rename to match the changes. This means this PR is indeed a breaking change for code references to these fields. This is why this PR is marked for 1.21 Minecraft as that is a good time for doing breaking changes. The reason I did not keep the old field names nor make dummy fields that references the new fields is this would create a lot of tech debt that I felt the modloaders don't want to take on. If this is a deal breaker and that the dummy fields should be added to keep code compat, let me know and I'll update this PR to not be breaking for code even with the tech debt.

As you read this PR's code, you'll notice I put the optional tag entries at the bottom of all the datagen classes for tags. This is so that the optional entry shows up last in the tag's json (cleaner to me and shows it is not super important to viewers) and also makes it very easy to delete in 1.22 Minecraft. Just select the code block at bottom and delete. Done. No more backwards compat. Hopefully this helps!

More info on individual tags can be found in the Misc section.


Datagenning tag translations

This might seem strange at first but it turns out that recipe viewer mods are making use of translations for item tags. I asked about this and another person said their own mod would also benefit from item tag translations. So there is a demand for translations of these tags and currently, every mod that does will need to add translations for all the c and forge tag entries from the modloaders. Doesn't seem right.

So instead, I added translations for the tags and I figured, since I am here making item tag translations, might as well add translations for the rest of the common tags for completeness. I did not add translations for minecraft item tags but if desired, I can update this PR to include datagen translations for Minecraft's item tags. Not sure if this is something a Modloader wants to handle from Minecraft. But at the very least, modloaders should be translating their own tags to help out modders and reduce duplicate translations between mods.

Also, LanguageProvider.add(<TagKey>, <name>) is added as a helper method that modders can use to make it easier to datagen tag translations. Just takes the TagKey and translated name and will make the lang entry for you!


Log warning for legacy namespace presence

This part is critical to getting modders to update to the new namespace and conventions. Basically, there's no way we can reach out to every modder through Discord or through changelogs as people just update their modloader/minecraft version and start porting. There are too many ways that people can slip through the cracks.

So to have a solution that gets this new namespace and convention change out to all modders that actually need this info, this PR include code that will grab all the tags from all Minecraft registries and check for the presence of the legacy namespace. If it is found, it will log a short message that states:

Dev warning - Legacy Tags detected. Please migrate your 'forge' namespace tags to 'common' namespace! See net.minecraftforge.common.Tags.java for all tags.
NOTE: Many tags have been moved around or renamed. Some new ones were added so please review the new tags. And make sure you follow tag conventions for new tags!
You can disable this message in Neoforge's common config by setting logLegacyTagWarnings to "SILENCED" or see individual tags with "DEV_VERBOSE".

If set to verbose, the above message will print out this format:

Dev warning - Legacy Tags detected. Please migrate your 'forge' namespace tags to 'common' namespace! See net.minecraftforge.common.Tags.java for all tags.
NOTE: Many tags have been moved around or renamed. Some new ones were added so please review the new tags. And make sure you follow tag conventions for new tags!
You can disable this message in Neoforge's common config by setting logLegacyTagWarnings to "SILENCED" or see individual tags with "DEV_VERBOSE".

Legacy tags:
    TagKey[minecraft:item / forge:pies]
    TagKey[minecraft:item / forge:potatoes]
    TagKey[minecraft:entity_type / forge:trains]

This is set to print only in DEV_SHORT mode by default so that mod's development environment after world load will see only the shortened message. The modes are SILENCED, DEV_SHORT, DEV_VERBOSE, PROD_SHORT, PROD_VERBOSE. Prod is included as an opt-in option in case modloader maintainers want to see how many legacy tags are in use for 1.21 modpacks. The prod options can be removed from PR if desired.

Hopefully this isn't too intrusive. But at least the message says how to turn it off as well as where to find the new tags. This should greatly help speed up modders switching to the new tags and help them see what the desired conventions are. The end goal is a common namespace that is not a wild-west of conventions like the c namespace is. Will this be successful? I don't know but I have hope that it will. Only way to know for sure is to try this out.


Log warning for untranslated item tags presence

NOTE: This part is a bit more controversial but if desired, this logging can be chopped out of this PR.

EDIT: This is now set to off by default and has to be opt-in by the modder in the config file.

The idea is since recipe viewers and other mods rely on item tags to be translated, a short message will be logged if any are found to be untranslated from mods. I did not do the other tag types because there is no demand for those to be translated and thus, doesn't make sense to push for translated tags of other stuff. Only item tags.

The code in this PR will grab all the item tags and if running on client side, check if the tag is translated. If it is not, it will log a short message that states:

Dev warning - Untranslated Item Tags detected. Please translate your item tags so other mods such as recipe viewers can properly display your tag's name.
The format desired is tag.item.<namespace>.<path> for the translation key with slashes in path turned into periods.
You can disable this message in Neoforge's common config by setting logUntranslatedItemTagWarnings to "SILENCED" or see individual tags with "DEV_VERBOSE".

Setting it to verbose basically prints out all the item tags like so:

Dev warning - Untranslated Item Tags detected. Please translate your item tags so other mods such as recipe viewers can properly display your tag's name.
The format desired is tag.item.<namespace>.<path> for the translation key with slashes in path turned into periods.
You can disable this message in Neoforge's common config by setting logUntranslatedItemTagWarnings to "SILENCED" or see individual tags with "DEV_VERBOSE".

Untranslated item tags:
  modded1:pies
  modded2:potatoes
  modded2:paintings/1x1

This is set to print only in DEV_SHORT mode by default so that mod's development environment after world load will see only the shortened message. The default is SILENCED mode and has to be opt-in. The modes are SILENCED, DEV_SHORT, DEV_VERBOSE, PROD_SHORT, PROD_VERBOSE. Prod is included as an opt-in option in case modloader maintainers want to see how many untranslated modded item tags are in use for 1.21 modpacks. The prod options can be removed from PR if desired.


Misc

So for the individual tags in this PR, there's a lot of changes done. This is not a comprehensive list but some notable points.

  • Added Cherry Grove to biome tags. Yeah it was missing lol. I also added a c:is_floral biome tag to hold biomes that are super dense in flowers as quite a few mods are trying to add their stuff to flowery based biomes.

  • Added "vegetation" to the is_sparse and is_dense biome tag names so it is now is_vegetation_sparse and is_vegetation_dense. Much more clear that the sparse and dense is talking about the density of plants. Not density of other stuff like boulders lol. Makes it easier to match up with the Fabric PR as well.

  • Added c:honey fluid tag as honey is added by several mods and is in dire need of standardizing.

  • glass is now glass_blocks to be clearer that it is about the full block form of glass. Not a tag of glass blocks and glass panes together.

  • Added c:boats and c:minecart tags because we probably should have these tags for better compat lol. Surprised they weren't made before.

  • Moved forge:shears to c:tools/shears because Shears are a tool. Not sure why it was classified as not before.

  • c:relocation_not_supported block and block entity type tags are is added because there's many mods that were trying to add their own forge:relocation_not_supported or forge:immovable tags before. Now it is standardized and should help with adoption to ease mod compatibility.

  • forge:glass/silica is renamed to c:glass/cheap because my understanding of the silica tag was it was really for checking if a glass block was cheap to be made to help prevent consumption of more expensive glass blocks for various mechanics. So changing the term from silica to cheap should make this far more clear and hopefully better uptake. Especially as a mod could have a glass block that is made from say Sand + Nether Stars to create Star Glass. They would've put this expensive glass into the silica tag because it used Sand but then defeats the original purpose of this tag. Hence why now it is using cheap.

  • Brand new c:dyed_blocks and c:dyed_items tag was added and has a folder for individual colors in it. I added this because the Forge tags had 32 colors tags for Stained Glass and Stained Glass Panes. It's awful. Imagine if this was extended to every colored block... My solution was to remove those colored glass tags and instead, create new overarching tags that collect all dyed color blocks and items. And the subfolder for these dyed_blocks and dyed_items lists each individual color. The goal is now if you wanted Red Stained Glass Blocks, you would check if a block is in the c:glass_blocks tag and is also in the c:dyed_blocks/red tag. If so, there's your collection of Red Stain Glass Blocks! This is now universal and can be used for any dyed color block! Much cleaner and far more useful. I hope color-based mods like Spectrum will make good use of this!

  • c:capturing_not_supported entity type tag now added because there's over 20 mods that I know of that lets players carry an entity or stuff them into an item to move around. I added one entity myself and had to add it to so many mod's disallow tags for picking up into items. We definitely need to standardize this so entity creators only have 1 tag to add to and other mods can pull this tag into their own disallow tag for their stuff.

  • Not every tag from the Fabric PR is ported over to this PR. I only ported the tags that I assumed would be very useful to have such as c:boats and c:minecarts. The reason for this is some tags in the other loader could be questionable so rather than bog this PR down with arguments over these various tags, I'll leave it up to another person's PR to attempt to add tags from one loader to another after my PR. All I did was make sure that the tags in each loader follows a convention and tags for the same thing matches up.

Conclusion

Again, please let me know your feedback or suggestions or thoughts about the various parts of this PR. It is not perfect and getting loaders to use the same namespace and convention for tags is a controversial topic. That is why I am making this PR early long before 1.21 so we can have enough time for discussions.

Please also checkout the discussions on the PR for the other loader too as what is talked about there could also impact this PR. No matter what namespace is chosen in the end, both PRs will use the same namespace. That is a hard requirement of this PR so please make sure others from the other loader are involved in the discussion.

Fabric PR: FabricMC/fabric#3310

Thank you for you time to read this! I know it was long. But this is very important for the modding ecosystems as what is decided for these PRs will have long-lasting impact.

@CLAassistant
Copy link

CLAassistant commented Sep 10, 2023

CLA assistant check
All committers have signed the CLA.

@TelepathicGrunt TelepathicGrunt changed the title Unify tag namespace and conventions between Neoforge and Fabric for 1.21.0 MC Unify tag namespace and conventions between Neoforge and Fabric for 1.21 MC Sep 10, 2023
@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Sep 10, 2023

Note with my latest push I just did: common:capturing_not_supported entity type tag now added because there's over 20 mods that I know of that lets players carry an entity or stuff them into an item to move around. I added one entity myself and had to add it to so many mod's disallow tags for picking up into items. We definitely need to standardize this so entity creators only have 1 tag to add to and other mods can pull this tag into their own disallow tag for their stuff.

Picture of my pain with this lmao: https://imgur.com/svORGdt

@Commoble
Copy link
Contributor

I'm not translating my tag keys, I don't want logspam in dev for untranslated tag keys, and I don't want to have to change the client config each time I set up a new workspace to not log untranslated tag keys

@moonfather1
Copy link

you are being tame about notifying people, TG. i suggest a proper warning screen, like the one for experimental worldgen (full screen, white letters on dirt background).

it's not too punishing for people to click one button (dev env only of course), but would drastically help with visibility.

@Commoble
Copy link
Contributor

Given that mojang isn't translating its own tags, I don't think pushing devs toward making translation keys for theirs isn't something that neoforge should be obligated to do

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Sep 14, 2023

Updated PR to now have the item tag translation warning be off by default and modders have to opt-in through the config. The legacy tag warning will direct people to the config anyway and there the modder can choose to have their item tags monitored for missing translations.

Reminder, still good practice to translate your item tags so recipe viewers and other modders can benefit from the tag translations! People do want to follow vanilla's example of hiding resourcelocations from player unless player opts-in to F3 or advanced tooltips. So more tag translations leads to more immersive modpacks with regards to names of tags displayed to players in various modded UI.

@moonfather1 I dunno. I'm on the fence on that if it is too obtrusive. If enough people tell me they are in favor of that, my hard condition for it is that it only shows up once ever per workspace and never again once modder clicks through the window. Only shows for legacy tag warning and only in dev environment of course. Also someone will need to help me as I stink with getting UI/screens to work lol

@Commoble
Copy link
Contributor

The logspam shouldn't be opt-in, it shouldn't be in neoforge at all. If people want to warn themselves about their own tags, they can write their own warning system.

@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request 1.20 Targeted at Minecraft 1.20 breaking change Breaks binary compatibility labels Sep 17, 2023
@Speiger
Copy link
Contributor

Speiger commented Sep 19, 2023

@TelepathicGrunt I am not a really big fan of the "translatable tag keys".
I get where you come from but on the other hand there is a easy solution to all this.
Mods simply can apply a pascal case reggex/formatter on to the resourcelocation system and it will look decent enough.
That is what i do for example for dimension Ids to name.
Simply treat: "_" as spaces and apply a first letter up logic and you get a good output out of 90% of the tags.

One thing that could be standardized is:
The option to allow for translations for the tags, but it should be "optional improvement" System and the default should just apply the pascal converter.
If it looks bad people can provide resource packs through CF/Modrinth that covers all the ones they want improved.

Here is the standard implementation that i use for ALL my mods.

	public static MutableComponent dimension(ResourceKey<Level> dimension) {
		String key = Util.makeDescriptionId("dimension", dimension.location());
		Language lang = Language.getInstance();
		return lang.has(key) ? translate(key) : literal(toPascalCase(dimension.location().getPath()));
	}
	
	public static String toPascalCase(String input) {
		StringBuilder builder = new StringBuilder();
		for(String s : input.replaceAll("_", " ").replaceAll("-", " ").split(" ")) {
			builder.append(firstLetterUppercase(s)).append(" ");
		}
		return builder.substring(0, builder.length() - 1);
	}

(Note this implementation expects the server knows the key at least in english, at least if you want to send server chat messages. If the logic is client sided executed it won't rely on the server knowing the translation)
(Edit: I know the toPascalCase implementation could be optimized a lot)

Edit2:
Forcing all devs to support something they don't want could lead to malicious compliance.
What stops the dev to just write a script that auto adds "unsupported" or pascal auto generated to all unknown tags by default to prevent this error from even showing up?
(Not saying that that will happen, but this middle ground suggestion has the upside to basically allow devs that want to implement translation support while others that don't want to to simply ignore it without feeling attacked for it)

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Sep 19, 2023

@Speiger counterpoint, EMI mod already standardized how the translation keys are done and already warns devs in dev environment about untranslated tags. So following EMI’s example, that’s the way to go. EMI’s standard is already quite common on Fabric so it doesn’t make sense to start a new competing standard out of nowhere that will cause clashes just like we are experiencing with tags. We are not repeating the same mistakes

Second, translations should be specified and not created through regex so people of other languages can actually make translations. Also remember, not everyone represents spaces in tag names with _ in their resourcelocations and some tag names don’t represent the tag itself like forge:rods actually being Sticks. We want to help people with accessibility too as narrator would perform much better with dedicated clean translations than guesswork regex and narrator will flop with resourcelocations itself

third, this isn’t forced on anyone. The dev has to opt-in to the neoforge config to even get the log warning about untranslated tags. And if someone malicious gives bad translations to try and mess with recipe viewers and stuff, that modder will just get bug reports or pack makers shipping a resourcepack to fix the translation.

The current desired standard is show resourcelocations by default and if a modder wishes to, the modder can provide a translation to show instead to help with immersion and narrator accessibility

@Speiger
Copy link
Contributor

Speiger commented Sep 19, 2023

@TelepathicGrunt

The current desired standard is show resourcelocations by default and if a modder wishes to, the modder can provide a translation to show instead to help with immersion and narrator accessibility

Your current implementation doesn't actually represent what you state.

Since you only provide a translation key and not a component you insinuate that the translation key always will work and make moddevs responsible in ensuring that the backup system works.

If you want to implement a "optional" standard then at least provide a implementation that also handles the "optional" part of it.

The rest of the answer has nothing to comment to.
It would just loop.

Anyways last answer from my side.

@eerussianguy
Copy link
Contributor

My one question is say I have a metal that’s not in vanilla but for consistency I include it — eg common:ingots/tin. I guess the solution is for every mod to translate it on their own (and it’s not like there’s much argument over the translation) but it’s worth thinking about

@Random832
Copy link
Contributor

Is it that important for the "non-pretty" option to be a resource location rather than a raw translation key? I don't see something like "itemtag.common.ingots.tin" as obviously worse than "#common:ingots/tin"

@shartte shartte force-pushed the Common-Tags-Refactor branch from c6b41e4 to 99ab012 Compare April 16, 2024 23:22
@Shadows-of-Fire Shadows-of-Fire added this to the 20.5 Stable Release milestone Apr 17, 2024
@sciwhiz12 sciwhiz12 added 1.20.5 Targeted at Minecraft 1.20.5 and removed 1.21 Targeted at Minecraft 1.21 labels Apr 17, 2024
Copy link
Member

@Matyrobbrt Matyrobbrt left a comment

Choose a reason for hiding this comment

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

You have more places where you use Neoforge with lowercase f. Please fix those too.

(Leaving review comments is painfully laggy, so I didn't do it for each one.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20.5 Targeted at Minecraft 1.20.5 breaking change Breaks binary compatibility enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.