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.

Sign up for GitHub

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

Jump to bottom

Support trade rebalance experiment #3311

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Support trade rebalance experiment #3311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac08e20
Support trade rebalance experiment
apple502j Sep 11, 2023
3c32eec
Add pool IDs
apple502j Sep 11, 2023
56d6ddf
Merge branch '1.20.2' of https://github.com/FabricMC/fabric into obje…
apple502j Sep 16, 2023
b7c35fc
Delayed pool modification
apple502j Sep 16, 2023
40b9530
Fix unused import
apple502j Sep 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 154 additions & 0 deletions ...pi-v1/src/main/java/net/fabricmc/fabric/api/object/builder/v1/trade/TradeOfferHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@

package net.fabricmc.fabric.api.object.builder.v1.trade;

import java.util.Collection;
import java.util.List;
import java.util.function.Consumer;

import org.jetbrains.annotations.ApiStatus;

import net.minecraft.village.TradeOffers;
import net.minecraft.village.VillagerProfession;

Expand All @@ -30,6 +33,9 @@
public final class TradeOfferHelper {
/**
* Registers trade offer factories for use by villagers.
* This adds the same trade offers to current and rebalanced trades.
* To add separate offers for the rebalanced trade experiment, use
* {@link #registerVillagerOffers(VillagerProfession, int, VillagerOffersAdder)}.
*
* <p>Below is an example, of registering a trade offer factory to be added a blacksmith with a profession level of 3:
* <blockquote><pre>
Expand All @@ -43,11 +49,38 @@ public final class TradeOfferHelper {
* @param factories a consumer to provide the factories
*/
public static void registerVillagerOffers(VillagerProfession profession, int level, Consumer<List<TradeOffers.Factory>> factories) {
TradeOfferInternals.registerVillagerOffers(profession, level, (trades, rebalanced) -> factories.accept(trades));
}

/**
* Registers trade offer factories for use by villagers.
* This method allows separate offers to be added depending on whether the rebalanced
* trade experiment is enabled.
* If a particular profession's rebalanced trade offers are not added at all, it falls back
* to the regular trade offers.
*
* <p>Below is an example, of registering a trade offer factory to be added a blacksmith with a profession level of 3:
* <blockquote><pre>
* TradeOfferHelper.registerVillagerOffers(VillagerProfession.BLACKSMITH, 3, (factories, rebalanced) -> {
* factories.add(new CustomTradeFactory(...);
* });
* </pre></blockquote>
*
* <p><strong>Experimental feature</strong>. This API may receive changes as necessary to adapt to further experiment changes.
*
* @param profession the villager profession to assign the trades to
* @param level the profession level the villager must be to offer the trades
* @param factories a consumer to provide the factories
*/
@ApiStatus.Experimental
public static void registerVillagerOffers(VillagerProfession profession, int level, VillagerOffersAdder factories) {
TradeOfferInternals.registerVillagerOffers(profession, level, factories);
}

/**
* Registers trade offer factories for use by wandering trades.
* This does not add offers for the rebalanced trade experiment.
* To add rebalanced trades, use {@link #registerRebalancedWanderingTraderOffers}.
*
* @param level the level the trades
* @param factory a consumer to provide the factories
Expand All @@ -56,6 +89,20 @@ public static void registerWanderingTraderOffers(int level, Consumer<List<TradeO
TradeOfferInternals.registerWanderingTraderOffers(level, factory);
}

/**
* Registers trade offer factories for use by wandering trades.
* This only adds offers for the rebalanced trade experiment.
* To add regular trades, use {@link #registerWanderingTraderOffers(int, Consumer)}.
*
* <p><strong>Experimental feature</strong>. This API may receive changes as necessary to adapt to further experiment changes.
*
* @param factory a consumer to add trade offers
*/
@ApiStatus.Experimental
public static synchronized void registerRebalancedWanderingTraderOffers(Consumer<WanderingTraderOffersBuilder> factory) {
factory.accept(new TradeOfferInternals.WanderingTraderOffersBuilderImpl());
}

/**
* @deprecated This never did anything useful.
*/
Expand All @@ -66,4 +113,111 @@ public static void refreshOffers() {

private TradeOfferHelper() {
}

@FunctionalInterface
public interface VillagerOffersAdder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boolean parameter is a no-go for me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Mojang can add multiple offer rebalance packs and mods can add multiple rebalance options; it's better for them to query the data pack status instead, so maybe pass an object that can provide such data pack/experiment enabled information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mojang offering 2 trade rebalance packs is just a speculation - we should not over-complicate the API; and since this API does not allow mods to modify existing trades, I doubt there would be many mods that does that to warrant expansion of this API.

Copy link
Contributor

@liach liach Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then Mojang can remove the trade rebalance pack, by integrating into mainline or dropping it, at any time. What would this boolean serve by then?

What I am looking for is another parameter which mods can query if ANY experimental pack, from mojang or mods, are added. Say a mod want to add a villager trade for bundle if bundle pack is enabled; then this parameter should be some sort of view/manager that allows the mod to query this information and thus decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liach The boolean-taking version would be deprecated and replaced with the existing, non-taking version. However, a deprecation is expected for any design we could take (apart from just outright not supporting the experiment).

The reason such parameter isn't added is simple: it is not possible. This method is called statically on game load before a world is even loaded, and calling the method pow(2, experiments.length) times (and storing the result, then applying on world load) is unreasonable. Requiring that this be called during world load is inconsistent with the existing API, and there is no convincing reason to do this.

I also question the "modded experiment" part of the comment. Fabric API does not provide an API to register an experiment yet, so there is no reason to support modded experiments.

Finally, a mod that wants to conditionally add bundles in the offer do not need a special parameter. All you need is a custom TradeOffer.Factory, which creates an offer only if the experiment is added. (This method can return null since this update.)

Copy link
Contributor

@liach liach Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at the code, this is indeed some bad design from mojang. I think a better solution would be to patch MerchantEntity.fillRecipes implementations to fire 2 events where mods can modify the resulting TradeOffers.Factory list; you can pass the profession, level, and the server world in the regular villager event (and just the world in the wandering trader event), which should be sufficient for mods to make decisions mostly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I did not want to make it too different from existing one; I also don't know why this is needed apart from some hypothetical scenarios.

void onRegister(List<TradeOffers.Factory> factories, boolean rebalanced);
}

/**
* A builder for rebalanced wandering trader offers.
*
* <p><strong>Experimental feature</strong>. This API may receive changes as necessary to adapt to further experiment changes.
*
* @see #registerRebalancedWanderingTraderOffers(Consumer)
*/
@ApiStatus.NonExtendable
@ApiStatus.Experimental
public interface WanderingTraderOffersBuilder {
/**
* The pool index for the "buy items" pool.
* Two trade offers are picked from this pool.
*
* <p>In vanilla, this pool contains offers to buy water buckets, baked potatoes, etc.
* for emeralds.
*/
int BUY_ITEMS_POOL = 0;
/**
* The pool index for the "sell special items" pool.
* Two trade offers are picked from this pool.
*
* <p>In vanilla, this pool contains offers to sell logs, enchanted iron pickaxes, etc.
*/
int SELL_SPECIAL_ITEMS_POOL = 1;
/**
* The pool index for the "sell common items" pool.
* Five trade offers are picked from this pool.
*
* <p>In vanilla, this pool contains offers to sell flowers, saplings, etc.
*/
int SELL_COMMON_ITEMS_POOL = 2;

/**
* Adds a new pool to the offer list. Exactly {@code count} offers are picked from
* {@code factories} and offered to customers.
* @param count the number of offers to be picked from {@code factories}
* @param factories the trade offer factories
* @return this builder, for chaining
* @throws IllegalArgumentException if {@code count} is not positive or if {@code factories} is empty
*/
WanderingTraderOffersBuilder pool(int count, TradeOffers.Factory... factories);

/**
* Adds a new pool to the offer list. Exactly {@code count} offers are picked from
* {@code factories} and offered to customers.
* @param count the number of offers to be picked from {@code factories}
* @param factories the trade offer factories
* @return this builder, for chaining
* @throws IllegalArgumentException if {@code count} is not positive or if {@code factories} is empty
*/
default WanderingTraderOffersBuilder pool(int count, Collection<? extends TradeOffers.Factory> factories) {
return pool(count, factories.toArray(TradeOffers.Factory[]::new));
}

/**
* Adds trade offers to the offer list. All offers from {@code factories} are
* offered to each customer.
* @param factories the trade offer factories
* @return this builder, for chaining
* @throws IllegalArgumentException if {@code factories} is empty
*/
default WanderingTraderOffersBuilder addAll(Collection<? extends TradeOffers.Factory> factories) {
return pool(factories.size(), factories);
}

/**
* Adds trade offers to the offer list. All offers from {@code factories} are
* offered to each customer.
* @param factories the trade offer factories
* @return this builder, for chaining
* @throws IllegalArgumentException if {@code factories} is empty
*/
default WanderingTraderOffersBuilder addAll(TradeOffers.Factory... factories) {
return pool(factories.length, factories);
}

/**
* Adds trade offers to an existing pool.
*
* <p>See the constants for vanilla trade offer pool indices that are always available.
* @param poolIndex the pool index
* @param factories the trade offer factories
* @return this builder, for chaining
* @throws IndexOutOfBoundsException if {@code poolIndex} is out of bounds
*/
WanderingTraderOffersBuilder addOffersToPool(int poolIndex, TradeOffers.Factory... factories);

/**
* Adds trade offers to an existing pool.
*
* <p>See the constants for vanilla trade offer pool indices that are always available.
* @param poolIndex the pool index
* @param factories the trade offer factories
* @return this builder, for chaining
* @throws IndexOutOfBoundsException if {@code poolIndex} is out of bounds
*/
default WanderingTraderOffersBuilder addOffersToPool(int poolIndex, Collection<TradeOffers.Factory> factories) {
return addOffersToPool(poolIndex, factories.toArray(TradeOffers.Factory[]::new));
}
}
}
61 changes: 58 additions & 3 deletions ...der-api-v1/src/main/java/net/fabricmc/fabric/impl/object/builder/TradeOfferInternals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,53 @@
package net.fabricmc.fabric.impl.object.builder;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;

