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

Merged
merged 126 commits into from
Apr 16, 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!

Neoforge PR: neoforged/NeoForge#135

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 will be sticking with c namespace

  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.

  1. Datagen translations for modloader common tags under the key format of tag.<registry>.common.<tagname> where slashes are turned into periods. EDIT: Will be a future PR instead

  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)EDIT: Will be a future PR instead

Details And Reasons Of Each Goal:

Namespace choice:

Edit: We are sticking with c as we should not let this PR die over namespace concerns. It is important that modding ecosystems all agree on a single standard to make the lives of datapackers, modders, players, and pack makers easier. However, folder conventions will have to stay as I cannot get the other ecosystems to accept non-folder convention. So some work will still need to be done with fabric modder to migrate to new tags as well as spread the word to help clean up the currently polluted c namespace.

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

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. Edit: New commit now makes the old tag module marked as deprecated with comments redirecting to v2 of the tag module with the common tags. This means this PR is no longer a breaking change for code usage.

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

EDIT: Will be a future PR instead

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, TagUtil.getTagTranslationKey is added as a helper method that modders can use to make it easier to datagen tag translations. Just takes the TagKey and spits out what the lang key is.


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 old `c` tags to our new `c` tags that follows better conventions!
See classes under net.fabricmc.fabric.api.tag.convention.v2 package 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! The convention is `c` with nouns generally being plural and adjectives being singular.
You can disable this message by this system property to your runs: `-Dfabric-tag-conventions-v1.legacyTagWarning=SILENCED`.
To see individual legacy tags found, set the system property to `-Dfabric-tag-conventions-v1.legacyTagWarning=DEV_VERBOSE` instead. Default is `DEV_SHORT`.

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

Dev warning - Legacy Tags detected. Please migrate your old `c` tags to our new `c` tags that follows better conventions!
See classes under net.fabricmc.fabric.api.tag.convention.v2 package 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! The convention is `c` with nouns generally being plural and adjectives being singular.
You can disable this message by this system property to your runs: `-Dfabric-tag-conventions-v1.legacyTagWarning=SILENCED`.
To see individual legacy tags found, set the system property to `-Dfabric-tag-conventions-v1.legacyTagWarning=DEV_VERBOSE` instead. Default is `DEV_SHORT`.

Legacy tags:
    TagKey[minecraft:block / c:barrel]   ->  TagKey[minecraft:block / c:barrels] 
    TagKey[minecraft:item / c:potato]   ->  TagKey[minecraft:item / c:potatoes] 
    TagKey[minecraft:item / c:blue_dyes]   ->  TagKey[minecraft:item / c:dyes/blue] 

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. EDIT: Prod option removed

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 Fabric API's properties config file by setting log-untranslated-item-tag-warning 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 Fabric API's properties config file by setting log-untranslated-item-tag-warning 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.

  • The deprecated tag fields for buckets and tools were deleted since this PR is already a code breaking change. Might as well do a small cleanup. These can be added back if no code-breaking change is wanted.

  • Folders are added and many tags migrated to them.

  • block tag c:movement_restricted is renamed to c:relocation_not_supported since that tag name is what many forge mods were using and it covers teleporting blocks to another location. Movement restricted sounded like it was for piston pushing stuff when in reality, it is meant for any kind of block relocating behavior.

  • is_ prefix is added to the biome tags to match the Neoforge PR and how vanilla names their biome tags.

  • c:tools/bows was split into c:tools/bows and c:tools/crossbows because these are two separate tools and has different behaviors. Mods that don't care can pull from both tags. But mods that do now can pull exactly what they need. Also, c:tools/fishing_rods was added as well since it is a tool and I seen quite a few modded Fishing Rods made.

  • 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 Neoforge PR is ported over to this PR. I only ported the tags that I assumed would be very useful to have such as c:tools/crossbows. 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.

Neoforge PR: neoforged/NeoForge#135

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.

@apple502j
Copy link
Contributor

@TelepathicGrunt I recommend not dropping a breaking-change PR directly, without any prior discussion... I also wonder what's the Quilt's position on this.

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Sep 10, 2023

@apple502j I had some discussions already with some Fabric maintainers. But best shown in PR what the idea is and the PR can change if needed. As stated, if desired, I can adjust PR to not have the breaking changes with the fields but comes at cost of more tech debt.

Quilt pulls from Fabric so it doesn't matter if they want their own thing, they still have to be compatible with Fabric anyway. So discussion will be here if Quilt wants to talk about it.

Edit: If the concern is about the wording of "both loaders", it's because I consider Quilt as a subset of Fabric in terms of tag compatibility. So it wouldn't make sense to PR Quilt when they will pull this PR in anyway from Fabric. For the purpose of this discussion, Fabric/Quilt are considered the same ecosystem.

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

sylv256 commented Sep 10, 2023

Edit: If the concern is about the wording of "both loaders", it's because I consider Quilt as a subset of Fabric in terms of tag compatibility. So it wouldn't make sense to PR Quilt when they will pull this PR in anyway from Fabric. For the purpose of this discussion, Fabric/Quilt are considered the same ecosystem.

