fabric icon indicating copy to clipboard operation
fabric copied to clipboard

De-hardcode Shears

Open deirn opened this issue 4 years ago • 21 comments

Redirects all item == Items.SHEARS from vanilla. Used this to transform the loot tables (of course i'm not doing that manually)

grep -L '"item": "minecraft:shears"' * | xargs rm -f
grep -rli '"item": "minecraft:shears"' * | xargs -i@ sed -i 's/"item": "minecraft:shears"/"tag": "fabric:shears"/g' @

Closes #1226

deirn avatar Jan 26 '21 06:01 deirn

For now, replacing the whole loot table jsons is fine in my opinion, however at some point I think we need a better way to do this, so that any mod can go in and replace stuff like this in a way that would allow for multiple mods to work on the same loot table.

Prospector avatar Jan 26 '21 23:01 Prospector

That sounds like a nightmare to support :P

deirn avatar Jan 27 '21 02:01 deirn

maybe it would make sense to mixin to where the condition is checked, then to see if its shears then to check if its in the tag rather than change the loottables

OroArmor avatar Jan 27 '21 02:01 OroArmor

I believe I could mixin to MatchToolLootCondition#test. The upside is it'll transform any loot tables, but then there won't be a way to check only for vanilla shears. I looked around on forge and it just replaces the loot table like I did.

deirn avatar Jan 27 '21 03:01 deirn

hm thats a good point. however, im not sure why someone would want something specific to the vanilla shears. There also might be a way to add another condition that forces the item in that case, but this feels hacky. The idea of transforming the loottables isn't bad, i was just hoping there would be a better way in code to do so.

OroArmor avatar Jan 27 '21 03:01 OroArmor

I definitely think Fabric should only ever touch vanilla loot tables, not modded ones, what I was mainly talking about would be allowing multiple other mods to make similar modifications to vanilla loot tables. For example, one mod that changed a drop, and another that changed a condition. However, as I said, out of scope for this PR.

Prospector avatar Jan 28 '21 17:01 Prospector

For leaves/cobweb loot tables, etc., you can use Fabric Loot Table API v1 or wait for new v2. Would be a good test for @Juuxel

maityyy avatar Jan 29 '21 04:01 maityyy

As Prospector said, there's currently no easy way to modify entries via code. Adding a new pool with same result won't work since there will be duplicates. https://youtu.be/9zXBNFLADGg

deirn avatar Jan 29 '21 07:01 deirn

Guess we will write a loot table visitor, like the tree visitors used by asm, javac api and all those cool stuff?

liach avatar Jan 29 '21 13:01 liach

Guess we will write a loot table visitor, like the tree visitors used by asm, javac api and all those cool stuff?

Is anyone currently working on that?

Edit: I mean if no one is, I can work on that.

RedstoneParadox avatar Feb 17 '21 23:02 RedstoneParadox

~~rip stale fabric pr number 1996894~~

kanpov avatar Jan 31 '22 13:01 kanpov

As Prospector said, there's currently no easy way to modify entries via code. Adding a new pool with same result won't work since there will be duplicates. https://youtu.be/9zXBNFLADGg

@deirn I think that can be done with #1241's LootTableEvents.REPLACE if you actually implement the entry modification yourself. (Just iterate through the pools and replace whatever's needed, then construct a new table and replace the old one)

Juuxel avatar Jan 31 '22 13:01 Juuxel

Wouldn't that require a way to not modify "manual" loot tables? We probably want users to be able to override our changes. Can't we use datagen to replace vanilla's tables?

Technici4n avatar Jan 31 '22 13:01 Technici4n

Wouldn't that require a way to not modify "manual" loot tables?

Yeah, and that's an issue with the loot api in general; see Fabricord #api.

Can't we use datagen to replace vanilla's tables?

How do we ensure that FAPI's "augmented" vanilla loot tables are not loaded instead of loot tables in other mods? If a mod makes an actual replacement for the loot table, that should be preferred over that sort of compat replacement.

Juuxel avatar Jan 31 '22 13:01 Juuxel

No idea how we could prevent that tbh. Possibly with a resource condition that allows other mods to disable our tables?

Technici4n avatar Jan 31 '22 18:01 Technici4n

would love this, as im currently trying to make a mod that adds stone shears

AnAwesomGuy avatar Apr 17 '23 08:04 AnAwesomGuy

Would be interesting to see how Forge addresses this.

Technici4n avatar Apr 17 '23 09:04 Technici4n

Last time I looked at Forge it just replaced the loot tables.

deirn avatar Apr 18 '23 06:04 deirn

That seems fine by me, we could likely datagen them.

Technici4n avatar Apr 18 '23 07:04 Technici4n

https://github.com/OroArmorModding/Multi-Item-Lib/ https://github.com/VazkiiMods/Botania/blob/1.19.x/Fabric/src/main/java/vazkii/botania/fabric/mixin/ItemPredicateFabricMixin.java

AnAwesomGuy avatar Apr 23 '23 07:04 AnAwesomGuy

Multi-Item-Lib does not handle loot tables, it seems like. IMO, instead of using an in-code registry like done in Multi-Item-Lib, items should be put in a tag instead, like c:shears for easier backwards compatility and matching the vanilla way, like other tools like pickaxes are handled.

DaFuqs avatar May 05 '23 13:05 DaFuqs