import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
import org.apache.commons.lang3.ArrayUtils;
import org.slf4j.LoggerFactory;
import org.apache.commons.lang3.tuple.Pair;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import net.minecraft.village.TradeOffers;
import net.minecraft.village.VillagerProfession;

import net.fabricmc.fabric.api.object.builder.v1.trade.TradeOfferHelper;

public final class TradeOfferInternals {
private static final Logger LOGGER = LoggerFactory.getLogger("fabric-object-builder-api-v1");

private TradeOfferInternals() {
}

/**
* Make the rebalanced profession map modifiable, then copy all vanilla
* professions' trades to prevent modifications from propagating to the rebalanced one.
*/
private static void initVillagerTrades() {
if (!(TradeOffers.REBALANCED_PROFESSION_TO_LEVELED_TRADE instanceof HashMap)) {
Map<VillagerProfession, Int2ObjectMap<TradeOffers.Factory[]>> map = new HashMap<>(TradeOffers.REBALANCED_PROFESSION_TO_LEVELED_TRADE);

for (Map.Entry<VillagerProfession, Int2ObjectMap<TradeOffers.Factory[]>> trade : TradeOffers.PROFESSION_TO_LEVELED_TRADE.entrySet()) {
if (!map.containsKey(trade.getKey())) map.put(trade.getKey(), trade.getValue());
}

TradeOffers.REBALANCED_PROFESSION_TO_LEVELED_TRADE = map;
}
}

// synchronized guards against concurrent modifications - Vanilla does not mutate the underlying arrays (as of 1.16),
// so reads will be fine without locking.
public static synchronized void registerVillagerOffers(VillagerProfession profession, int level, Consumer<List<TradeOffers.Factory>> factory) {
public static synchronized void registerVillagerOffers(VillagerProfession profession, int level, TradeOfferHelper.VillagerOffersAdder factory) {
Objects.requireNonNull(profession, "VillagerProfession may not be null.");
registerOffers(TradeOffers.PROFESSION_TO_LEVELED_TRADE.computeIfAbsent(profession, key -> new Int2ObjectOpenHashMap<>()), level, factory);
initVillagerTrades();
registerOffers(TradeOffers.PROFESSION_TO_LEVELED_TRADE.computeIfAbsent(profession, key -> new Int2ObjectOpenHashMap<>()), level, trades -> factory.onRegister(trades, false));
registerOffers(TradeOffers.REBALANCED_PROFESSION_TO_LEVELED_TRADE.computeIfAbsent(profession, key -> new Int2ObjectOpenHashMap<>()), level, trades -> factory.onRegister(trades, true));
}

public static synchronized void registerWanderingTraderOffers(int level, Consumer<List<TradeOffers.Factory>> factory) {
Expand All @@ -63,4 +86,36 @@ public static void printRefreshOffersWarning() {
Throwable loggingThrowable = new Throwable();
LOGGER.warn("TradeOfferHelper#refreshOffers does not do anything, yet it was called! Stack trace:", loggingThrowable);
}

public static class WanderingTraderOffersBuilderImpl implements TradeOfferHelper.WanderingTraderOffersBuilder {
/**
* Make the trade list modifiable.
*/
static void initWanderingTraderTrades() {
if (!(TradeOffers.REBALANCED_WANDERING_TRADER_TRADES instanceof ArrayList)) {
TradeOffers.REBALANCED_WANDERING_TRADER_TRADES = new ArrayList<>(TradeOffers.REBALANCED_WANDERING_TRADER_TRADES);
}
}

@Override
public TradeOfferHelper.WanderingTraderOffersBuilder pool(int count, TradeOffers.Factory... factories) {
if (factories.length == 0) throw new IllegalArgumentException("cannot add empty pool");
if (count <= 0) throw new IllegalArgumentException("count must be positive");

Pair<TradeOffers.Factory[], Integer> pool = Pair.of(factories, count);
initWanderingTraderTrades();
TradeOffers.REBALANCED_WANDERING_TRADER_TRADES.add(pool);
return this;
}

@Override
public TradeOfferHelper.WanderingTraderOffersBuilder addOffersToPool(int poolIndex, TradeOffers.Factory... factories) {
Objects.checkIndex(poolIndex, TradeOffers.REBALANCED_WANDERING_TRADER_TRADES.size());
initWanderingTraderTrades();
Pair<TradeOffers.Factory[], Integer> pool = TradeOffers.REBALANCED_WANDERING_TRADER_TRADES.get(poolIndex);
TradeOffers.Factory[] modified = ArrayUtils.addAll(pool.getLeft(), factories);
TradeOffers.REBALANCED_WANDERING_TRADER_TRADES.set(poolIndex, Pair.of(modified, pool.getRight()));
return this;
}
}
}
2 changes: 2 additions & 0 deletions fabric-object-builder-api-v1/src/main/resources/fabric-object-builder-api-v1.accesswidener
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ accessible method net/minecraft/world/poi/PointOfInterestTypes register

extendable class net/minecraft/block/entity/BlockEntityType$BlockEntityFactory
accessible class net/minecraft/village/TradeOffers$TypeAwareBuyForOneEmeraldFactory
mutable field net/minecraft/village/TradeOffers REBALANCED_PROFESSION_TO_LEVELED_TRADE Ljava/util/Map;
mutable field net/minecraft/village/TradeOffers REBALANCED_WANDERING_TRADER_TRADES Ljava/util/List;

accessible method net/minecraft/entity/SpawnRestriction register (Lnet/minecraft/entity/EntityType;Lnet/minecraft/entity/SpawnRestriction$Location;Lnet/minecraft/world/Heightmap$Type;Lnet/minecraft/entity/SpawnRestriction$SpawnPredicate;)V

Expand Down
36 changes: 34 additions & 2 deletions ...er-api-v1/src/testmod/java/net/fabricmc/fabric/test/object/builder/VillagerTypeTest1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@

import net.minecraft.entity.Entity;
import net.minecraft.entity.passive.WanderingTraderEntity;
import net.minecraft.item.Item;
import net.minecraft.item.ItemStack;
import net.minecraft.item.Items;
import net.minecraft.registry.Registries;
import net.minecraft.text.Text;
import net.minecraft.util.math.random.Random;
import net.minecraft.village.TradeOffer;
Expand All @@ -40,14 +42,44 @@
public class VillagerTypeTest1 implements ModInitializer {
@Override
public void onInitialize() {
TradeOfferHelper.registerVillagerOffers(VillagerProfession.ARMORER, 1, factories -> {
factories.add(new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.GOLD_INGOT, 3), new ItemStack(Items.NETHERITE_SCRAP, 4), new ItemStack(Items.NETHERITE_INGOT), 2, 6, 0.15F)));
TradeOfferHelper.registerVillagerOffers(VillagerProfession.ARMORER, 1, (factories, rebalanced) -> {
Item scrap = rebalanced ? Items.NETHER_BRICK : Items.NETHERITE_SCRAP;
factories.add(new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.GOLD_INGOT, 3), new ItemStack(scrap, 4), new ItemStack(Items.NETHERITE_INGOT), 2, 6, 0.15F)));
});
// Toolsmith is not rebalanced yet
TradeOfferHelper.registerVillagerOffers(VillagerProfession.TOOLSMITH, 1, (factories, rebalanced) -> {
Item scrap = rebalanced ? Items.NETHER_BRICK : Items.NETHERITE_SCRAP;
factories.add(new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.GOLD_INGOT, 3), new ItemStack(scrap, 4), new ItemStack(Items.NETHERITE_INGOT), 2, 6, 0.15F)));
});

