-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
Patch EmeraldsForVillagerTypeItem
to allow modders to make Custom VillagerType
s
#1875
Conversation
|
In my opinion, while this does remove the error, if the goal is allow custom villager types, then you also need to consider patching I don't think my comments are a blocker in any sense if you just want to ignore the vanilla systems, but if you are adding a mixin to either of the two areas I mentioned, that would mean that removing the error isn't sufficient for the usecase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree with Champ and will open a PR with a data map for that map. I also think that the immutability of the maps is making this an issue.
As for this PR while it seems fine, as @ChampionAsh5357 it doesn't fully fix the problem as it still doesn't add a way for custom villager types to be compatible with vanilla professions, they would just have empty trades for those slots. I don't consider this a blocker either but it's worth considering a way to modify the vanilla trades.
patches/net/minecraft/world/entity/npc/VillagerTrades.java.patch
Outdated
Show resolved
Hide resolved
patches/net/minecraft/world/entity/npc/VillagerTrades.java.patch
Outdated
Show resolved
Hide resolved
patches/net/minecraft/world/entity/npc/VillagerTrades.java.patch
Outdated
Show resolved
Hide resolved
Little confused what you mean by them not being compatible with vanilla professions as this is only one trade for fisherman all the other ones should work by default, granted we should add a way to add to this trade, maybe a datamap or something but oddly specific single trade datamap sounds odd. I appricate you making the pr for the by_biome as that was something I was thinking about next after this |
…make Custom Villager Types
patches/net/minecraft/world/entity/npc/VillagerTrades.java.patch
Outdated
Show resolved
Hide resolved
patches/net/minecraft/world/entity/npc/VillagerTrades.java.patch
Outdated
Show resolved
Hide resolved
EmeraldsForVillagerTypeItem
to allow modders to make Custom VillagerType
s
This comment has been minimized.
This comment has been minimized.
🚀 This PR has been released as NeoForge version |
Created backport PR: #1910 |
Currently if you make a custom Villager type the game will crash when you Load into or Create a world, since mc throws an exception when a VillagerType has no Trades, This makes sense in Vanilla's case but makes it impossible for modders to make there own villager types without extreme difficulty.
The first change in the patch simply removes the check completely, preventing the crash. (The other option would be to leave the check but make the list empty so it never throws the exception, I decided against this as then the check here would just be pointless code to run but I am willing to do it this way if need be)
The second change in the patch makes sure that the item cost itemstack isn't empty since there will be no trades of this type for Modded VillagerTypes, if it is empty we return null, if we don't include this check we would get air trades
Fabric has a very similar mixin to allow this:
https://github.com/FabricMC/fabric/blob/1.21.4/fabric-object-builder-api-v1/src/main/java/net/fabricmc/fabric/mixin/object/builder/TradeOffersTypeAwareBuyForOneEmeraldFactoryMixin.java