Minecraftforge: [1.13] Extend the Tag system to cover Forge Registries

[MajorTuvok]

Supersedes #5020. There have been several changes to the proposal but because the originial issue-creator doesn't seem to want to PR his proposal (though It might take a bit til I'm ready to PR this), I'm creating this Issue here.

In 1.13 the Tag System allows blocks, items and functions to be tagged, this however does not extend to Entities.

As discussed in #5020 it seems like a good idea to have a system which allows Modders to categorize Enitities like f.e. fire, water, air, flying, etc. Extending the exisiting Tag system has these benifits as far as I can see:

• Modders will be able to categorize Entities properly allowing them to adapt their behaviour accordingly (like f.e. increase damage of a fireball, when hitting a water entity)
• Users will be able to reference Entity-Tags in functions and commands, which might make others lives easier
• Server owners (and maybe Modpack authors) will be able to configure behaviour changes in their games (like declaring certain entities with additional tags, or adding mod-compatibilty tags (depending on how far the PR goes, one could even change things like suddenly make an entity able to breathe underwater))

This would not be able to prevent people from adding logically each other exculding tags (like f.e. attack/melee and attack/none), but I don't think that anyone who doesn't deliberatly want to destroy the game would add such contrasts...

So what do you guys think? Should this be made a PR when 1.13 forge code rolls out?