TradeOfferHelper.registerWanderingTraderOffers(1, factories -> {
factories.add(new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.GOLD_INGOT, 3), new ItemStack(Items.NETHERITE_SCRAP, 4), new ItemStack(Items.NETHERITE_INGOT), 2, 6, 0.35F)));
});

TradeOfferHelper.registerRebalancedWanderingTraderOffers(builder -> {
builder.pool(
1,
Registries.ITEM.stream().filter(item -> item.getFoodComponent() != null).map(
item -> new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.NETHERITE_INGOT), new ItemStack(item), 3, 4, 0.15F))
).toList()
);
builder.addAll(
new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.NETHERITE_INGOT), new ItemStack(Items.MOJANG_BANNER_PATTERN), 1, 4, 0.15F))
);
builder.addOffersToPool(
TradeOfferHelper.WanderingTraderOffersBuilder.BUY_ITEMS_POOL,
new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.BLAZE_POWDER, 1), new ItemStack(Items.EMERALD, 4), 3, 4, 0.15F)),
new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.NETHER_WART, 5), new ItemStack(Items.EMERALD, 1), 3, 4, 0.15F)),
new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.GOLDEN_CARROT, 4), new ItemStack(Items.EMERALD, 1), 3, 4, 0.15F))
);
builder.addOffersToPool(
TradeOfferHelper.WanderingTraderOffersBuilder.SELL_SPECIAL_ITEMS_POOL,
new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.EMERALD, 6), new ItemStack(Items.BRUSH, 1), 1, 4, 0.15F)),
new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.DIAMOND, 16), new ItemStack(Items.ELYTRA, 1), 1, 4, 0.15F)),
new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.EMERALD, 3), new ItemStack(Items.LEAD, 2), 3, 4, 0.15F))
);
});

CommandRegistrationCallback.EVENT.register((dispatcher, registryAccess, environment) -> {
dispatcher.register(literal("fabric_applywandering_trades")
.then(argument("entity", entity()).executes(context -> {
Expand Down
2 changes: 1 addition & 1 deletion ...er-api-v1/src/testmod/java/net/fabricmc/fabric/test/object/builder/VillagerTypeTest2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
public class VillagerTypeTest2 implements ModInitializer {
@Override
public void onInitialize() {
TradeOfferHelper.registerVillagerOffers(VillagerProfession.ARMORER, 1, factories -> {
TradeOfferHelper.registerVillagerOffers(VillagerProfession.WEAPONSMITH, 1, factories -> {
factories.add(new SimpleTradeFactory(new TradeOffer(new ItemStack(Items.DIAMOND, 5), new ItemStack(Items.NETHERITE_INGOT), 3, 4, 0.15F)));
});
TradeOfferHelper.registerVillagerOffers(VillagerProfession.ARMORER, 1, factories -> {
Expand Down
Loading