Sold maps reroll (#7)

Added a gamerule that allows maps to be rerolled, if they have been sold at least once.

.github/workflows/build.yml

@@ -5,9 +5,8 @@
 
 name: build
 on:
-  push:
-    tags:
-      - '*'
+  - workflow_call
+  - workflow_dispatch
 
 jobs:
   build:

.github/workflows/release.yml

@@ -0,0 +1,26 @@
+name: release
+on:
+  push:
+    tags:
+      - '*'
+
+jobs:
+  build:
+    uses: ./.github/workflows/build.yml
+  release:
+    needs: [ build ]
+    runs-on: ubuntu-22.04
+    permissions:
+      contents: write
+    steps:
+      - name: Download artifacts
+        uses: actions/download-artifact@v3
+        with:
+          name: Artifacts
+          path: ./
+      - name: Draft release
+        uses: ncipollo/[email protected]
+        with:
+          artifacts: ./*.jar
+          artifactErrorsFailBuild: true
+          draft: true

README.md

@@ -6,23 +6,31 @@ The most noticeable effects will be felt on any profession able to sell enchante
 
 ## Triggers
 
-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.  
+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.
 - `shiftingWares.depleteReroll`:
 	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
+## Exploration map trades
+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.
 
-- **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.
+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 "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 map's item name, or its translation key, is used to tell apart different types of maps.
 
-- **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.
 
 ## Technical details
+- This mod runs under the assumption that villagers have at most 2 trades per level. It will always try to enforce this layout when rerolling.  
 
-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.
+- 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.  
+These paddings are required to ensure trades are rerolled with one of equivalent level; a trade's position in the list is the only indication to its level.
+Placeholder trades will never take the place of a valid trade; they will only show up if all other options are exhausted.
+
+- The "Demand Bonus" game mechanic is mostly removed, because the demand bonus data is deleted along with the offers that are rerolled. Any effect it may still have is uncertain.
+
+- Depleted rerolls have a chance to yield duplicate trades.

build.gradle

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

changelog.md

@@ -0,0 +1,19 @@
+# v1
+## 1.0
+### 1.0.0
+Initial release
+### 1.0.1
+- Fixed a crash that would occur after a Cartographer failed to generate a map trade in a world that generates no structure.
+### 1.0.2
+**This version contains a critical bug and should not be used**
+- Force cartographers remember the maps they generated, and always resell it.
+### 1.0.3
+**This version contains a critical bug and should not be used**
+- Introduced placeholder trades. Fixes cartographers having trouble generating high-level trades in worlds with no structures.
+### 1.0.4
+- Fixes mapping issue affecting 1.0.2 and 1.0.3
+### 1.0.5
+- The level of each trade slot is evaluated more accurately for every job.
+
+## 1.1
+- Added a gamerules that allows cartographers to regenerate maps that have been sold at least once.

gradle.properties

@@ -6,12 +6,12 @@ org.gradle.parallel=true
 # check these on https://fabricmc.net/develop
 minecraft_version=1.20.1
 yarn_mappings=1.20.1+build.10
-loader_version=0.14.22
+loader_version=0.15.3
+
+#Fabric api
+fabric_version=0.91.0+1.20.1
 
 # Mod Properties
-mod_version=1.0.5
+mod_version=1.1.0
 maven_group=tk.estecka.shiftingwares
 archives_base_name=shifting-wares
-
-# Dependencies
-fabric_version=0.86.1+1.20.1

gradle/wrapper/gradle-wrapper.properties

@@ -1,6 +1,6 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-8.2-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.5-bin.zip
 networkTimeout=10000
 validateDistributionUrl=true
 zipStoreBase=GRADLE_USER_HOME

gradlew

@@ -83,7 +83,8 @@ done
 # This is normally unused
 # shellcheck disable=SC2034
 APP_BASE_NAME=${0##*/}
