Add chainable method for assigning spell NPC selector

src/main/java/electroblob/wizardry/registry/Spells.java

@@ -5,7 +5,6 @@ import electroblob.wizardry.entity.construct.*;
 import electroblob.wizardry.entity.living.*;
 import electroblob.wizardry.entity.projectile.*;
 import electroblob.wizardry.spell.*;
-import net.minecraft.entity.EntityLiving;
 import net.minecraft.init.MobEffects;
 import net.minecraft.item.EnumAction;
 import net.minecraft.util.ResourceLocation;
@@ -322,7 +321,7 @@ public final class Spells {
 		registry.register(new SpellProjectile<>("darkness_orb", EntityDarknessOrb::new).addProperties(Spell.DAMAGE, Spell.EFFECT_DURATION, Spell.EFFECT_STRENGTH).soundValues(0.5f, 0.4f, 0.2f));
 		registry.register(new ShadowWard());
 		registry.register(new Decay());
-		registry.register(new SpellBuff("water_breathing", 0.3f, 0.3f, 1, () -> MobEffects.WATER_BREATHING){ @Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return false; } }.soundValues(0.7f, 1.2f, 0.4f));
+		registry.register(new SpellBuff("water_breathing", 0.3f, 0.3f, 1, () -> MobEffects.WATER_BREATHING).npcSelector((e, o) -> false).soundValues(0.7f, 1.2f, 0.4f));
 		registry.register(new Tornado());
 		registry.register(new Glide());
 		registry.register(new SummonSpiritHorse());
@@ -373,7 +372,7 @@ public final class Spells {
 		registry.register(new Intimidate());
 		registry.register(new Banish());
 		registry.register(new SixthSense());
-		registry.register(new SpellBuff("darkvision", 0, 0.4f, 0.7f, () -> MobEffects.NIGHT_VISION){ @Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return false; } }.soundValues(0.7f, 1.2f, 0.4f));
+		registry.register(new SpellBuff("darkvision", 0, 0.4f, 0.7f, () -> MobEffects.NIGHT_VISION).npcSelector((e, o) -> false).soundValues(0.7f, 1.2f, 0.4f));
 		registry.register(new Clairvoyance());
 		registry.register(new PocketWorkbench());
 		registry.register(new ImbueWeapon());

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

@@ -43,6 +43,7 @@ import net.minecraftforge.registries.IForgeRegistryEntry;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import java.util.*;
+import java.util.function.BiPredicate;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
@@ -150,8 +151,16 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 	/** The pitch variation of the sound played when this spell is cast. Defaults to 0. */
 	protected float pitchVariation = 0;
 
+	// The following two fields are supposed to eliminate the need for boilerplate classes in spell packs.
+	// As an example, spells in the twilight forest spell pack only appear on custom spell book and scroll items and
+	// NPCs can only cast them if they spawned in the twilight forest. Without these convenience fields, spells that
+	// would not otherwise require their own classes have to have one just so they can override those two behaviours.
+
 	/** List of items for which this spell is applicable (used by default behaviour of {@link Spell#applicableForItem(Item)}). */
 	protected Item[] applicableItems;
+	/** Predicate that specifies a condition that NPCs must satisfy in order to spawn with this spell equipped (used by
+	 * default behaviour of {@link Spell#canBeCastBy(EntityLiving, boolean)}). */
+	protected BiPredicate<EntityLiving, Boolean> npcSelector; // Kinda ugly but it's better than boilerplate classes
 
 	private static int nextSpellId = 0;
 	/** The spell's integer ID, mainly used for networking. */
@@ -191,6 +200,7 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 		this.sounds = createSounds();
 		this.id = nextSpellId++;
 		this.items(WizardryItems.spell_book, WizardryItems.scroll);
+		this.npcSelector((e, o) -> canBeCastByNPCs()); // Fallback to old behaviour until we remove it entirely
 	}
 
 	// ========================================= Initialisation methods ===========================================
@@ -454,13 +464,17 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 	/**
 	 * Whether the given entity can cast this spell. If you have overridden
 	 * {@link Spell#cast(World, EntityLiving, EnumHand, int, EntityLivingBase, SpellModifiers)}, you should override
-	 * this to return true (either always or under certain circumstances).
+	 * this to return true (either always or under certain circumstances), or alternatively assign an NPC selector via
+	 * {@link Spell#npcSelector(BiPredicate)} (recommended for general spell classes).
 	 * @param npc The entity to query.
-	 * @param override True if a player in creative mode is assigning this spell the given entity, false otherwise.
+	 * @param override True if a player in creative mode is assigning this spell to the given entity, false otherwise.
 	 *                 Usually this means situational conditions should be ignored.
 	 */
+	// We could make this final and force everyone to move over to the predicate system, but for particularly complex
+	// behaviour (i.e. several lines of code) it gets too ugly, and then you end up moving the contents of the predicate
+	// to a static method anyway and referring to it via method reference... so we may as well leave people the option.
 	public boolean canBeCastBy(EntityLiving npc, boolean override){
-		return canBeCastByNPCs();
+		return npcSelector.test(npc, override);
 	}
 
 	/**
@@ -592,37 +606,6 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 		return properties.getBaseValue(identifier);
 	}
 
-	/** Returns whether the spell is enabled in any of the given {@link electroblob.wizardry.util.SpellProperties.Context Context}s.
-	 * A spell may be disabled globally in the config, or it may be disabled for one or more specific contexts in
-	 * its JSON file using a resource pack. If called with no arguments, defaults to any context, i.e. only returns
-	 * false if the spell is completely disabled in all contexts. */
-	public final boolean isEnabled(SpellProperties.Context... contexts){
-		return enabled && (contexts.length == 0 || properties.isEnabled(contexts));
-	}
-
-	/** Sets whether the spell is enabled or not. */
-	public final void setEnabled(boolean isEnabled){
-		this.enabled = isEnabled;
-	}
-
-	/** Returns true if the given item has a variant for this spell. By default, returns true if the given item is
-	 * in this spell's {@link Spell#applicableItems} list (set using {@link Spell#items(Item...)}). Override to do
-	 * something more complex. */
-	public boolean applicableForItem(Item item){
-		return Arrays.asList(applicableItems).contains(item);
-	}
-
-	/**
-	 * Sets which items this spell can appear on (these default to the regular spell book and scroll).
-	 * @param applicableItems The items this spell should naturally appear on (or no items at all).
-	 * @return The spell instance, allowing this method to be chained onto the constructor. Note that since this method
-	 * only returns a {@code Spell}, if you are chaining multiple methods onto the constructor this should be called last.
-	 */
-	public Spell items(Item... applicableItems){
-		this.applicableItems = applicableItems;
-		return this;
-	}
-
 	/**
 	 * Returns the unlocalised name of the spell, without any prefixes or suffixes, e.g. "flame_ray". <b>This should
 	 * only be used for translation purposes.</b>
@@ -809,6 +792,51 @@ public abstract class Spell extends IForgeRegistryEntry.Impl<Spell> implements C
 		}
 	}
 
+	// ============================================= Misc methods ===============================================
+
+	/** Returns whether the spell is enabled in any of the given {@link electroblob.wizardry.util.SpellProperties.Context Context}s.
+	 * A spell may be disabled globally in the config, or it may be disabled for one or more specific contexts in
+	 * its JSON file using a resource pack. If called with no arguments, defaults to any context, i.e. only returns
+	 * false if the spell is completely disabled in all contexts. */
+	public final boolean isEnabled(SpellProperties.Context... contexts){
+		return enabled && (contexts.length == 0 || properties.isEnabled(contexts));
+	}
+
+	/** Sets whether the spell is enabled or not. */
+	public final void setEnabled(boolean isEnabled){
+		this.enabled = isEnabled;
+	}
+
+	/** Returns true if the given item has a variant for this spell. By default, returns true if the given item is
+	 * in this spell's {@link Spell#applicableItems} list (set using {@link Spell#items(Item...)}). Override to do
+	 * something more complex. */
+	public boolean applicableForItem(Item item){
+		return Arrays.asList(applicableItems).contains(item);
+	}
+
+	/**
+	 * Sets which items this spell can appear on (these default to the regular spell book and scroll).
+	 * @param applicableItems The items this spell should naturally appear on (or no items at all).
+	 * @return The spell instance, allowing this method to be chained onto the constructor. Note that since this method
+	 * only returns a {@code Spell}, if you are chaining multiple methods onto the constructor this should be called last.
+	 */
+	public Spell items(Item... applicableItems){
+		this.applicableItems = applicableItems;
+		return this;
+	}
+
+	/**
+	 * Specifies a condition that NPCs must satisfy in order to spawn with this spell equipped (this defaults to always
+	 * true).
+	 * @param selector A condition that NPCs must satisfy in order to spawn with this spell equipped.
+	 * @return The spell instance, allowing this method to be chained onto the constructor. Note that since this method
+	 * only returns a {@code Spell}, if you are chaining multiple methods onto the constructor this should be called last.
+	 */
+	public Spell npcSelector(BiPredicate<EntityLiving, Boolean> selector){
+		this.npcSelector = selector;
+		return this;
+	}
+
 	// Spells are sorted according to tier and element. Where several spells have the same tier and element,
 	// they will remain in the order they were registered.
 	@Override

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

@@ -59,12 +59,11 @@ public class SpellArrow<T extends EntityMagicArrow> extends Spell {
 		super(modID, name, EnumAction.NONE, false);
 		this.arrowFactory = arrowFactory;
 		this.addProperties(RANGE);
+		this.npcSelector((e, o) -> true);
 	}
 	
 	@Override public boolean requiresPacket(){ return false; }
 	
