Unleash the full potential of spell properties files!

- Spell properties files can now be overridden globally in config/ebwizardry/spells
- Spell properties files can now be overridden for specific worlds in [world save]/data/spells
- Moved the main config file to the new ebwizardry folder because it makes more sense for it to be there now
- Probably broke something in the process

src/main/java/electroblob/wizardry/Settings.java

@@ -17,6 +17,7 @@ import net.minecraftforge.fml.common.event.FMLPreInitializationEvent;
 import net.minecraftforge.fml.common.network.simpleimpl.IMessage;
 import net.minecraftforge.fml.relauncher.Side;
 
+import java.io.File;
 import java.util.*;
 import java.util.regex.Pattern;
 
@@ -364,7 +365,7 @@ public final class Settings {
 	 */
 	void initConfig(FMLPreInitializationEvent event){
 
-		config = new Configuration(event.getSuggestedConfigurationFile());
+		config = new Configuration(new File(Wizardry.configDirectory, Wizardry.MODID + ".cfg"));
 		config.load();
 
 		Wizardry.logger.info("Setting up main config");

src/main/java/electroblob/wizardry/Wizardry.java

@@ -31,6 +31,8 @@ import net.minecraftforge.fml.common.network.NetworkRegistry;
 import net.minecraftforge.fml.common.registry.GameRegistry;
 import org.apache.logging.log4j.Logger;
 
+import java.io.File;
+
 /**
  * <i>"Electroblob's Wizardry adds an RPG-like system of spells to Minecraft, with the aim of being as playable as
  * possible. No crazy constructs, no perk trees, no complex recipes - simply find spell books, cast spells, and master
@@ -82,6 +84,10 @@ public class Wizardry {
 	 * - <b>INFO</b>: Anything that might happen during normal mod operation that the user needs to know about. */
 	public static Logger logger;
 
+	/** A {@link File} object representing wizardry's config folder, {@code config/ebwizardry}). As of wizardry 4.2.4,
+	 * this folder contains the main config file and the global spell properties folder, if used. */
+	public static File configDirectory;
+
 	// The instance of wizardry that Forge uses.
 	@Instance(Wizardry.MODID)
 	public static Wizardry instance;
@@ -97,6 +103,7 @@ public class Wizardry {
 		
 		proxy.registerResourceReloadListeners();
 
+		configDirectory = new File(event.getModConfigurationDirectory(), Wizardry.MODID);
 		settings.initConfig(event);
 
 		// Capabilities

src/main/java/electroblob/wizardry/spell/Spell.java

@@ -29,6 +29,10 @@ import net.minecraft.util.text.ITextComponent;
 import net.minecraft.util.text.TextComponentTranslation;
 import net.minecraft.world.World;
 import net.minecraftforge.event.RegistryEvent;
+import net.minecraftforge.event.world.WorldEvent;
+import net.minecraftforge.fml.common.Mod;
+import net.minecraftforge.fml.common.eventhandler.SubscribeEvent;
+import net.minecraftforge.fml.common.network.FMLNetworkEvent;
 import net.minecraftforge.fml.relauncher.Side;
 import net.minecraftforge.fml.relauncher.SideOnly;
 import net.minecraftforge.registries.ForgeRegistry;
@@ -84,6 +88,7 @@ import java.util.stream.Collectors;
  * @see ItemScroll ItemScroll
  * @see Spells
  */
+@Mod.EventBusSubscriber
 public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements Comparable<Spell> {
 
 	// Spell checklist:
@@ -117,6 +122,8 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 	private final String unlocalisedName;
 	/** This spell's associated SpellProperties object. */
 	private SpellProperties properties;
+	/** A reference to the global spell properties for this spell, so they are only loaded once. */
+	private SpellProperties globalProperties;
 	/** Used in initialisation. */
 	private Set<String> propertyKeys = new HashSet<>();
 
@@ -250,6 +257,7 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 
 		if(!arePropertiesInitialised()){
 			this.properties = properties;
+			if(this.globalProperties == null) this.globalProperties = properties;
 		}else{
 			Wizardry.logger.info("A mod attempted to set a spell's properties, but they were already initialised.");
 		}
@@ -266,9 +274,14 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 					.map(s -> s.properties).toArray(SpellProperties[]::new)));
 		}else{
 			// On the client side, wipe the spell properties so the new ones can be set
-			for(Spell spell : registry){
+			// TESTME: Can we guarantee this happens before the packet arrives?
-				spell.properties = null; // TESTME: Can we guarantee this happens before the packet arrives?
+			clearProperties();
-			}
+		}
+	}
+
+	private static void clearProperties(){
+		for(Spell spell : registry){
+			spell.properties = null;
 		}
 	}
 