This is not true at all. Quilt can just not support this for Quilt-exclusive mods and make a hack so it converts back to c.

@TelepathicGrunt
Copy link
Contributor Author

This is not true at all. Quilt can just not support this for Quilt-exclusive mods and make a hack so it converts back to c.

If that is what Quilt ends up wanting to do, that is totally fine. I can't stop anyone or any modloader from what decision they ultimately choose. But lets focus back to this PR please.

@modmuss50
Copy link
Member

Please keep this discussion on topic, this is about what we are doing for Fabric. 👍

@Juuxel
Copy link
Member

Juuxel commented Sep 10, 2023

Second, the c namespace is in disaster in conventions at the moment.

AFAIK this is somewhat due to Forge->Fabric mod ports just changing the namespace and using those conventions on the wrong platform. I'm a bit worried that if a new namespace is introduced, people will just move their existing tags to a new namespace, further adding to the tag soup:

  • Proper common: tags (common:tools/pickaxes)
  • Old forge: tags (forge:tools/pickaxe -> common:tools/pickaxe)
  • Old c: tags (c:pickaxes -> common:pickaxes)

In my opinion, c is still a good namespace choice here due to

  • It not being a valid mod ID
  • The short name also making the namespace feel unimportant in tag (etc) IDs

I've also seen the c namespace being used in non-tag contexts such as #3129, so this change wouldn't only affect 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.

As far as I can tell, there are only three examples of folder tags:

  • mineable tags
  • Banner pattern item tags
  • has_structure tags

(This comes from a quick search through the Minecraft Wiki list of tags, it's possible I missed something)

Many "folderable" tags have been added in 1.19/1.20 (cherry wood, mob spawn tags for biomes etc) since the mineables and other folder tags were introduced, so it's not purely backwards compat.

Since the folder naming pattern is inconsistent with vanilla, I feel like a nice solution for that would be to support some sort of tag aliases where c:ingots/copper and c:copper_ingots could refer to the same tag.


Log warning for untranslated item tags presence

IMO this is a bit questionable since it affects a non-modding feature (data packs), making the modding API create log spam for untranslated tags that can be added by data packs – which can't provide translations in the first place.

@TelepathicGrunt
Copy link
Contributor Author

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

Copy link
Contributor

@haykam821 haykam821 left a comment

Choose a reason for hiding this comment

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

For consistency with Yarn, the most deeply nested element of a tag key should come first in its field name; for example, the minecraft:mineable/pickaxe tag key is assigned to the BlockTags.PICKAXE_MINEABLE field.

@dexman545
Copy link
Contributor

Just some thoughts, not a review, I also don't particularly care if tags are folders or not so long as the tags exist and are used.

c was chosen as it was (apparently) common for datapacks to use at the time. I don't know how true that is, but that was part of the logic for choosing it. Has that changed?

Currently, FAPI uses c to mean "conventional," hence the API name... it would be weird to have a "conventional tag api for common tags." I'd rather keep the c namespace.

I'm still not sure how the top level view of a datagenned datapack folder is of any consequence - especially when the API part is still "bloated." Maybe we need an API for getting tags within folders?

I'd rather have the compat issue be dealt with by tag aliases (some form of API that says tag A and B are the same).

@TelepathicGrunt TelepathicGrunt changed the title Unify tag namespace and conventions between Neoforge and Fabric for 1.21 MC Unify tag namespace and conventions between Neoforge and Fabric for 1.20.5 MC Apr 12, 2024
@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Apr 14, 2024
@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Apr 16, 2024
@modmuss50
Copy link
Member

Many thanks for this PR and all of the comments, I plan to merge this later today 🚀

@modmuss50 modmuss50 merged commit 7f945d5 into FabricMC:1.20.5 Apr 16, 2024
4 checks passed
Alexander01998 added a commit to Wurst-Imperium/Mo-Glass that referenced this pull request Apr 16, 2024
@quinn-semele
Copy link

Is there a reason why entity_defence_enchantment hasn't been changed to entity_defence_enchantments so that it lines up with what exists in NeoForge's version of this PR?

https://github.com/neoforged/NeoForge/pull/135/files#diff-8e486693a538d27b02d3ff401bb3236aaf4245732b99ffd4a9ca6b0907e8b194
https://github.com/FabricMC/fabric/pull/3310/files#diff-dbe9f80271edbdb0e61d5c5e25639f4c7011c0126ad090629d9037b6cc48b442

@TelepathicGrunt
Copy link
Contributor Author

@quinn-semele Just a typo that would need to be corrected to match the other fabric tags
image

MightyKnight added a commit to MightyKnight/ShieldDisruptor that referenced this pull request Apr 24, 2024
Tags were unified in 1.20.5 between mod loaders and now contain folders.
See PR: FabricMC/fabric#3310
Shield tag is now: #c:tools/shields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This issue is a good idea and a PR should be made enhancement New feature or request last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet