V3 - Trade Id (#15)

- Removed the Trade Cache and related APIs.
- Persistent trades can no longer be rerolled, until they've been used.
- Trade Factories now provide their persistence solely through getters, instead of being required to reimplement the whole caching logic.
- Persistence data is now stored directly in the trades themselves, instead of inferred from brittle item components.

.github/workflows/build.yml

@@ -5,6 +5,7 @@
 
 name: build
 on:
+  - pull_request
   - workflow_call
   - workflow_dispatch
 

README.md

@@ -1,12 +1,17 @@
 # Shifting Wares
 
-Causes villager to occasionally re-roll their trade offers, making them more flexible and less exploitable.
+Causes villager to re-roll their trade offers on their own accord, up to three times a day.
+The effect of thiswill be most noticeable on professions that can sell enchanted or coloured items; a single one of these villagers will be able to offer a much greater variety of wares over time.
 
-The most noticeable effects will be felt on any profession able to sell enchanted items; a single of these villager will be able to offer a much greater variety of wares over time.
+This aims to reduce the benefits of exploitative playstyle, while making serendipitous playstyles more viable.
+
+While it's still possible to get the trade you want by repeatedly breaking a villager's workstation, the benefits of doing so become *very* short lived. Waiting for a natural reroll is probably a better use of your time.
+
+Because all trades eventually expire, villagers become much easier to replace if they die, so you don't need to care as much about their safety. You can let them roam around freely with little risk.
 
 ## Triggers
 
-There are two gamerules that control when trades can be re-rolled. Both are enabled by default.  
+There are two gamerules that control when trades can be re-rolled. Both are enabled by default.
 Disabling all rules effectively disables the mod.
 - `shiftingWares.dailyReroll`:
 	Causes villagers to re-roll **all** their offers once per day, the first time they restock at their job station.
@@ -18,12 +23,7 @@ Disabling all rules effectively disables the mod.
 Minecraft permanently saves any create map, and lock their structures from appearing on other exploration maps.
 To prevent daily rerolls from throwing away endless amounts of unsold maps, those trades are handled differently.
 
-Cartographers will remember each map they sell, and offer it again it the next time the same map trade comes up.
-The gamerule `shiftingWares.allowMapReroll` (disabled by default) will allow them to forget a map, after it has been sold at least once.
-
-The map's item name, (or preferably, its translation key), is used to tell apart different types of maps.
-Since 1.20.5, the `item_name` is preferred over the `custom_name`.
-
+By default, map trades will never be rerolled. They may only be rerolled if the gamerule `shiftingWares.allowMapReroll` is enabled, and if the trade has been used at least once.
 
 ## Technical details
 - If a villager is unable to generate all registered trades for a level, it will be replaced with an empty trade. With vanilla trades, this should only ever happen to cartographers, who are unable to generate explorer maps in worlds with no structures.  
@@ -35,6 +35,7 @@ Placeholder trades will never take the place of a valid trade; they will only sh
 - Depleted rerolls have a chance to yield duplicate trades.
 
 ## For developpers
-By default, shifting-Wares assumes 2 trades per level, and pulls its trade pools from the same place as Vanilla.
+By default, shifting-Wares assumes 2 trades per level, and pulls its trade pools from the same place as Vanilla. Other mods can override this by using the `shifting-wares` entry-point, and implementing [`ITradeLayoutProvider`](./src/main/java/fr/estecka/shiftingwares/api/ITradeLayoutProvider.java).
 
-If you have a mod that changes any of that, Shifting-Wares has an API you can use to specify the trade pools and layout that should be used instead.
+If your mod contains custom implementation of map trade factories, or produces other items attached with permanent data, you can communicate to Shifting-Wares that they should be persistent, by implementing the same methods defined in [`IShiftingTradeFactory`](./src/main/java/fr/estecka/shiftingwares/api/IShiftingTradeFactory.java).
+No dependency on Shifting-Wares is required for this, you only need to provide methods with matching names and prototypes.

build.gradle

@@ -1,5 +1,5 @@
 plugins {
-	id 'fabric-loom' version '1.8-SNAPSHOT'
+	id 'fabric-loom' version '1.7-SNAPSHOT'
 	id 'maven-publish'
 }
 

changelog.md

@@ -44,3 +44,9 @@ This boost performances when using Fresh Animations
 ### 2.2.3
 - Updated for MC 1.21.2
 - Starting MC 1.21.2, placeholder trades now use `item_model`'s to look empty again.
+
+# v3
+- Removed trade cache.
+- Trades can now be marked a persistent by their factories. Persistent items are *never* rerolled until they have been sold. Filled map trades are automatically marked as persistent regardless of the factories setting. This prevents mods who implement custom factories from causing unsold maps to be discarded.
+- Trades can now be given an identifier by their factories. ShiftingWares will prevent villagers from getting multiple trades coming from the same factory during rerolls. This will prevent the villager's listing from getting clogged up with multiple copies of the same persistent trade after any reroll, and prevent duplicatas of non-persistent from ocurring during depleted rerolls.
+- Other mods can provide the relevant data for their factories, by simply implementing the correct methods. No dependency on ShiftingWares is required.

gradle.properties

@@ -12,6 +12,6 @@ loader_version=0.16.7
 fabric_version=0.106.1+1.21.3
 
 # Mod Properties
-mod_version=2.2.3
-maven_group=tk.estecka.shiftingwares
+mod_version=3.0.0
+maven_group=fr.estecka.shiftingwares
 archives_base_name=shifting-wares

port.md

@@ -20,5 +20,5 @@ Current master
 - `getRandom()` was moved from `LivingEntity` to its parent class `Entity`. No code change required, but needs recompilation.
 
 ### 1.21.2
-#### No workaround:
-- `getGamerules()` was moved from `World` to its child class `ServerWorld`. Code change can be avoided by casting, but recompilation is still needed.
+#### Worked around:
+- `World.getGamerules()` was moved to `ServerWorld`. Call it from `MinecraftServer` instead.

src/main/java/fr/estecka/shiftingwares/ShiftingTradeData.java

@@ -0,0 +1,71 @@
+package fr.estecka.shiftingwares;
+
+import java.util.Optional;
+import org.jetbrains.annotations.Nullable;
+import com.mojang.serialization.Codec;
+import com.mojang.serialization.codecs.RecordCodecBuilder;
+import net.minecraft.component.DataComponentTypes;
+import net.minecraft.item.ItemStack;
+import net.minecraft.util.Identifier;
+import net.minecraft.village.TradeOffer;
+import net.minecraft.village.TradeOffers;
+import fr.estecka.shiftingwares.api.IShiftingTradeFactory;
+import fr.estecka.shiftingwares.duck.ITradeOfferDuck;
+
+public class ShiftingTradeData
+{
+	static public final Codec<ShiftingTradeData> CODEC = RecordCodecBuilder.create(
+		instance -> instance.group(
+			Identifier.CODEC.optionalFieldOf("tradeId").forGetter(data -> Optional.ofNullable(data.tradeId)),
+			Codec.BOOL.fieldOf("isPersistent").orElse(false).forGetter(data -> data.isPersistent),
+			Codec.BOOL.fieldOf("wasNeverUsed").orElse(true).forGetter(data -> data.wasNeverUsed)
+		)
+		.apply(instance, ShiftingTradeData::new)
+	);
+
+	public @Nullable Identifier tradeId = null;
+	public boolean isPersistent = false;
+	public boolean wasNeverUsed = true;
+
+	public ShiftingTradeData(){};
+
+	public ShiftingTradeData(Optional<Identifier> tradeId, boolean isPersistent, boolean wasNeverUsed){
+		this.tradeId = tradeId.orElse(null);
+		this.isPersistent = isPersistent;
+		this.wasNeverUsed = wasNeverUsed;
+	};
+
+
+	/**
+	 * Should be called immediately  after a factory  has produced  a new trade.
+	 * It's possible  for factories to wrap  other factories; persistence should 
+	 * be preserved down the line.
+	 * Here, trade ids are being  preserved as much as possible, but this has no
+	 * use currently, since SW will not be able to identify the inner factories.
+	 */
+	static public void FinalizeTrade(@Nullable TradeOffer offer, TradeOffers.Factory factory){
+		if (offer == null)
+			return;
+
+		IShiftingTradeFactory factoryData = IShiftingTradeFactory.Of(factory);
+		ShiftingTradeData offerData = ITradeOfferDuck.Of(offer).shiftingwares$GetTradeData();
+
+		offerData.isPersistent |= factoryData.shiftingwares$IsItemPersistent();
+
+		Identifier factoryId = factoryData.shiftingwares$GetTradeId();
+		if (factoryId != null)
+			offerData.tradeId = factoryId;
+
+		ItemStack sellItem = offer.getSellItem();
+		if (!offerData.isPersistent && ShouldBePersistent(sellItem)){
+			offerData.isPersistent = true;
+			ShiftingWares.LOGGER.error("A trade factory just produced a persistent item, but did not declare it as such: {} ({})", sellItem.getName().getString(), sellItem.getItem());
+		}
+		else if (offerData.isPersistent && factoryId != null)
+			ShiftingWares.LOGGER.info("Created new persistent trade: {}", factoryId);
+	}
+
+	static public boolean ShouldBePersistent(ItemStack stack){
+		return stack.contains(DataComponentTypes.MAP_ID);
+	}
+}

src/main/java/tk/estecka/shiftingwares/ShiftingWares.java → src/main/java/fr/estecka/shiftingwares/ShiftingWares.java

@@ -1,11 +1,12 @@
-package tk.estecka.shiftingwares;
+package fr.estecka.shiftingwares;
 
 import net.fabricmc.api.ModInitializer;
 import net.fabricmc.fabric.api.gamerule.v1.CustomGameRuleCategory;
 import net.fabricmc.fabric.api.gamerule.v1.GameRuleFactory;
 import net.fabricmc.fabric.api.gamerule.v1.GameRuleRegistry;
 import net.minecraft.component.DataComponentTypes;
 import net.minecraft.item.Items;
+import net.minecraft.registry.Registries;
 import net.minecraft.text.Text;
 import net.minecraft.util.Formatting;
 import net.minecraft.util.Identifier;
@@ -14,16 +15,17 @@
 import net.minecraft.village.TradedItem;
 import net.minecraft.world.GameRules;
 import net.minecraft.world.GameRules.BooleanRule;
-import tk.estecka.shiftingwares.TradeLayouts.VanillaTradeLayout;
+import fr.estecka.shiftingwares.TradeLayouts.VanillaTradeLayout;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 public class ShiftingWares
 implements ModInitializer
 {
-	static public final Logger LOGGER = LoggerFactory.getLogger("Shifting-Wares");
+	static public final String MODID = "shifting-wares";
+	static public final Logger LOGGER = LoggerFactory.getLogger(MODID);
 
-	static public final CustomGameRuleCategory RULE_CATTEGORY = new CustomGameRuleCategory(Identifier.of("shifting-wares","gamerules"), Text.translatable("gamerule.category.shiftingwares").formatted(Formatting.BOLD, Formatting.YELLOW));
+	static public final CustomGameRuleCategory RULE_CATTEGORY = new CustomGameRuleCategory(Identifier.of(MODID, "gamerules"), Text.translatable("gamerule.category.shiftingwares").formatted(Formatting.BOLD, Formatting.YELLOW));
 
 	static public final GameRules.Key<BooleanRule> DAILY_RULE    = GameRuleRegistry.register("shiftingWares.dailyReroll",    RULE_CATTEGORY, GameRuleFactory.createBooleanRule(true));
 	static public final GameRules.Key<BooleanRule> DEPLETED_RULE = GameRuleRegistry.register("shiftingWares.depleteReroll",  RULE_CATTEGORY, GameRuleFactory.createBooleanRule(true));
@@ -32,12 +34,13 @@ public class ShiftingWares
 	static public final TradeOffer PLACEHOLDER_TRADE;
 
 	static {
-		TradedItem fake_air = new TradedItem(Items.EMERALD)
-			.withComponents( builder -> builder
-				.add(DataComponentTypes.ITEM_NAME, Text.literal("Unobtainium"))
-				.add(DataComponentTypes.HIDE_TOOLTIP, Unit.INSTANCE)
-				.add(DataComponentTypes.ITEM_MODEL, Identifier.ofVanilla("air"))
-			);
+		TradedItem fake_air = new TradedItem(Items.EMERALD).withComponents( builder -> {
+			builder.add(DataComponentTypes.ITEM_NAME, Text.literal("Unobtainium"));
+			builder.add(DataComponentTypes.HIDE_TOOLTIP, Unit.INSTANCE);
+			if (Registries.DATA_COMPONENT_TYPE.containsId(Identifier.ofVanilla("item_model"))) // 1.21.0 compatibility
+				builder.add(DataComponentTypes.ITEM_MODEL, Identifier.ofVanilla("air"));
+			return builder;
+		});
 
 		PLACEHOLDER_TRADE = new TradeOffer(
 			fake_air,

src/main/java/tk/estecka/shiftingwares/TradeLayouts/VanillaTradeLayout.java → src/main/java/fr/estecka/shiftingwares/TradeLayouts/VanillaTradeLayout.java

@@ -1,4 +1,4 @@
-package tk.estecka.shiftingwares.TradeLayouts;
+package fr.estecka.shiftingwares.TradeLayouts;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -10,19 +10,23 @@
 import net.minecraft.village.VillagerData;
 import net.minecraft.village.VillagerProfession;
 import net.minecraft.village.TradeOffers.Factory;
-import tk.estecka.shiftingwares.ShiftingWares;
-import tk.estecka.shiftingwares.api.ITradeLayoutProvider;
+import fr.estecka.shiftingwares.ShiftingWares;
+import fr.estecka.shiftingwares.api.ITradeLayoutProvider;
 
 public class VanillaTradeLayout
 implements ITradeLayoutProvider
 {
-	static public final Identifier TRADE_REBALANCE_FLAGID = Identifier.of("minecraft", "trade_rebalance");
+	/**
+	 * This is a leftover from 1.20.x, which provided backward compatibility
+	 * for versions without that experimental feature. Although the check is
+	 * currently useless, it's expected to become useful again whenever that
+	 * feature is removed.
+	 */
 	static public final boolean IS_EXP_TRADE_AVAILABLE;
-
 	static {
 		var featureSet = FeatureFlags.FEATURE_MANAGER.getFeatureSet();
 		var featureIds = FeatureFlags.FEATURE_MANAGER.toId(featureSet);
-		IS_EXP_TRADE_AVAILABLE = featureIds.contains(TRADE_REBALANCE_FLAGID);
+		IS_EXP_TRADE_AVAILABLE = featureIds.contains(Identifier.ofVanilla("trade_rebalance"));
 	}
 
 	public List<Factory[]>	GetTradeLayout(VillagerEntity villager){