@@ -871,4 +884,32 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 					&& (this.element == null || spell.getElement() == this.element);
 		}
 	}
+
+	// ============================================ Event handlers ==============================================
+
+	// Not ideal but it solves the reloading of spell properties without breaking encapsulation
+
+	@SubscribeEvent
+	public static void onWorldLoadEvent(WorldEvent.Load event){
+		if(!event.getWorld().isRemote){
+			if(event.getWorld().provider.getDimension() != 0) return; // Only do it once per save file
+			clearProperties();
+			SpellProperties.loadWorldSpecificSpellProperties(event.getWorld());
+			for(Spell spell : Spell.registry){
+				if(!spell.arePropertiesInitialised()) spell.setProperties(spell.globalProperties);
+			}
+		}
+	}
+
+	@SubscribeEvent
+	public static void onClientDisconnectEvent(FMLNetworkEvent.ClientDisconnectionFromServerEvent event){
+		// Why does the world UNLOAD event happen during world LOADING? How does that even work?!
+		clearProperties();
+		for(Spell spell : Spell.registry){
+			// If someone wants to access them from the menu, they'll get the global ones (not sure why you'd want to)
+			// No need to sync here since the server is about to shut down anyway
+			spell.setProperties(spell.globalProperties);
+		}
+	}
+
 }

src/main/java/electroblob/wizardry/util/SpellProperties.java

@@ -13,13 +13,16 @@ import electroblob.wizardry.spell.Spell;
 import io.netty.buffer.ByteBuf;
 import net.minecraft.util.JsonUtils;
 import net.minecraft.util.ResourceLocation;
+import net.minecraft.world.World;
 import net.minecraftforge.common.crafting.CraftingHelper;
 import net.minecraftforge.fml.common.Loader;
 import net.minecraftforge.fml.common.ModContainer;
+import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.io.IOUtils;
 
 import java.io.BufferedReader;
+import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.util.*;
@@ -40,9 +43,6 @@ import java.util.stream.Collectors;
  * @author Electroblob
  * @since Wizardry 4.2
  */
