Fabric API

Fabric API

106M Downloads

Datapack tags always replace, even when they should append

YanisBft opened this issue ยท 26 comments

commented

API version: 0.1.4.71

Description:
My mod adds new walls. So I have to add these new walls to the vanilla block tag minecraft:walls.
With a "false" value in the "replace" field, my walls should append to the vanilla ones. But instead, they replace them.

commented

They don't, works just fine for me. Can you show debugging/screenshots or anything as evidence?

commented

See the image below:
On the left, the vanilla wall does not have "#minecraft:walls" block tag anymore.
On the right, my mod wall does have it, so the two walls can't connect to each other.
image

EDIT: You can also test my mod and see the issue directly (https://minecraft.curseforge.com/projects/fabriblocks)

commented

Did you try false instead of "false"?

commented

Blame json.

commented

I tested both @asiekierka

commented

This issue still happens with the latest API (0.2.4.112)

commented

I can take a look at this in a few hours.

commented

As I didn't say it before, it works fine in dev environment. The issue comes when using the mod after the build.

commented

Just ran into this issue as well. When adding vanilla tags to blocks, in the dev env, and when using "replace": "false", it works, but when using that and not in dev env, it does not work.

Using:

minecraft_version=1.14.2 Pre-Release 4
yarn_mappings=1.14.2 Pre-Release 4+build.1
loader_version=0.4.8+build.154
fabric_version=0.3.0-pre+build.167

My current solution to this is to add the vanilla blocks to my own vanilla tags, but that is not really maintanable

commented

I tested this with the mod Desire Paths using desire-paths-1.2.0.jar

When the mod is active, bamboo saplings cannot be planted on grass or sand blocks, it can only be planted on desire-paths blocks.

The file bamboo_plantable_on.json inside the jar contains this:

{
  "replace": false,
  "values": [
    "desire-paths:grass_dirt_inter",
    "desire-paths:mycelium_dirt_inter",
    "desire-paths:podzol_dirt_inter",
    "desire-paths:dirt_coarse_inter"
  ]
}

If the file is edited like this:

{
  "replace": false,
  "values": [
    "desire-paths:grass_dirt_inter",
    "desire-paths:mycelium_dirt_inter",
    "desire-paths:podzol_dirt_inter",
    "desire-paths:dirt_coarse_inter",
    "minecraft:grass_block",
    "minecraft:sand"
  ]
}

Then bamboo saplings can be planted on grass and sand blocks.

So it seems that "replace": false is not being honored.

It was mentioned on Discord that this happens only in production and not in dev env.

commented

My testing on api 0.4.0 shows that this issue applies depending on some variable. The tags vanilla tags missing in eclipse but when mod is compiled and on an actual client there are no issues.

commented

This is definitely still an issue, now only in dev and not in production:

In dev:
image

In production:
image

commented

how about gradlew runclient?

commented

Using gradlew runclient it also replaces:

This happens with bamboo_plantable_on.json as such:

{
  "replace": false,
  "values": [
    "desire-paths:grass_dirt_inter",
    "desire-paths:mycelium_dirt_inter",
    "desire-paths:podzol_dirt_inter",
    "desire-paths:dirt_coarse_inter",


    "minecraft:bamboo",
    "minecraft:bamboo_sapling",
    "minecraft:gravel",
    "#minecraft:sand",
    "#minecraft:dirt_like"
  ]
}

With bamboo_plantable_on.json as such:

{
  "replace": false,
  "values": [
    "desire-paths:grass_dirt_inter",
    "desire-paths:mycelium_dirt_inter",
    "desire-paths:podzol_dirt_inter",
    "desire-paths:dirt_coarse_inter"
  ]
}

commented

so this may either be resource loader bug or loom bug. last time it was loom not loading vanilla resource when mod resource is present.

commented

Still a bug with Fabric API 0.3.0 build 183

commented

I still have this issue.

commented

I've noticed no change after applying @Chocohead 's fix. Also in my case, it doesn't just "replace", but rather only vanilla blocks added to tags are recognized on the debug screen as containing the tag, but not by crafting recipes or by the clear command. Mod blocks that are supposed to contain the tag do not have it at all, for all I can see. This is the same with or without the "fix".

commented

Seems to still be happening on latest snapshot/fabric api. Tried adding my blocks to the new piglin item / block data tags but once loaded only my blocks / items are in that tag despite replace being false.

Note: I haven't tested chocoheads fix.

Edit: Issue only happens in dev.

commented

Can confirm still having this issue trying to work with the climbable tag.

commented

the climbable tag works fine for me

commented

tag works- the โ€œfalseโ€ parameter doesnโ€™t work correctly. you must put the value in quotes.

commented

Could this be fixed? I rely on tags to test things in my mod and it's impossible to see what it should actually be if the tags are replaced. Thanks!

commented

This problem still exists for

minecraft_version = 1.16.3
loader_version = 0.9.3+build.207
fabric_version = 0.21.0+build.407-1.16
commented

This is a classpath ordering problem, although a much more subtle one than expected:

Firstly, it's important to remember KnotClassLoader operates as an inverse delegating classloader. That is the resources (and therefore classes) it knows about will come before anything on the parent classloader. The reason behind this is to allow Mixin to be applied to all the classes it is aware of, at the expense of the chance for accidents of course.

Next is to look at how Tags load. RegistryTagManager is responsible for reloading tags whenever a new save is loaded, each tag type is handled by its own RegistryTagContainer. The relevant loading to this problem happens in TagContainer#prepareReload's lambda. It will search for all the JSONs which are in the appropriate data asset directory, then load the declarations the resource manager is aware of for each distinct tag name. These declarations are found via ReloadableResourceManagerImpl#getAllResources, which will use NamespaceResourceManager#getAllResources for the appropriate namespace a particular tag name comes from. For example, #minecraft:logs will search all declarations of tags/blocks/logs.json in the minecraft namespace. From here each ResourcePack (which has at least one asset in the namespace) will handle providing what they have, and it is here that the culprit is made clear. The class to blame for all this is DefaultResourcePack.

DefaultResourcePack is the ResourcePack responsible for the asset in the Minecraft jar. Its subclass DefaultClientResourcePack is also responsible for searching the asset index for anything the jar doesn't have (such as sounds, which are handled fine coincidentally). Because Minecraft is designed around being the only asset provider properly on the classpath it makes some sweeping presumptions about where it can find these assets. All asset loading is done either via DefaultResourcePack.class.getResource or DefaultResourcePack.class.getResourceAsStream. Both of which happen to be completely dependent on KnotClassLoader's choice for which resource it picks.

But Choco, you say, the first jar KnotClassLoader knows about is always the GameProvider (ie the Minecraft jar). And for that you are right. Yet there is now two complexities to this problem. The first is what happens if the jar doesn't contain a resource yet another mod does, and the second to why tag loading is broken in dev.

The first complexity is simple, the asset is duplicated. The default pack claims to have it, but will return an identical stream to the real ModNioResourcePack (or indeed just leak something out of a mod jar even if it didn't have its own resource pack given).

The second complexity, is brought about from the Mixin 7495ea2 added for DefaultResourcePack. It goes through every duplicate instance of an asset on the classpath (through KnotClassLoader reverse delegating) then presumes that the last instance is also from the Minecraft jar. Which for me is true, unless I happen to change the classpath ordering. Then it isn't. So if my mod assets are last, they will appear to be the Minecraft jar's assets and load instance (very similar to the first complexity). My assets will only be last of course if they're on the boot classpath to start with, which is only true when an IDE has put them there. Hence a dev only problem.

It is worth mentioning, perhaps as an aside, that the Mixin fix also happens to crash out DirectoryResourcePack#isValidPath when a resource is from a jar rather than shadow Minecraft's check in DefaultResourcePack#isValidUrl. DefaultResourcePack might have changed since it was added, either way it's why found is outside of the try block (and why there's two tries to start with).

The fix I came up with is to replace MixinDefaultResourcePack with

@Mixin(DefaultResourcePack.class)
abstract class DefaultResourcePackFix {
	@Shadow
	private static @Final Map<ResourceType, FileSystem> typeToFileSystem;

	@Inject(method = "findInputStream", at = @At("HEAD"), cancellable = true)
	private void onFindInputStream(ResourceType resourceType, Identifier identifier, CallbackInfoReturnable<InputStream> callback) {
		if (DefaultResourcePack.resourcePath != null) {
			// Fall through to Vanilla logic, they have a special case here.
			return;
		}

		FileSystem fs = typeToFileSystem.get(resourceType);
		if (fs == null) return; // Apparently Minecraft couldn't find its own resources, they'll be an error in the log for this

		Path path = fs.getPath(resourceType.getDirectory(), identifier.getNamespace(), identifier.getPath());
		if (path != null && Files.isRegularFile(path)) {
			try {
				callback.setReturnValue(Files.newInputStream(path));
			} catch (IOException e) {
				// Something went wrong, vanilla doesn't log these errors though
			}
		}
	}

	@Inject(method = "contains", at = @At("HEAD"), cancellable = true)
	private void fixContains(ResourceType type, Identifier id, CallbackInfoReturnable<Boolean> callback) {
		if (DefaultResourcePack.resourcePath != null) return;

		FileSystem fs = typeToFileSystem.get(type);
		if (fs == null) return;

		Path path = fs.getPath(type.getDirectory(), id.getNamespace(), id.getPath());
		if (path != null) callback.setReturnValue(Files.isRegularFile(path));
	}
}

Haven't tested it outside of dev but I can't give you all the answers in one night :P The first complexity is not entirely fixed as DefaultResourcePack#openRoot(String) might find a resource from another URL if not present in the Minecraft jar, but fixing that would require actually finding the real jar ourselves rather than using what Mojang helpfully already provided.

commented

The ability to finely control internal resource pack ordering is blocking for an issue on my mod magneticflux-/fabric-diagonal-panes#15. I think this issue is closely related, and could be solved at the same time if we clean up resource loading a bit more.