-APP_HOME=$( cd "${APP_HOME:-./}" && pwd -P ) || exit
+# Discard cd standard output in case $CDPATH is set (https://github.com/gradle/gradle/issues/25036)
+APP_HOME=$( cd "${APP_HOME:-./}" > /dev/null && pwd -P ) || exit
 
 # Use the maximum available, or set MAX_FD != -1 to use that value.
 MAX_FD=maximum
@@ -144,15 +145,15 @@ if ! "$cygwin" && ! "$darwin" && ! "$nonstop" ; then
     case $MAX_FD in #(
       max*)
         # In POSIX sh, ulimit -H is undefined. That's why the result is checked to see if it worked.
-        # shellcheck disable=SC3045
+        # shellcheck disable=SC2039,SC3045
         MAX_FD=$( ulimit -H -n ) ||
             warn "Could not query maximum file descriptor limit"
     esac
     case $MAX_FD in  #(
       '' | soft) :;; #(
       *)
         # In POSIX sh, ulimit -n is undefined. That's why the result is checked to see if it worked.
-        # shellcheck disable=SC3045
+        # shellcheck disable=SC2039,SC3045
         ulimit -n "$MAX_FD" ||
             warn "Could not set maximum file descriptor limit to $MAX_FD"
     esac
@@ -201,11 +202,11 @@ fi
 # Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
 DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
 
-# Collect all arguments for the java command;
-#   * $DEFAULT_JVM_OPTS, $JAVA_OPTS, and $GRADLE_OPTS can contain fragments of
-#     shell script including quotes and variable substitutions, so put them in
-#     double quotes to make sure that they get re-expanded; and
-#   * put everything else in single quotes, so that it's not re-expanded.
+# Collect all arguments for the java command:
+#   * DEFAULT_JVM_OPTS, JAVA_OPTS, JAVA_OPTS, and optsEnvironmentVar are not allowed to contain shell fragments,
+#     and any embedded shellness will be escaped.
+#   * For example: A user cannot expect ${Hostname} to be expanded, as it is an environment variable and will be
+#     treated as '${Hostname}' itself on the command line.
 
 set -- \
         "-Dorg.gradle.appname=$APP_BASE_NAME" \

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

@@ -1,10 +1,6 @@
 package tk.estecka.shiftingwares;
 
-import java.util.Optional;
-import net.minecraft.item.ItemStack;
-
 public interface IVillagerEntityDuck 
 {
-	Optional<ItemStack>	GetCachedMap(String key);
-	void	AddCachedMap(String key, ItemStack mapItem);
+	MapTradesCache shiftingwares$GetTradeCache();
 }

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

@@ -1,66 +1,154 @@
 package tk.estecka.shiftingwares;
 
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
-import net.minecraft.entity.passive.VillagerEntity;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import org.jetbrains.annotations.Nullable;
 import net.minecraft.item.FilledMapItem;
 import net.minecraft.item.ItemStack;
 import net.minecraft.item.Items;
 import net.minecraft.nbt.NbtCompound;
+import net.minecraft.nbt.NbtElement;
+import net.minecraft.nbt.NbtList;
+import net.minecraft.nbt.NbtString;
 import net.minecraft.text.TextContent;
 import net.minecraft.text.TranslatableTextContent;
