Cartographers crash fix (#1)

Fixed a crash that would occur after a Cartographer failed to generate a map trade in a world that generates no structure.

README.md

@@ -6,15 +6,25 @@ The most noticeable effects will be felt on any profession able to sell enchante
 
 ## Gamerules
 
-There are two gamerules that control when trades can be re-rolled. When creating a new world, those rules are On by default, and found under the "Mobs" category. Disabling all rules effectively nullifies the mod.
+There are two gamerules that control when trades can be re-rolled. When creating a new world, both rules are On by default, and found under the "Mobs" category.  
+Disabling all rules effectively disables the mod.
 - `shiftingWares.dailyReroll`:
-	Causes villager to reroll **all** their offers once per day, the first time they restock at their job station.
+	Causes villagers to reroll **all** their offers once per day, the first time they restock at their job station.
 - `shiftingWares.depleteReroll`:
-	Causes villager to re-roll any **fully depleted** trade offer, whenever they restock at their job station.
+	Causes villagers to re-roll any **fully depleted** trade offer, whenever they restock at their job station.
 	This also prevents offers from being refilled, if they have a remaining uses.
 
 ## Caveats
 
-When re-rolling depleted trades, there is a chance that a villager may end up with the same offer twice. However this can only happen when re-rolling a single trade for a given job level.
+- When re-rolling depleted trades, there is a chance that a villager may end up with the same offer twice. However this can only happen when re-rolling a single trade for a given job level.
 
-The "Demand Bonus" game mechanic is mostly removed, as the demand bonus data is deleted along the offers that are being re-rolled. Any effect it may still have is uncertain.
+- The "Demand Bonus" game mechanic is mostly removed, as the demand bonus data is deleted along the offers that are being re-rolled. Any effect it may still have is uncertain.
+
+## Known issues
+
+- **When a cartographer re-rolls depleted trades in a world set to generate with no structure**, some rerolls may yield trades one level lower than they should.
+In particular, it is impossible for them to yield a Master level trade, unless they reroll at least two trades at once.  
+**This issue is not destructive**; affected trades will _not_ degenerate endlessly, but simply bounce back and forth between the intended level and the previous level. Daily reroll is guaranteed to yield appropriate trades, and erase any oddities.
+
+- **When a cartographer yields a new map trade**, a new map ID will be created. It will be saved to the disk whether you buy it or not, and lead to a new structure farther away than the previous one.  
+As a result, cartographers may end up somewhat broken when using daily-rerolls, and send you unexpectedly far away.

gradle.properties

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

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

@@ -2,10 +2,11 @@
 
 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.TradeOffers.Factory;
@@ -38,56 +39,80 @@ public void	Reroll(){
 			return;
 		}
 
-		for (int jobLevel=0; (2*jobLevel)<offers.size(); jobLevel++){
-			int i1 = jobLevel*2;
-			int i2 = i1 + 1;
+		for (int jobLevel=0, i=0; i<offers.size(); ++jobLevel)
+		{
+			if (jobPool.size() <= jobLevel){
+				ShiftingWares.LOGGER.error("Not enough trade pools to fully reroll {} ({}) after index {}", villager, job, i);
+				break;
+			}
 
-			boolean b1 = !depletedOnly || offers.get(i1).isDisabled();
-			boolean b2 = i2<offers.size() && ( !depletedOnly || offers.get(i2).isDisabled() );
+			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;
+			}
 
-
 			Factory[] levelPool = jobPool.get(jobLevel+1);
-			if (levelPool == null)
+			if (levelPool == null) {
 				ShiftingWares.LOGGER.error("No trade pool for job level: {} lvl{}", job, jobLevel);
-			else if (levelPool.length < 1)
+				continue;
+			}
+			else if (levelPool.length < 1) {
 				ShiftingWares.LOGGER.error("Empty trade pool for job level: {} lvl{}", job, jobLevel);
-			else if (b1 && b2)
-				DuplicataAwareReroll(i1, i2, levelPool);
-			else if (b1)
-				SingleReroll(i1, levelPool);
-			else if (b2)
-				SingleReroll(i2, levelPool);
-		}
+				continue;
+			}
 
+			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;
+			}
+		}
 	}
 
-	private void	SingleReroll(int i, Factory[] levelPool){
+	@Nullable
+	private TradeOffer	SingleReroll(Factory[] levelPool){
 		TradeOffers.Factory result = levelPool[random.nextInt(levelPool.length)];
-		offers.set(i, result.create(villager, random));
+		return result.create(villager, random);
 	}
 
-	private void	DuplicataAwareReroll(int i1, int i2, Factory[] levelPool){
-		if (levelPool.length >= 2) 
-		{
-			List<Factory> randomPool = new ArrayList<Factory>(levelPool.length);
-			for (var f : levelPool)
-				randomPool.add(f);
-			
-			int roll;
-			roll = random.nextInt(randomPool.size());
-			offers.set(i1, randomPool.get(roll).create(villager, random));
+	/**
+	 * May generate less than the requested amount, to try accounting for 
+	 * cartographers generating less trades in worlds with no mappable 
+	 * structure.
+	 */
+	private List<TradeOffer>	DuplicataAwareReroll(Factory[] levelPool, int amount, int jobLevel){
+		var result = new ArrayList<TradeOffer>(2);
+		var randomPool = new ArrayList<Factory>(levelPool.length);
+		for (var f : levelPool)
+			randomPool.add(f);
 
-			randomPool.remove(roll);
-
-			roll = random.nextInt(randomPool.size());
-			offers.set(i2, randomPool.get(roll).create(villager, random));
-		}
-		else 
-		{
-			ShiftingWares.LOGGER.warn("Trade pool is smaller than the number of trades for this job level: {} lvl{}", job, i1/2);
-			offers.set(i1, levelPool[0].create(villager, random));
-			offers.set(i2, levelPool[0].create(villager, random));
+		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 {
+				int roll = random.nextInt(randomPool.size());
+				TradeOffer 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);
+			}
 		}
+		return result;
 	}
-	
+
 }