-// There is not a particular semantic reason to separate this from the Spell class itself. However, doing so means that
-// everything related to the JSON spell system is kept in one place and doesn't clutter the (already long) Spell class.
-// Additionally, it allows SpellProperties objects to be passed around during loading and syncing.
 public final class SpellProperties {
 
 	private static final Gson gson = new Gson();
@@ -245,32 +245,57 @@ public final class SpellProperties {
 	// things don't work as expected then that may be why - pretty sure it's fine though since the properties get
 	// wiped client-side on each login anyway.
 	public static void init(){
+
 		// Collecting to a set should give us one of each mod ID
 		Set<String> modIDs = Spell.getSpells(Spell.allSpells).stream().map(s -> s.getRegistryName().getNamespace()).collect(Collectors.toSet());
 
-		boolean flag = true;
+		boolean flag = loadConfigSpellProperties();
 
 		for(String modID : modIDs){
-			flag &= loadSpellProperties(modID); // Don't short-circuit, or mods later on won't get loaded!
+			flag &= loadBuiltInSpellProperties(modID); // Don't short-circuit, or mods later on won't get loaded!
 		}
 
 		if(!flag) Wizardry.logger.warn("Some spell property files did not load correctly; this will likely cause problems later!");
 	}
 
-	// Sooooooo I just realised that resource packs - you know, that famously client-side thing - can now define
+	// There are now three 'layers' of spell properties - in order of priority, these are:
-	// stuff that should be specified by the server. No wonder it all got moved to data packs in 1.13...
+	// 1. World-specific properties, stored in saves/[world]/data/spells
-	// Anyway, for the time being we're in 1.12 so we're gonna have to do this instead.
+	// 2. Global overrides, stored in config/ebwizardry/spells
+	// 3. Built-in properties, stored in mods/[mod jar]/assets/spells
+	// There's a method for loading each of these below, because that makes sense to me!
+
+	public static void loadWorldSpecificSpellProperties(World world){
+
+		Wizardry.logger.info("Loading custom spell properties for world {}", world.getWorldInfo().getWorldName());
+
+		File spellJSONDir = new File(new File(world.getSaveHandler().getWorldDirectory(), "data"), "spells");
+
+		if(spellJSONDir.mkdirs()) return; // If it just got created it can't possibly have anything inside
+
+		if(!loadSpellPropertiesFromDir(spellJSONDir)) Wizardry.logger.warn("Some spell property files did not load correctly; this will likely cause problems later!");
+	}
+
+	private static boolean loadConfigSpellProperties(){
+
+		Wizardry.logger.info("Loading spell properties from config folder");
+
+		File spellJSONDir = new File(Wizardry.configDirectory, "spells");
+
+		if(!spellJSONDir.exists()) return true; // If there's no global spell properties folder, do nothing
+
+		return loadSpellPropertiesFromDir(spellJSONDir);
+	}
 
 	// For crafting recipes, Forge does some stuff behind the scenes to load recipe JSON files from mods' namespaces.
 	// This leverages the same methods.
 
-	private static boolean loadSpellProperties(String modID){
+	private static boolean loadBuiltInSpellProperties(String modID){
 
 		// Yes, I know you're not supposed to do orElse(null). But... meh.
 		ModContainer mod = Loader.instance().getModList().stream().filter(m -> m.getModId().equals(modID)).findFirst().orElse(null);
 
 		if(mod == null){
-			Wizardry.logger.warn("Tried to load spell properties for mod with ID '" + modID + "', but no such mod was loaded");
+			Wizardry.logger.warn("Tried to load built-in spell properties for mod with ID '" + modID + "', but no such mod was loaded");
 			return false; // Failed!
 		}
 
@@ -279,7 +304,7 @@ public final class SpellProperties {
 		List<Spell> spells = Spell.getSpells(s -> s.getRegistryName().getNamespace().equals(modID));
 		if(modID.equals(Wizardry.MODID)) spells.add(Spells.none); // In this particular case we do need the none spell
 
-		Wizardry.logger.info("Loading spell properties for " + spells.size() + " spells in mod " + modID);
+		Wizardry.logger.info("Loading built-in spell properties for " + spells.size() + " spells in mod " + modID);
 
 		// This method is used by Forge to load mod recipes and advancements, so it's a fair bet it's the right one
 		// In the absence of Javadoc, here's what the non-obvious parameters do:
@@ -307,12 +332,16 @@ public final class SpellProperties {
 						return true;
 					}
 
-					BufferedReader reader = null;
-
 					// We want to do this regardless of whether the JSON file got read properly, because that prints its
 					// own separate warning
 					if(!spells.remove(spell)) Wizardry.logger.warn("What's going on?!");
 
+					// Ignore spells overridden in the config folder
+					// This needs to be done AFTER the above line or it'll think there are missing spell properties files
+					if(spell.arePropertiesInitialised()) return true;
+
+					BufferedReader reader = null;
+
 					try{
 
 						reader = Files.newBufferedReader(file);
@@ -348,6 +377,54 @@ public final class SpellProperties {
 
 		return success;
 	}
+
+	private static boolean loadSpellPropertiesFromDir(File dir){
+
+		boolean success = true;
+
+		for(File file : FileUtils.listFiles(dir, new String[]{"json"}, true)){
+
+			// The structure in world and config folders is subtly different in that the "spells" and mod id directories
+			// are in the opposite order, i.e. it's spells/modid/whatever.json instead of modid/spells/whatever.json
+			String relative = dir.toPath().relativize(file.toPath()).toString(); // modid\whatever.json
+			String nameAndModID = FilenameUtils.removeExtension(relative).replaceAll("\\\\", "/"); // modid/whatever
+			String modID = nameAndModID.split("/")[0]; // modid
+			String name = nameAndModID.substring(nameAndModID.indexOf('/') + 1); // whatever
+
+			ResourceLocation key = new ResourceLocation(modID, name);
+
+			Spell spell = Spell.registry.getValue(key);
+
+			// If no spell matches a particular file, log it and just ignore the file
+			if(spell == null){
+				Wizardry.logger.info("Spell properties file " + nameAndModID + ".json does not match any registered spells; ensure the filename is spelled correctly.");
+				continue;
+			}
+
+			BufferedReader reader = null;
+
+			try{
+
+				reader = Files.newBufferedReader(file.toPath());
+
+				JsonObject json = JsonUtils.fromJson(gson, reader, JsonObject.class);
+				SpellProperties properties = new SpellProperties(json, spell);
+				spell.setProperties(properties);
+
+			}catch(JsonParseException jsonparseexception){
+				Wizardry.logger.error("Parsing error loading spell property file for " + key, jsonparseexception);
+				success = false;
+			}catch(IOException ioexception){
+				Wizardry.logger.error("Couldn't read spell property file for " + key, ioexception);
+				success = false;
+			}finally{
+				IOUtils.closeQuietly(reader);
+			}
+		}
+
+		return success;
+	}
+
 }
 
 // We probably could have used the attribute system for all of this, but I am reluctant to do so for a number of