[BUG] ? Not working <ore:craftingToolHardHammer>

[ghost]

Describe the bug
I'm making my own crafting recipes via CraftTweaker and I'm using gregtech materials to make some recipes harder.

Versions
Forge: 14.23.5.2855
GTCE: 1.12.2-1.17.1.770
IC2 Classic: 1.12-1.5.5.2.1
CraftTweaker: 1.12-4.1.20.663

Setup
Playing Solo
New world generated

Expected behavior
My recipe should say via JustEnoughItems when I click let's say on "Iron Fence" (from IC2 Classic) it should require any crafted hammer to make that item but it accepts only "Darmstadium Hammer" but I set up in recipe this ore:craftingToolHardHammer so it should say that I can craft it with any hammer but it's not working. When I'm using ore:craftingToolScrewdriver it is working but for some reason ore:craftingToolHardHammer isn't.

Basically I removed IC2 Classic crafting recipe for Iron Fences via their config file and than added my own recipe via CraftTweaker which is looking like this:
//IronFence
recipes.addShaped(<ic2:blockfenceiron> *6, [

[<ore:craftingToolScrewdriver>, null, <ore:craftingToolHardHammer>],
[<gregtech:meta_item_1:12184>, <gregtech:meta_item_1:12184>, <gregtech:meta_item_1:12184>],
[<gregtech:meta_item_1:12184>, <gregtech:meta_item_1:12184>, <gregtech:meta_item_1:12184>]]);`

And the "<ore:craftingToolHardHammer>" isn't working :(

[warjort]

When you say it is not working, do you mean:

• it is not cycling through all the different materials that a hammer can be made from
• or only the Darmstadium hammer is working in the crafting table?

The first of those is the "intended" behaviour.

The way GTCE tools work, is they are all really the same item. They are differentiated internally by their material id.
But when it comes to registering with the ore dictionary, they are only registered with Darmstadium as the material (forge and JEI know nothing of the material id so GTCE has to choose a material to make something concrete for them to use).

GregTech/src/main/java/gregtech/api/items/toolitem/ToolMetaItem.java

Line 648 in 7230afe

OreDictionary.registerOre(oreDict.name(), getStackForm());

which eventually calls:

GregTech/src/main/java/gregtech/api/items/toolitem/ToolMetaItem.java

Lines 662 to 667 in 7230afe

	@Override
	public ItemStack getStackForm(int amount) {
	ItemStack rawStack = super.getStackForm(amount);
	setToolMaterial(rawStack, Materials.Darmstadtium);
	return rawStack;
	}

This shouldn't stop other material's tools being useable for real crafting, but it does mean JEI only knows about and shows the Darmstadium tool in the ore dictionary.

Note: I assume when you say the screwdriver is working, what you are actually seeing is it cycling through the normal and electric screwdriver and not every material?

See also: #1505

[idcppl]

This is an issue between Forge and CraftTweaker. CraftTweaker doesn't respect NBT in OreDicts. It could easily be fixed on our side, by changing the OreDict to have a empty NBT and changing this method to return Darmstadtium.

GregTech/src/main/java/gregtech/api/items/toolitem/ToolMetaItem.java

Lines 588 to 606 in 7230afe

	public static SolidMaterial getToolMaterial(ItemStack itemStack) {
	NBTTagCompound statsTag = getToolStatsTag(itemStack);
	if (statsTag == null) {
	return Materials.Aluminium;
	}
	String toolMaterialName;
	if (statsTag.hasKey("Material")) {
	toolMaterialName = statsTag.getString("Material");
	} else if (statsTag.hasKey("PrimaryMaterial")) {
	toolMaterialName = statsTag.getString("PrimaryMaterial");
	} else {
	return Materials.Aluminium;
	}
	Material material = Material.MATERIAL_REGISTRY.getObject(toolMaterialName);
	if (material instanceof SolidMaterial) {
	return (SolidMaterial) material;
	}
	return Materials.Aluminium;
	}

If you're looking for a quick fix, this can help.

https://gist.github.com/idcppl/c29120b0cbe7ab81fd329eae8802a244

[ghost]

By not working I mean only the Darmstadium hammer is working in the crafting table, I mean .. I dont need to every recipe to be cycling every hammer but idc if it would. I only want to be able craft with every hammer .. like I could craft my recipe for Iron Fences with any hammer (bronze,steel,iron and etc.)

//EDIT: Would be nice if you could fix it inside your mod please :(, in the mean time I'm gonna try that quick fix.
//EDIT2: Your quick fix worked, thanks a lot!

[Exaxxion]

The workaround @idcppl mentions is the same approach we use in Omnifactory for custom recipes involving tool groups (Omnifactory _oreDict.zs). Not sure what he means by "This is an issue between Forge and CraftTweaker" though (seems Jared also took umbrage with that comment on the linked CT issue).

My assumption is that it has to do with the properties of the material being stored in the item's NBT, as the workaround involves recreating the dictionaries with untagged items via CT. When the recipes are registered by GTCE with those dictionaries, they work with all tools from the group as one would expect; trying to do the same with CT it gets weird on you.

[jaredlll08]

(seems Jared also took umbrage with that comment on the linked CT issue)

Well yea, I do take issue with you guys trying to throw me under the bus when you are the ones abusing the OreDictionary.

As said by Mezz ((Former?) Forge Developer) here: MinecraftForge/MinecraftForge#1446 (comment), the OreDictionary is not designed to work with NBT.

This is backed up by the code:
https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L669-L694

Which uses an Item (Not ItemStack. So no NBT) to generate a hash to prevent duplicate Items in a single oredict.

As for:

CraftTweaker doesn't respect NBT in OreDicts.

We get all the items in the OreDictionary for the specified name:
https://github.com/CraftTweaker/CraftTweaker/blob/1.12/CraftTweaker2-MC1120-Main/src/main/java/crafttweaker/mc1120/oredict/MCOreDictEntry.java#L188

We convert them to our IItemStacks (Which supports NBT):
https://github.com/CraftTweaker/CraftTweaker/blob/1.12/CraftTweaker2-MC1120-Main/src/main/java/crafttweaker/api/minecraft/CraftTweakerMC.java#L224-L229

Which calls this to make the actual item:
https://github.com/CraftTweaker/CraftTweaker/blob/1.12/CraftTweaker2-MC1120-Main/src/main/java/crafttweaker/mc1120/item/MCItemStack.java#L78-L90

At no point do we throw out the NBT of the stack, we take the data that is provided by Forge and convert it to our own format.

If Forge doesn't provide us the correct NBT (because as shown above, Forge's OreDictionary isn't designed to work with NBT), then I don't see how we can just magic the correct NBT out of thin air.

As for comments made by @warjort here: CraftTweaker/CraftTweaker#1378 (comment)

But trying to reference that ore dictionary in a crafttweaker recipe will lead to the recipe only accepting items that have matching NBT.

Once again, CraftTweaker deals with what it is given by Forge, if for some reason Forge gives us an item with NBT, we are going to match against that NBT, we can't know that the item needs it's NBT cleared, and clearing the NBT on all items that we get from the OreDict would be wasted cpu cycles since only GregTech items require special handling.

[warjort]

when you are the ones abusing the OreDictionary.

I am not sure GTCE is abusing the OreDictionary. It is expecting the NBT to be ignored "as per spec".

if for some reason Forge gives us an item with NBT, we are going to match against that NBT, we can't know that the item needs it's NBT cleared, and clearing the NBT on all items that we get from the OreDict would be wasted cpu cycles since only GregTech items require special handling.

But isn't the underlying issue that forge does ignore the NBT (even if still has a reference to it in the item stack)
but CraftTweaker does not?
This isn't special handling, it is what is required for consistency between Forge and CraftTweaker.

[Rongmario]

I do find it funny that a CT dev out of all people is talking about wasted cpu cycles

[Exaxxion]

Thank you for taking the time explain, Jared. I think idcppl was just incorrect and not necessarily malicious.

I'm not familiar enough with the spec to comment definitively without further research, but there may be something we can do on our end. Perhaps registering the tool ore dictionaries with untagged items, like what we do for the workaround?

@Rongmario, banter doesn't really help in the resolution of this issue and will just make people annoyed at each other. It's not productive.

[warjort]

I'm not familiar enough with the spec to comment definitively without further research, but there may be something we can do on our end. Perhaps registering the tool ore dictionaries with untagged items, like what we do for the workaround?

That is what is suggested above and I mentioned here with its downsides: CraftTweaker/CraftTweaker#1378 (comment)

There is the further issue within GTCE that tools are registered with default material "Darmstadium" in the ore dictionary.
But the MetaItem "fixes" this to Aluminium when the tag is not present. Probably not a big deal if this discrepency is just for the ore dictionary item stacks?

[Exaxxion]

I'll take a look at the linked comment, @warjort.

As for the default material if tags are missing, I don't think it matters much since the idea is that any material will work as long as it's the right tool. If consistency in how it shows up in JEI is deemed sufficiently important, we can probably change which material is used as the fallback when NBT is missing.

[jaredlll08]

I am not sure GTCE is abusing the OreDictionary. It is expecting the NBT to be ignored "as per spec".

Where did you see that spec?

But isn't the underlying issue that forge does ignore the NBT (even if still has a reference to it in the item stack)
but CraftTweaker does not?

Forge doesn't clear the NBT from the item that you provide it, it just copies the stack and inserts it into the dict.
(https://github.com/MinecraftForge/MinecraftForge/blob/1.12.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L704)

So as far as Forge is concerned, the ItemStack you provided is the ItemStack for the base Item in that specific Oredict entry.

If that spec does exist and it says that oredict is meant to be ignored (as in Forge should be clearing the nbt before inserting the stack), then this is an issue on Forge's side.

Like I said, CraftTweaker can't just magic the correct NBT out of thin air, it works with what Forge provides it.

This isn't special handling, it is what is required for consistency between Forge and CraftTweaker.

If I did that, I would actually be inconsistent with Forge since the code that I see does technically allow a single ItemStack of a base Item to have NBT in the oredict , so that could introduce oddities with other mods if I clear the NBT of their item.

[Exaxxion]

If I did that, I would actually be inconsistent with Forge since the code that I see does technically allow a single ItemStack of a base Item to have NBT in the oredict , so that could introduce oddities with other mods if I clear the NBT of their item.

I can think of a few such mods just off the top of my head yeah. CT seems to be doing exactly what it should be.

@warjort I read your concerns from the linked comment and I don't think it will be a problem. The final say on any PRs will of course go to Lag, but I think the approach is the right one.

[warjort]

Where did you see that spec?

That's why I put the "as per spec" in quotes. It is not documentated except as dicta like on the bug report you linked.
But since OreDictionary is defined by the forge project it's implementation can be regarded as the de facto spec.

If I did that, I would actually be inconsistent with Forge since the code that I see does technically allow a single ItemStack of a base Item to have NBT in the oredict , so that could introduce oddities with other mods if I clear the NBT of their item.

I am not saying you should clear it, I am saying you should ignore it during matching (like forge does).
In fact, on the other post I give the downside to the linked workaround, as it as it introduces run time override due to defaulting and will break mods that don't expect it to be clear.

[Exaxxion]

and will break mods that don't expect it to be clear.

Can you provide an example of this? I'm having trouble envisioning how the bare item would break some other mod.

[jaredlll08]

I am not saying you should clear it, I am saying you should ignore it during matching (like forge does).
In fact, on the other post I give the downside to the linked workaround, as it as it introduces run time override due to defaulting and will break mods that don't expect it to be clear.

Not sure where Forge matches it, but last I checked we do more lenient matching than Forge usually does for ItemStacks.

We do partial NBT matching, so if you had an item like:

<item:minecraft:dirt>.withTag({custom: "data"});

Forge would only accept a piece of Dirt with that exact tag, no more.
Whereas CraftTweaker will accept any piece of dirt that has that tag, regardless of what other tags it has, so it would accept:

<item:minecraft;dirt>.withTag({custom: "data", renamed: "true"});

which forge would not.

I believe this whole issue could be fixed if GT registered the item with the absolute minimum NBT that it requires, so if you needed a {type: "ingot"} tag, you can have it, and CraftTweaker will accept an item that has that tag, and any other material tag.

Going to be honest with you guys, 1.12 is not a priority for me at all, I'm already not a fan of the few hours I spent researching this that could have been spent porting to 1.17.

So while I accept PRs from others, and if someone reports something that is a 5 minute fix that doesn't require me to even open the game, I'll do it, but anything more, like a whole rewrite of our matching system, is completely off the table.

[warjort]

Can you provide an example of this? I'm having trouble envisioning how the bare item would break some other mod.

You could have a mod that does what we do, but doesn't have the tag defaulting like GTCE if they are missing.
This would work fine when running with forge, but the matching would break for CT recipes.
Creating a new ore dictionary with the NBT removed from the item stacks (like in the workaround linked above) would probably break such a mod as it NPEs on missing tags (likely during rendering?).

[Exaxxion]

You could have a mod that does what we do, but doesn't have the tag defaulting like GTCE if they are missing.

Sounds like that mod's problem to me. It's like intentionally introducing nulls but not checking for them. If we're changing things on our end, it would at best affect GTCE and a small number of addon mods for which the developers are in regular contact with each other and could accommodate such a change. Modpack authors should likewise be able to adapt without much trouble, if it affects them at all.

Going to be honest with you guys, 1.12 is not a priority for me at all, I'm already not a fan of the few hours I spent researching this that could have been spent porting to 1.17.

I understand where you're coming from and I really appreciate that you did take the time to explain how things work on CT's side.

[jaredlll08]

For what it's worth, I think I can count the issues I've had about GT oredict on a single hand, so you could just leave it as is, then write a wiki page somewhere that explains the incompatibility and link to the gist with a fix and call it a day.

If people come to me with issues, I'll just link to that page

[Exaxxion]

The circumstances leading to it are pretty specific yeah: one needs to be using GTCE but also making a custom recipe with CT that uses one of the crafting tool ore dictionaries. Besides pack devs almost nobody would run into this.

We'll look into it and try to fix it. Linking to the workaround gist if anyone brings it up to you again in the meantime sounds like a good plan.