+import net.minecraft.village.TradeOffer;
 import net.minecraft.village.TradeOfferList;
 
 public class MapTradesCache 
 {
+	static public final int DATA_FORMAT = 1;
+	static public final String FORMAT_KEY  = "shifting-wares:data_format";
 	static public final String MAPID_CACHE = "shifting-wares:created_maps";
+	static public final String SOLD_ITEMS  = "shifting-wares:sold_items";
 
-	static public void	FillCacheFromTrades(VillagerEntity villager){
-		IVillagerEntityDuck villagerMixin = (IVillagerEntityDuck)villager;
-		TradeOfferList offers = villager.getOffers();
-		for (int i=0; i<offers.size(); ++i)
+	private final Map<String, ItemStack> cachedItems = new HashMap<String,ItemStack>();
+	private final Set<String> soldItems = new HashSet<>();
+
+	/**
+	 * @return null if the item needs not or cannot be cached.
+	 */
+	@Nullable
+	static public String	FindCacheKey(ItemStack item){
+		if (!item.isOf(Items.FILLED_MAP))
+			return null;
+
+		if (!item.hasCustomName()){
+			ShiftingWares.LOGGER.error("Unable to identify map#{} with no name:\n{}", FilledMapItem.getMapId(item), item);
+			return null;
+		}
+
+		TextContent fullName = item.getName().getContent();
+		String key;
+		if (fullName instanceof TranslatableTextContent translatable)
+			key = translatable.getKey();
+		else {
+			ShiftingWares.LOGGER.error("Map#{} name is not a translation key: {} {}", FilledMapItem.getMapId(item), fullName.getClass(), fullName);
+			key = item.getName().getString();
+		}
+
+		return key;
+	}
+
+	public Optional<ItemStack>	GetCachedMap(String key){
+		if (this.cachedItems.containsKey(key))
+			return Optional.of(this.cachedItems.get(key));
+		else
+			return Optional.empty();
+	}
+
+	public void	AddCachedMap(String key, ItemStack mapItem){
+		Integer neoId=FilledMapItem.getMapId(mapItem);
+		if (!cachedItems.containsKey(key))
+			ShiftingWares.LOGGER.info("New map trade: #{} @ {}", neoId, key);
+		else
 		{
-			ItemStack sellItem = offers.get(i).getSellItem();
-			if (!sellItem.isOf(Items.FILLED_MAP))
-				continue;
+			Integer oldId=FilledMapItem.getMapId(cachedItems.get(key));
+			if (soldItems.contains(key))
+				ShiftingWares.LOGGER.info("New map trade #{}->#{} @ {}", oldId, neoId, key);
+			else if (Objects.equals(neoId, oldId))
+				ShiftingWares.LOGGER.warn("Updating existing map trade #{} @ {}", neoId, key);
+			else
+				ShiftingWares.LOGGER.error("Overwriting existing map trade: #{}->#{} @ {}", oldId, neoId, key);
+		}
+
+		cachedItems.put(key, mapItem);
+		soldItems.remove(key);
+	}
+
+	public boolean HasSold(String key){
+		return soldItems.contains(key);
+	}
 
-			if (!sellItem.hasCustomName()){
-				ShiftingWares.LOGGER.error("Unable to identify map#{} with no name in slot {} of {}\n{}", FilledMapItem.getMapId(sellItem), i, villager, sellItem);
+	/**
+	 * 1. Scans the trade list for maps that might not have been cached.
+	 * This is  only  really useful  the first time  a villager  is loaded after
+	 * installing the mod. The rest of the time, it's paranoid safeguard.
+	 * 
+	 * 2. Detects maps  that  have been  sold  at least once, and marks them  as
+	 * potentially re-rollable.
+	 */
+	public void	FillCacheFromTrades(TradeOfferList offers){
+		for (TradeOffer offer : offers) 
+		{
+			ItemStack sellItem = offer.getSellItem();
+			String cacheKey = FindCacheKey(sellItem);
+			if (cacheKey == null)
 				continue;
-			}
 
-			TextContent fullName = sellItem.getName().getContent();
-			String nameKey;
-			if (fullName instanceof TranslatableTextContent)
-				nameKey = ((TranslatableTextContent)fullName).getKey();
-			else {
-				ShiftingWares.LOGGER.error("Map name is not a translatation key: {}", fullName);
-				nameKey = sellItem.getName().getString();
+			if (offer.hasBeenUsed()) {
+				this.soldItems.add(cacheKey);
+				ShiftingWares.LOGGER.info("Marked map as sold: #{} @ {}", FilledMapItem.getMapId(sellItem), cacheKey);
 			}
 
-			var oldItem = villagerMixin.GetCachedMap(nameKey);
+			var oldItem = this.GetCachedMap(cacheKey);
 			if (oldItem.isEmpty() || !ItemStack.areEqual(sellItem, oldItem.get())){
-				ShiftingWares.LOGGER.warn("Caught a map trade that wasn't properly cached: #{} @ {}", FilledMapItem.getMapId(sellItem), villagerMixin);
-				villagerMixin.AddCachedMap(nameKey, sellItem);
+				ShiftingWares.LOGGER.warn("Caught a map trade that wasn't properly cached: #{} @ {}", FilledMapItem.getMapId(sellItem), cacheKey);
+				this.AddCachedMap(cacheKey, sellItem);
 			}
 		}
 	}
 
-	static public Map<String,ItemStack>	ReadMapCacheFromNbt(NbtCompound nbt, Map<String, ItemStack> map){
-		NbtCompound nbtmap = nbt.getCompound(MAPID_CACHE);
-		if (nbtmap == null)
-			return map;
 
-		for (String key : nbtmap.getKeys())
-			map.put(key, ItemStack.fromNbt(nbtmap.getCompound(key)));
-		return map;
+/******************************************************************************/
+/* # Serialization                                                            */
+/******************************************************************************/
+
+	public void	ReadMapCacheFromNbt(NbtCompound nbt){
+		NbtCompound nbtcache = nbt.getCompound(MAPID_CACHE);
+		NbtList nbtsold = nbt.getList(SOLD_ITEMS, NbtElement.STRING_TYPE);
+
+		if (nbtcache != null)
+		for (String key : nbtcache.getKeys()){
+			ItemStack item = ItemStack.fromNbt(nbtcache.getCompound(key));
+			this.cachedItems.put(key, item);
+		}
+
+		if (nbtsold != null)
+		for (int i=0; i<nbtsold.size(); ++i){
+			String key = nbtsold.getString(i);
+			this.soldItems.add(key);
+		}
 	}
 
-	static public NbtCompound	WriteMapCacheToNbt(NbtCompound nbt, Map<String, ItemStack> map){
-		NbtCompound nbtmap = new NbtCompound();
-		for (var pair : map.entrySet())
-			nbtmap.put(pair.getKey(), pair.getValue().writeNbt(new NbtCompound()));
+	public NbtCompound	WriteMapCacheToNbt(NbtCompound nbt){
+		NbtCompound nbtcache = new NbtCompound();
+		NbtList nbtsold = new NbtList();
+
+		for (var pair : this.cachedItems.entrySet())
+			nbtcache.put(pair.getKey(), pair.getValue().writeNbt(new NbtCompound()));
+		for (String key : this.soldItems)
+			nbtsold.add(NbtString.of(key));
+
+		nbt.putInt(FORMAT_KEY, DATA_FORMAT);
+		if (!nbtcache.isEmpty()) nbt.put(MAPID_CACHE, nbtcache);
+		if (!nbtsold.isEmpty() ) nbt.put(SOLD_ITEMS, nbtsold);
 
-		nbt.put(MAPID_CACHE, nbtmap);
 		return nbt;
 	}
+
 }

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

@@ -14,8 +14,11 @@
 public class ShiftingWares implements ModInitializer
 {
 	static public final Logger LOGGER = LoggerFactory.getLogger("Shifting-Wares");
-	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 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 GameRules.Key<BooleanRule> MAP_RULE      = GameRuleRegistry.register("shiftingWares.allowMapReroll", GameRules.Category.MOBS, GameRuleFactory.createBooleanRule(false));
+
 	static public final TradeOffer PLACEHOLDER_TRADE = new TradeOffer(ItemStack.EMPTY, ItemStack.EMPTY, ItemStack.EMPTY, 0, 0, 0, 0, 0);
 
 	@Override