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

Crash due to TradeOffersTypeAwareBuyForOneEmeraldFactoryMixin incorrectly assuming null trade items will create empty item stacks #4456

Open
Antikyth opened this issue Feb 20, 2025 · 0 comments · May be fixed by #4457
Labels
bug Something isn't working

Comments

@Antikyth
Copy link

Antikyth commented Feb 20, 2025

This method in the mixin assumes that an empty item stack will be created due to null being passed in the constructor.

/**
 * To prevent "item" -> "air" trades, if the result of a type aware trade is air, make sure no offer is created.
 */
@Inject(method = "create", at = @At(value = "NEW", target = "net/minecraft/village/TradeOffer"), locals = LocalCapture.CAPTURE_FAILEXCEPTION, cancellable = true)
private void failOnNullItem(Entity entity, Random random, CallbackInfoReturnable<TradeOffer> cir, ItemStack buyingItem) {
    if (buyingItem.isEmpty()) { // Will return true for an "empty" item stack that had null passed in the ctor
        cir.setReturnValue(null); // Return null to prevent creation of empty trades
    }
}

Unfortunately, at least in 1.20.1, item stack constructors take ItemConvertible and call asItem on the input.

@Nullable
@Override
public TradeOffer create(Entity entity, Random random) {
    if (entity instanceof VillagerDataContainer) {
        ItemStack itemStack = new ItemStack((ItemConvertible)this.map.get(((VillagerDataContainer)entity).getVillagerData().getType()), this.count);
        return new TradeOffer(itemStack, new ItemStack(Items.EMERALD), this.maxUses, this.experience, 0.05F);
    } else {
        return null;
    }
}

Which causes a null pointer exception if a null item is given.

I imagine this was not noticed until now because in vanilla, the only type aware trade is when a fisherman is at its maximum level and a mod has added a custom villager type. My custom villager type also has its own type aware trades at a lower level though, so this crash happened without max level - but I don't think this could have ever worked, at least with how TypeOffersTypeAwareBuyForOneEmeraldFactory is implemented in 1.20.1.

I suppose to fix this the mixin will have to add a condition before the item stack is created in TypeOffersTypeAwareBuyForOneEmeraldFactory#create, since the mixin disables vanilla's crash in the TypeOffersTypeAwareBuyForOneEmeraldFactory constructor for missing trades.

@apple502j apple502j added the bug Something isn't working label Feb 20, 2025
Antikyth added a commit to Antikyth/fabric that referenced this issue Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants