Skip to content
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 VillagerTypes #1875

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

JT122406
Copy link
Contributor

@JT122406 JT122406 commented Jan 19, 2025

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

@neoforged-automation neoforged-automation bot added the 1.21.4 Targeted at Minecraft 1.21.4 label Jan 19, 2025
@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@ChampionAsh5357
Copy link
Contributor

In my opinion, while this does remove the error, if the goal is allow custom villager types, then you also need to consider patching VillagerType#byBiome to allow spawning villagers or zombie villagers with their appropriate type. Additionally, what if someone wanted to add their own entry for the existing fisherman trade or for other 'villager type' trades? All this would do is hide the error and not allow for mutability.

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.

Copy link
Member

@Matyrobbrt Matyrobbrt left a 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.

@JT122406
Copy link
Contributor Author

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.

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

@Matyrobbrt Matyrobbrt enabled auto-merge (squash) January 24, 2025 10:26
@Matyrobbrt Matyrobbrt changed the title Patch VillagerTrades.EmeraldsForVillagerTypeItem to allow modders to make Custom VillagerTypes Patch EmeraldsForVillagerTypeItem to allow modders to make Custom VillagerTypes Jan 24, 2025
@Matyrobbrt Matyrobbrt merged commit 81df67a into neoforged:1.21.x Jan 24, 2025
6 checks passed
@Matyrobbrt

This comment has been minimized.

@neoforged-releases
Copy link

🚀 This PR has been released as NeoForge version 21.4.75-beta.

neoforged-automation bot pushed a commit that referenced this pull request Jan 24, 2025
@neoforged-automation
Copy link

Created backport PR: #1910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.4 Targeted at Minecraft 1.21.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants