Trade amount fix (#3)

Fixed the issue whereby Cartographers had trouble generating high-level trades, in worlds set to generate no structure.  

Failed attempts at generating trades (Explorer Map trades in this case) may fall back to an empty placeholder trade. This will ensure all jobs always have the same amount of trade slots. This will retroactively apply to existing villagers with missing trade slots.

README.md

@@ -23,7 +23,6 @@ Disabling all rules effectively disables the mod.
 - **Cartographers are excluded from generating more different maps than they would in vanilla.** Maps they create are never forgotten by the game, and lock their structures from appearing on other maps. This would cause issues with Daily Rerolls, as cartographers would generate a lot of maps that would never be sold.   
 If you don't use Daily Rerolls and wish to let them generate new maps, you can roll-back to version 1.0.1 of the mod for now.
 
-## Known issues
+## Technical details
 
-- **When a cartographer re-rolls depleted trades in a world set to generate with no structures**, they may have trouble generating their highest level trades. In particular, it is impossible for them to yield a Master level trade, unless they reroll several specific trade slots at once.  
-*High-level trades are never definitely lost.* They will still reappear eventually. Daily rerolls in particular, are guaranteed to yield the appropriate trades, and erase any oddities.
+This mod runs with the assumption that villagers have 2 trades per level, and 1 master trade. It will always try to enforce this layout when rerolling. If a villagers is unable to yield enough trades for a given level, (e.g: Explorer Map trades in worlds with no structure), their trade list will be padded with empty trades in order to enforce the layout.

gradle.properties

@@ -9,7 +9,7 @@ yarn_mappings=1.20.1+build.10
 loader_version=0.14.22
 
 # Mod Properties
-mod_version=1.0.2
+mod_version=1.0.3
 maven_group=tk.estecka.shiftingwares
 archives_base_name=shifting-wares
 

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

@@ -3,6 +3,8 @@
 import net.fabricmc.api.ModInitializer;
 import net.fabricmc.fabric.api.gamerule.v1.GameRuleFactory;
 import net.fabricmc.fabric.api.gamerule.v1.GameRuleRegistry;
+import net.minecraft.item.ItemStack;
+import net.minecraft.village.TradeOffer;
 import net.minecraft.world.GameRules;
 import net.minecraft.world.GameRules.BooleanRule;
 
@@ -12,12 +14,11 @@
 public class ShiftingWares implements ModInitializer
 {
 	static public final Logger LOGGER = LoggerFactory.getLogger("Shifting-Wares");
-	static public GameRules.Key<BooleanRule> DAILY_RULE;
-	static public GameRules.Key<BooleanRule> DEPLETED_RULE;
+	static public final GameRules.Key<BooleanRule> DAILY_RULE    = GameRuleRegistry.register("shiftingWares.dailyReroll",   GameRules.Category.MOBS, GameRuleFactory.createBooleanRule(true));
+	static public final GameRules.Key<BooleanRule> DEPLETED_RULE = GameRuleRegistry.register("shiftingWares.depleteReroll", GameRules.Category.MOBS, GameRuleFactory.createBooleanRule(true));
+	static public final TradeOffer PLACEHOLDER_TRADE = new TradeOffer(ItemStack.EMPTY, ItemStack.EMPTY, ItemStack.EMPTY, 0, 0, 0, 0, 0);
 
 	@Override
 	public void onInitialize() {
-		DAILY_RULE    = GameRuleRegistry.register("shiftingWares.dailyReroll",   GameRules.Category.MOBS, GameRuleFactory.createBooleanRule(true));
-		DEPLETED_RULE = GameRuleRegistry.register("shiftingWares.depleteReroll", GameRules.Category.MOBS, GameRuleFactory.createBooleanRule(true));
 	}
 }

src/main/java/tk/estecka/shiftingwares/TradeShuffler.java

@@ -1,14 +1,13 @@
 package tk.estecka.shiftingwares;
 
 import java.util.ArrayList;
-import java.util.List;
-import org.jetbrains.annotations.Nullable;
 import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
 import net.minecraft.entity.passive.VillagerEntity;
 import net.minecraft.util.math.random.Random;
 import net.minecraft.village.TradeOffer;
 import net.minecraft.village.TradeOfferList;
 import net.minecraft.village.TradeOffers;
+import net.minecraft.village.VillagerData;
 import net.minecraft.village.TradeOffers.Factory;
 import net.minecraft.village.VillagerProfession;
 
@@ -19,6 +18,8 @@ public class TradeShuffler
 	private final boolean depletedOnly;
 
 	private final VillagerProfession job;
+	private final int jobLevel;
+
 	private final Random random;
 	private final TradeOfferList offers;
 	private final Int2ObjectMap<Factory[]> jobPool;
@@ -31,92 +32,79 @@ public TradeShuffler(VillagerEntity villager, boolean depletedOnly)
 
 		this.offers = villager.getOffers();
 		this.job = villager.getVillagerData().getProfession();
+		this.jobLevel = villager.getVillagerData().getLevel();
 		this.jobPool = TradeOffers.PROFESSION_TO_LEVELED_TRADE.get(job);
 		this.random = villager.getRandom();
 	}
 
 	public void	Reroll(){
 		if (jobPool == null){
-			ShiftingWares.LOGGER.error("No trade pool for job: {}", job);
+			ShiftingWares.LOGGER.error("No trade pool for job {}. Villager will not be rerolled.", job);
 			return;
 		}
 
 		MapTradesCache.FillCacheFromTrades(duck);
-		for (int jobLevel=0, i=0; i<offers.size(); ++jobLevel)
+		int tradeIndex = 0;
+		for (int tradeLvl=VillagerData.MIN_LEVEL; tradeLvl<=jobLevel; ++tradeLvl)
 		{
-			if (jobPool.size() <= jobLevel){
-				ShiftingWares.LOGGER.error("Not enough trade pools to fully reroll {} ({}) after index {}", villager, job, i);
-				break;
-			}
+			final int levelSize = (tradeLvl<VillagerData.MAX_LEVEL) ? 2 : 1;
 
-			boolean[] shouldReroll = new boolean[2];
-			shouldReroll[0] = !depletedOnly || offers.get(i).isDisabled();
-			shouldReroll[1] = i+1<offers.size() && ( !depletedOnly || offers.get(i+1).isDisabled() );
-			int amount = 0;
-			for (int n=0; n<2; ++n)
-				amount += shouldReroll[n] ? 1 : 0;
-			if (amount <= 0){
-				i += 2;
-				continue;
-			}
+			final TradeOffer[] rerollMap = new TradeOffer[levelSize];
+			for (int n=0; n<levelSize; ++n)
+				if (shouldReroll(tradeIndex+n))
+					rerollMap[n] = ShiftingWares.PLACEHOLDER_TRADE;
 
-			Factory[] levelPool = jobPool.get(jobLevel+1);
-			if (levelPool == null) {
-				ShiftingWares.LOGGER.error("No trade pool for job level: {} lvl{}", job, jobLevel);
-				continue;
-			}
-			else if (levelPool.length < 1) {
-				ShiftingWares.LOGGER.error("Empty trade pool for job level: {} lvl{}", job, jobLevel);
-				continue;
-			}
+			DuplicataAwareReroll(tradeLvl, rerollMap);
 
-			var newOffers = DuplicataAwareReroll(levelPool, amount, jobLevel);
-			for (int n=0; n<2; ++n) {
-				if (shouldReroll[n] && !newOffers.isEmpty()){
-					offers.set(i, newOffers.get(0));
-					newOffers.remove(0);
-					++i;
-				}
-				else if (!shouldReroll[n])
-					++i;
+			for (int n=0; n<levelSize; ++n, ++tradeIndex) {
+				while (offers.size() <= tradeIndex)
+					offers.add(ShiftingWares.PLACEHOLDER_TRADE);
+				if (rerollMap[n] != null)
+					offers.set(tradeIndex, rerollMap[n]);
 			}
 		}
 		MapTradesCache.FillCacheFromTrades(duck);
 	}
 
-	@Nullable
-	private TradeOffer	SingleReroll(Factory[] levelPool){
-		TradeOffers.Factory result = levelPool[random.nextInt(levelPool.length)];
-		return result.create(villager, random);
+	public boolean	shouldReroll(int tradeIndex){
+		return !depletedOnly 
+		    || offers.size() <= tradeIndex
+		    || offers.get(tradeIndex).isDisabled()
+		    ;
 	}
 
 	/**
-	 * May generate less than the requested amount, to try accounting for 
-	 * cartographers generating less trades in worlds with no mappable 
-	 * structure.
+	 * @param rerollMap	Will attempt to reroll every non-null entry.
 	 */
-	private List<TradeOffer>	DuplicataAwareReroll(Factory[] levelPool, int amount, int jobLevel){
-		var result = new ArrayList<TradeOffer>(2);
+	private TradeOffer[]	DuplicataAwareReroll(int tradeLvl, TradeOffer[] rerollMap){
+		Factory[] levelPool = jobPool.get(tradeLvl);
+		if (levelPool == null) {
+			ShiftingWares.LOGGER.error("No trade pool for job {} lvl.{}", job, jobLevel);
+			return rerollMap;
+		}
+		else if (levelPool.length < rerollMap.length) {
+			ShiftingWares.LOGGER.error("Trade pool smaller than expected for job {} lvl.{}", job, jobLevel);
+		}
+
 		var randomPool = new ArrayList<Factory>(levelPool.length);
 		for (var f : levelPool)
 			randomPool.add(f);
 
-		for (int i=0; i<amount; ++i) {
-			if (randomPool.isEmpty()){
-				ShiftingWares.LOGGER.error("Trade pool is smaller than the number of trades for this job level: {} lvl{}", job, jobLevel);
-				break;
-			}
-			else {
+		for (int n=0; n<rerollMap.length; ++n) 
+		if  (rerollMap[n] != null)
+		{
+			TradeOffer offer = null;
+			while (offer == null && !randomPool.isEmpty()) {
 				int roll = random.nextInt(randomPool.size());
-				TradeOffer offer = randomPool.get(roll).create(villager, random);
+				offer = randomPool.get(roll).create(villager, random);
 				randomPool.remove(roll);
-				if (offer != null)
-					result.add(offer);
-				else
-					ShiftingWares.LOGGER.warn("Failed to generate a valid offer for {} ({}) lvl{}", villager, job, jobLevel);
 			}
+			if (offer == null)
+				ShiftingWares.LOGGER.warn("Failed to generate a valid offer for {} lvl.{} ({})", job, tradeLvl, villager);
+			else
+				rerollMap[n] = offer;
 		}
-		return result;
+		return rerollMap;
 	}
 
 }

src/main/java/tk/estecka/shiftingwares/mixin/VillagerEntityMixin.java

@@ -105,7 +105,9 @@ private TradeOfferList RestockReroll(VillagerEntity me) {
 	@Redirect( method="needsRestock", at=@At(value="INVOKE", target="net/minecraft/village/TradeOffer.hasBeenUsed ()Z") )
 	private boolean RestockDepletedOnly(TradeOffer offer){
 		if (IsDepleteRerollEnabled())
-			return offer.isDisabled();
+			// Also excludes placeholder from triggering restocks. Those will be
+			// disabled, but not used.
+			return offer.hasBeenUsed() && offer.isDisabled();
 		else
 			return offer.hasBeenUsed();
 	}