-	@Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return true; }
-	
 	@Override public boolean canBeCastBy(TileEntityDispenser dispenser) { return true; }
 
 	/** Computes the velocity the projectile should be launched at to achieve the required range. */

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

@@ -68,6 +68,7 @@ public class SpellBuff extends Spell {
 		this.r = r;
 		this.g = g;
 		this.b = b;
+		this.npcSelector((e, o) -> true);
 	}
 
 	@Override
@@ -104,8 +105,6 @@ public class SpellBuff extends Spell {
 		return this;
 	}
 	
-	@Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return true; }
-	
 	@Override public boolean canBeCastBy(TileEntityDispenser dispenser) { return true; }
 
 	@Override

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

@@ -59,12 +59,11 @@ public class SpellConstruct<T extends EntityMagicConstruct> extends Spell {
 		super(modID, name, action, false);
 		this.constructFactory = constructFactory;
 		this.permanent = permanent;
+		this.npcSelector((e, o) -> true);
 		if(!permanent) this.addProperties(DURATION);
 	}
 	
 	@Override public boolean requiresPacket(){ return false; }
-
-	@Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return true; }
 	
 	@Override public boolean canBeCastBy(TileEntityDispenser dispenser) { return true; }
 	

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

@@ -55,6 +55,7 @@ public class SpellConstructRanged<T extends EntityMagicConstruct> extends SpellC
 	public SpellConstructRanged(String modID, String name, Function<World, T> constructFactory, boolean permanent){
 		super(modID, name, EnumAction.NONE, constructFactory, permanent);
 		this.addProperties(RANGE);
+		this.npcSelector((e, o) -> true);
 	}
 
 	/**
@@ -80,8 +81,6 @@ public class SpellConstructRanged<T extends EntityMagicConstruct> extends SpellC
 	}
 
 	@Override public boolean requiresPacket(){ return false; }
-
-	@Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return true; }
 	
 	@Override public boolean canBeCastBy(TileEntityDispenser dispenser) { return true; }
 	

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

@@ -67,6 +67,7 @@ public class SpellMinion<T extends EntityLiving & ISummonedCreature> extends Spe
 		super(modID, name, EnumAction.BOW, false);
 		this.minionFactory = minionFactory;
 		addProperties(MINION_LIFETIME, MINION_COUNT, SUMMON_RADIUS);
+		this.npcSelector((e, o) -> true);
 	}
 
 	/**
@@ -81,8 +82,6 @@ public class SpellMinion<T extends EntityLiving & ISummonedCreature> extends Spe
 	
 	@Override public boolean requiresPacket(){ return false; }
 	
-	@Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return true; }
-	
 	@Override public boolean canBeCastBy(TileEntityDispenser dispenser) { return true; }
 
 	@Override

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

@@ -59,12 +59,11 @@ public class SpellProjectile<T extends EntityMagicProjectile> extends Spell {
 		super(modID, name, EnumAction.NONE, false);
 		this.projectileFactory = projectileFactory;
 		addProperties(RANGE);
+		this.npcSelector((e, o) -> true);
 	}
 	
 	@Override public boolean requiresPacket(){ return false; }
 	
-	@Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return true; }
-	
 	@Override public boolean canBeCastBy(TileEntityDispenser dispenser) { return true; }
 
 	/** Computes the velocity the projectile should be launched at to achieve the required range. */

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

@@ -23,7 +23,7 @@ import javax.annotation.Nullable;
 
 /**
  * Generic superclass for all spells which use a raytrace to do something and (optionally) spawn particles along that
- * trajectory. This is for both continuous ('stream') spells and non-continuous ('bolt') spells This allows all the
+ * trajectory. This is for both continuous ('stream') spells and non-continuous ('bolt') spells. This allows all the
  * relevant code to be centralised. This class differs from most other spell superclasses in that it is abstract and as
  * such must be subclassed to define what the spell actually does. This is because ray-like spells do a wider variety of
  * different things, so it does not make sense to define more specific functions in this class since they would be
@@ -73,6 +73,7 @@ public abstract class SpellRay extends Spell {
 	public SpellRay(String modID, String name, boolean isContinuous, EnumAction action){
 		super(modID, name, action, isContinuous);
 		this.addProperties(RANGE);
+		this.npcSelector((e, o) -> true);
 	}
 	
 	// Although this class is abstract, someone might instantiate one of its subclasses more than once to make two
@@ -151,9 +152,7 @@ public abstract class SpellRay extends Spell {
 		this.aimAssist = aimAssist;
 		return this;
 	}
-	
-	@Override public boolean canBeCastBy(EntityLiving npc, boolean override){ return true; }
-	
+
 	@Override public boolean canBeCastBy(TileEntityDispenser dispenser) { return true; }
 
 	// Finally everything in here is standardised and written in a form that's actually readable - it was long overdue!