Edit:
After some Feedback I came to the conclusion, that it might be a good idea to not only extend this to Entities (or EntityEntry's to be accurate) but to ForgeRegistries as a whole (though it would be disabled by default).

The corresponding PR would then add an enableTagging() to the RegistryBuilder. This would also then result in tags being loaded for all ForgeRegistry's (Let's discuss below which should be excluded (SoundEvent and VillagerProfession? Though we could simply add no tags and let modders decide, whether they need it)), redirecting vanilla tagloading and tagsyncing to these (Items, Blocks and Functions will be loaded and synced the vanilla way, but the data will then be redirected to ForgeRegistry's instead of whatever Vanilla uses to store it...). This should ensure that Vanilla-Client-Forge-Server and the other way around will keep working correctly (as far as I know, Forge already has a mechanism, which sync's it's registries only with Forge-Forge connections, right?).

Benefit:
Not only could people do things as descirbed above and below with Entities, but things like f.e.:

• wands working better in Biomes tagged with magic
• only allowing enchantment of Items if the Enchantment has certain tags
• manipulating effects of potions with certain tags
• etc.
  ... (I think cross-Mod compatibilty in general will benefit of this)

I'm sure you guys have more ideas than I have, what can be done with that...

To be clear my PR will only address adding it to ForgeRegistries and doing some Entity compatibility within Vanilla code - for more I'd ask other people to find the places to add these to and submit PR's of their own (or tell me whilst I'm working on my PR, I can add it then), because I don't have the time to find every possible place for compatibilty with tags (I'm not even sure, whether I'll manage that with Entities, but I'll try)...

[covers1624]

Extending the tag system to entities is a wonderful idea, its in line with the data driven way things are moving to, and has the flexibility (as stated above) to allow mod pack makers & server owners to define their own because data pack's (unless i missed something about those).
Uses could even extend to /killall type commands, or even a mob 'Grinder' only operating on entities with specific tags, hostile, peaceful, farm_animals, fire, water. +1 on this.

[williewillus]

+1 for this
From what I can tell digging around 1.13, patching a new tag type + loading it + checks for it shouldn't actually take many lines.

The thing that needs to be checked though is how vanilla client responds to receiving tags it doesn't know about.

[liach]

i agree. btw is the tag applied to entity types or individual entities?

[williewillus]

entity types, the vanilla Tag system is designed for "type-level" tagging. E.g. tagging items, not itemstacks.

also, I did further investigation in 1.13. The packet type that syncs tags to the client is only designed to sync a hardcoded set of vanilla tags (blocks, items, fluids). All other tag types (e.g. functions) are not synced and would need a new packet type to be synced (so not vanilla compatible depending on how vanilla handles packets it doesn't understand). So this would be server-side only for now until mojang adds it themselves

[liach]

Since it's applied to types, I have no more concern about performance and is totally supportive of this proposal (or types for all forge registries?)

[MajorTuvok]

That's true one could propably do this for all forge registries if one wants too, but I don't really see the use of f. e. Tagged Biomes.

What might be useful in my eyes is tagging Potiontypes and Enchantments. But I think that this would be overkill...

Edit: When I'm thinking over it - one could add that to the registry system... (I have an idea on how to implement that, as long as vanilla clients don't care about extra packets or as long as forge notices that it is connected to a vanilla client...)

[MajorTuvok]

@williewillus where did you get the 1.13 sources from? Did you manually decompile Minecraft or am I missing something?

[MajorTuvok]

And couldn't we try to sync these tags only when connected to a forge client?
(I'm sorry, I'm not really informed of the inner workings of forge and have never done a PR so far)

[temp1011]

@MajorTuvok Biomes have a type though. Maybe that could be moved to a tag?

[MajorTuvok]

That's true thanks @temp1011...

Though I wouldn't go as far as move it to a tag, but rather append a tag (like for example that it's a magical biome or that it hasn't air...). I have the little problem that I don't know much about biomes and therefore don't even know what biome types are meant for, but I'll look that up later on.

[williewillus]

@liach good point. Maybe in 1.13 we can finally get hot load/unload of registry entries like items? Fingers crossed.

[MajorTuvok]

Btw. Are there any other suggestions at what should gain Tags, besides Entities?
(And what do you guys think about adding to Potiontypes, Enchantments and biomes?)

[Wyn-Price]

I'm not sure how feasible this idea is, but having a way to tag all forge registeries (including modded ones) sounds pretty useful. Maybe in the registery builder there could be an option to enable/disable tagging?

[MajorTuvok]

Ok, cause there also seem to be modderes agreeing with this (and not only my Minecraft playing but not programming friends) I'll modify this issue correspondantly...

[MajorTuvok]

I have a question, I just stumbled across BiomeDictonary.Type - wouldn't they be obsolete if Biomes could be tagged? (Replacing BiomeDictonary with tags...? (That be a pain to PR (I have done nothing with Biomes at all...)... pls volunteer guys or prove me wrong!))

(We can naturally keep both at the same time, but that would make it a pain to use - you'd always have to query both in order to get all the information you might want)

[Wyn-Price]

BiomeDictionary.Type can have subtypes. Thats about all I can see that's different between it and the tags

[MajorTuvok]

Well it is possible to create a tree structure with tags and adding utility Methods for using tags like that, shouldn't be too bad...

But what are those subtypes used for?

[liach]

I think a direct acyclic graph structure works better than a tree in this case (same goes for advancements) the subtypes are used for better identification.

[Wyn-Price]

When tag X is added to a certain biome, it checks all types to see which have sub-types of tag X. Those types are also added to the list of tags. It explains here https://github.com/MinecraftForge/MinecraftForge/blob/0cf5ef221cd6d7e850b848d6c1ff537a48921edd/src/main/java/net/minecraftforge/common/BiomeDictionary.java#L78-L81

[liach]

The current design of biome tags is already compatible with direct acyclic graph. I don't think the tags are designed to have any inheritance though.

[MajorTuvok]

ok, so we'll leave Biomes out of the tag System, and keep the Biome Dictonary as is. Thanks @Wyn-Price and @liach ...

[bs2609]

Tags can include other tags as far as I'm aware.

[Wyn-Price]

@bs2609 yeah they can. Vanilla uses it to have the tag minecraft:wooden_doors in the minecraft:doors tag.

[liach]

Great. Apparently the tags do support DAG, so it can indeed replace the biome type.

[liach]

From Minecraft wiki:

To utilize block, item, or function tags the JSON files must be placed in a data pack inside the data/(namespace)/tags/blocks, data/(namespace)/tags/items or data/(namespace)/tags/functions folder respectively.

Since Forge adds resource locations (namespaced strings) for Forge registries, what will the folder names look like? For example, if I have a registry named mymod:myregistry, will the folder be data/(namespace)/tags/mymod/myregistry? My concern is that if there is a folder named data/(namespace)/tags/mymod in which the tag file is located, the registry referred to may be minecraft:mymod instead, creating confusion.

[MajorTuvok]

I would have used the namespace as the namespace for the registry. In your example:
data/mymod/tags/myregistry

That would not create confusion, would it?

[MajorTuvok]

Ah know I see why this might be a problem - other mods would then put things in your folder...
Ok, I see the problem now (And will try to think, before writing next time)

[Wyn-Price]

I don't see anything wrong with data/(namespace)/tags/mymod/myregistry. That was exactly the folder structure I had in mind for something like this.

[MajorTuvok]

We should do the really long folder then: data/[data-namespace-owner] /tags/[registry-namespace] /[registry-name]

Yep, @Wyn-Price

This would also lead to me having to modify vanilla saving/loading (in order to preserve compatability with vanilla data packs I'd assume minecraft, if a registry-name appears outside a namespaced folder) ...

[liach]

The vanilla block registry has a Forge id of minecraft:blocks. As a result of data/(namespace)/tags/mymod/myregistry approach, forge will load from data/(namespace)/tags/minecraft/blocks folder while it needs to check data/(namespace)/tags/blocks at the same time. Should we allow skipping minecraft: in namespace in folder scanning or should we keep blocks, items, fluids, and functions as special cases?

[MajorTuvok]

And I would have allowed skipping minecraft, so that people could still use vanilla data packs without us having to hardcode something (I forgot to apply my edit, sry)

[Wyn-Price]

Rather than have special cases, I think we should skip all minecraft in folder scanning, however if people think otherwise, I wouldn't mind a special case system

[MajorTuvok]

I think that for consitency reasons we should allow people who want to organize a bit more to also add the minecraft sub folder. Having all standard forgeregistries directly below tags and then seperate folders for mods would be really inconsistent (though if they truly want to, they can do it) ...

[Wyn-Price]

But it would be more inconsistent to have the item/block/fluid tags in the "root" folder, then the rest of the minecraft: registeries in a folder.

[MajorTuvok]

Which is not what I'm saying - you could do as pleases you. Any folder who does not belong to a modid would be interpreted as a minecraft:registry-name folder. People could still place ALL under a seperate minecraft folder.

In theory people could also do the inconsitency you said, but that would be their choice then and not something we forced them into doing.

Depending on the implementation it might be possible to have 2 folders for minecraft registries at the same time, though I don't see why we should allow people to become that un-organized.

[Wyn-Price]

The only issue I see with having the folder structure as discussed, is that if a registery is called blocks:myreg, the files will have to go in data/(namespace)/tags/blocks/myreg, meaning they'd be loaded by the blocks tag parser

[Wyn-Price]

To solve that, we'd either have to enforce all vanilla tags in a minecraft/ folder (bad idea, will break compatibly with vanilla) or move the forge registeries tags to a further subfolder or different location. If anyone has any other ideas on how to solve this, then go ahead, as I'm sure I've overlooked something

[Wyn-Price]

Something like data/(namespace)/tags/forge/mymod/myregistry or data/(namespace)/forge-tags/mymod/myregistry or something similar. Doing that should solve that problem. I definitely think we should not change the vanilla stuff at all. Let it all be on data/(namespace)/tags

[MajorTuvok]

What I'm suggesting is the following loading order below the tags folder:

• try to find a modid (minecraft counts as a mod in that case) for the given folder
• on success interpret all sub-folders as belonging to registries of that mod
• on failure try to find a minecraft:? registry with the name of the folder
• if that fails too, print an error message

[MajorTuvok]

The vanilla mc stuff could then be loaded as normal, or in a seperate folder...

[Wyn-Price]

The system shouldn't be look at all the folders and try to match a registery to it, it should be go through all the registeries and try and find a folder for it.

[MajorTuvok]

This would then result in no warnings for skipped folders which might annoy people if the make a typo.

If you are concerned about log-spam, we could make logging it a config option.

[Wyn-Price]

If people make a typo them it won't load the folder. That's on then to fix the typo.

[MajorTuvok]

Yeah, but first they have to know that they made a typo...

Cause usually if you make a typo yourself you'll continue reading the correct statement multiple times even if it is not correct. You'd then wonder what the hell is going on. (at least that's what happened to me quite often)

[Wyn-Price]

That's still on the user to fix. Having directory scanning then matching the directory to a registery just to solve typos is not the way to go. If you need to help users and typos, it can have it log a warning if it finds no tag files to load, but even then I don't think that's necessary.

[MajorTuvok]

Very well.

I still think that we should scan directories (and if it is only to log which ones are skipped) but because I do understand, that it is the duty of the registry to say what's their stuff and should therefore be loaded - I can see your point too.

Thing is it might also be easier to implement with the current system. If I have understood resource Loading correctly I'd have to add a ResourceLoader too and I don't know whether one can easily iterate over the registries (withou exposing it puplicly) to find out which folder they want to be loaded.
But that might change with their forge rewrites - so let's see what's in the codebase.

Anyway I still think allowing minecraft as the default namespace is the way to go. It is defaulted in ResourceLocations that way and gives people the opportunity to organize their tags like they want (either in a seperate folder, or directly (which will propably preferred on servers which allow vanilla clients, cause they don't need to navigate through yet another folder)) .

We could propably mix the way's of loading it by going through folders and asking registries, whether they want to loas their tags from that folder. We could even allow to insert a callback to allow modders to specify their loading folders.

[Wyn-Price]

I've looked at the tag system as it is, and you don't need to create a ResourceLoader. The registery system is actually pretty flexible at the moment, and having the forge registeries include tags doesn't seem to hard to do. Syncing between client / server is already done, loading the tags is already done. You don't need to overcomplicate it by scanning the directories. If a user makes a typo, that is their problem, not ours. If they can't find their own problem, they'll ask on irc/fourms/discords and people will spot the typo.

[Wyn-Price]

The tag folders shouldn't be customized. That just reintroduces the overlapping issues we had before.

[MajorTuvok]

Though I didn't think that I'd use the vanilla way of syncing tags (according to comments above, they are hardcoded after all) - but rather sync them together with the registries and fallback to vanilla behaviour if there is a vanilla client connected (and delegate the vanilla packets through to the corresponding registries if connected to a vanilla server).

I didn't know that it would be so easy to load the tags... :)
(I thought that I'd have to discard the vanilla loading and replace it...)

As you can see I'm not really informed of the inner workings of forge or minecraft - sry for that.

[Wyn-Price]

I really don't think scanning through all tag directories and matching them up to a forge registery is the way to go. It's much easier to get the forge registeries, and have it look in the correct folder (easier due to the way the tag system works). There's no need to look over all the directories. Having it error when it finds an unknown folder would mean that a mod that adds support for another mods registery, and adds tags for that registery would error when the registery mod isn't loaded in the game. You have to remember, there may be tag files for forge registeries that aren't in game. No mod should be penalized for simply having files in a certain location.

[Wyn-Price]

Syncing tags is really easy the vanilla way, you'd just have to make a few changes to the system to allow for an unknown number of tag types

[MajorTuvok]

Sry, I forgot to delete my previous comment - I thought the second one would indicate that I agree with you (I first overlooked your first comment).

How are you guys able to look at 1.13 src-Code without a 1.13 forge?!?

I'll do some research know and see whether I can have a look at it myself - cause first time someone said hardcoded I thought they created seperate classes for each type of tag and sperate packets for each type of tag (which would have required me writing an abstract base class handling all these...).
But what you are saying seems like it is indeed extendable...

You're wording implies, that vanilla tags are synced in one single packet - so I'd only have to make sure, that forge-tags are only added to said packet if connected to another forge instance and that's it...

That sounds much, much easier than I thought it is.

[Wyn-Price]

They are on one packet. There aren't technically separate classes for each tag, although there are separate wrappers (which can be compressed into one class, it's just easier for vanilla to have it how it is). When a 1.13 forge comes out, I might also have a shot at a PR for this

[MajorTuvok]

So you're comment indicates, that you are able to see MC-1.13 src code, and that there is a non-stable forge somewhere out there, which I can't seem to find...

Cause this is not relevant for the issue, I'll ask on the forums and won't read or post anything to this issue until I've been abe to find out what's actually going on in the 1.13 sources.

[Wyn-Price]

To see 1.13 code there doesn't have to be a unstable forge. You can see code without forge. You just need a tool called MCPConfig

[MajorTuvok]

ok against my own comment - I'll write somthing.

I do know that it exists - I have no idea how it works (I've seen quite a few people posting really complex ways of using it, which is why I've feared it a bit). Though I'll try later on.

[MajorTuvok]

Ok, so I just looked into github from my vacation and saw that there is some 1.13-pre out there to create a PR for, right?
But should any feature PR for 1.13-pre even be proposed, or should they still keep waiting until there is a buildable/running forge?

So I'll just prepare that when I'm back home again - I might have it ready somewhere next week (if there are no protest against proposing the PR, I'll just open it then - a PR can stay open afterall)

[MajorTuvok]

If anyone wants to have a look at what I'm doing: here it is

I won't open a PR (mainly for discussion) just yet, because It looks like I'm unable to run forge yet (at least runClient doesn't work (fails with an StackOverflow if that helps cpw))

[MajorTuvok]

What about the generation of the data folder?
Should Forgeregistries hook into that?

[MajorTuvok]

And what about multiple Tags?
Say multiple mods would like to add it's own (f.e.) entities to an Forge provided tag, should we allow an additional 'add' field in the Tag specification or rather favor everyone creating different tags, but at runtime the namespace get's ignored?

[liach]

@MajorTuvok There is already a replace field which allows whether tag entries are added or totally replaced.

IMO we need special handling of lookup paths for tags because forge registries are namespaced, unlike vanilla ones.

[MajorTuvok]

@liach
The handling of lookup paths I'm doing now is as follows:
Registries with minecraft namespace:
namespace/tags/reg-name
namespace/tags/minecraft/reg-name
namespace/tags/forge/minecraft/reg-name
Registries with other namespaces
namespace/tags/forge/reg-namespace/reg-name

What I meant was, should forge copy the forge provided tags (and ideally modded ones) to the data folder like Vanilla is able to do (see net.minecraft.data.Main for reference) or just leave vanilla to it's stuff.
Edit:
Hold on, now I get what you are referring to...´
We could build top-level-tags out of the combination of all namespaced-tags once they are loaded...
perhaps under the Minecraft namespace?

[MajorTuvok]

For example run this after the Tags have been loaded into the map, but before they were build:
private void collectToMinecraftTags(Map> map) {

        Map<String, Set<ResourceLocation>> flattener = Maps.newHashMap();
        for (Map.Entry<ResourceLocation,Tag.Builder<T>> unbuildTag: map.entrySet())
        {
            if (!unbuildTag.getKey().getNamespace().equals("minecraft"))
            {
                Set<ResourceLocation> current = flattener.getOrDefault(unbuildTag.getKey().getPath(),new HashSet<>());
                current.add(unbuildTag.getKey());
                flattener.put(unbuildTag.getKey().getPath(), current);
            }
        }
        for (Map.Entry<String,Set<ResourceLocation>> flattenedEntry:flattener.entrySet())
        {
            ResourceLocation collected = new ResourceLocation("minecraft:"+flattenedEntry.getKey());
            Tag.Builder<T> builder = map.get(collected); //ensure that we don't delete existing minecraft tags
            map.put(collected, TagHelper.addAll(builder!=null?builder:Tag.Builder.create(),flattenedEntry.getValue()));
        }
    }

[MajorTuvok]

Will be closed by #5167