[Scrymud] [gingon at WANfear.com: new spell code]

[Justin Piper]

The Spell interface looks good to me, but for some of these member
functions I don't think the default implementation is sane, and I'm kind
of worried about those global instances of Spell classes.

int Spell::getSpellTarget(int, const String*, critter&)
   All completed spells should override this method to find the
   appropriate target.  This should be a pure virtual function to ensure
   that a spell that doesn't implement this won't compile.

void Spell::doSpellEffects()
void Spell::doWearOffEffects()
void Spell::doMovementEffects()
void Spell::doPerTickEffects()
   I think you're going to need a critter& argument to
   doMovementEffects() for it to be very useful.  That spell event
   somehow needs to know who did the actual moving in most cases.  Even
   if the spell only does anything when the critter it was cast on
   moves, you'll still need a way to tell if it was the victim or
   someone else in the room who did the actual moving.

   I'm Not 100% sure that not overriding these is really a problem
   worthy of a note in the log, except perhaps at the DEBUG log level.
   As it stands, I think you'll just end up with tons of junk in the log
   complaining that Orb of Power doesn't have its own implementation of
   doThisEffect and doTheOtherEffect.

   Also, would doDispelEffects perhaps be a better name for doWearOffEffects?

void Spell::doFailureCanned()
   Same deal here as before, spamming the log needlessly.  I do think
   this method should call the general doFailure() method, since 99.999%
   of spells are likely to fail the same when cast by a mob as when cast
   by an object.  Speaking of which, I don't see a general doFailure().
   Oh, and unless you can think of a reason that anything other than a
   child of the Spell cast would ever need to call the doFailure*()
   methods, they should probably be protected access.

void Spell::setupSpell(int, int, char*, char*, char*, char*, char[])
   This method seems very strange to me.  Why bother with this at all?
   Couldn't the args for this be private static consts in the concrete
   classes?  This method also necessitates those global instances of
   each spell class, which seems kind of clumsy, imo.  I'll have more to
   say on those later, though.

void MobSpell::onCast(int, const String*, critter&, int, int)
void ObjSpell::onCast(int, const String*, critter&, int, int)
void DoorSpell::onCast(int, const String*, critter&, int, int)
void RoomSpell::onCast(int, const String*, critter&, int, int)
void SelfSpell::onCast(int, const String*, critter&, int, int)
   Hmm... The way you've done things, I think you can actually put all
   of these into a Spell::onCast().  Since getSpellTarget() updates a
   member variable and returns a success/failure flag, Spell::onCast()
   should be able to just blindly call member functions without ever
   knowing specificly what getSpellTarget found.  In the event that this
   isn't flexible enough, onCast() can still be overridden.

SpellSpearOfDarkness spellSpearOfDarkness
SpellOrbOfPower spellOrbOfPower
SpellHolyWord spellHolyWord
   Okay, unless there's some specific reasoning behind these that I'm
   not seeing, I think this is a bad idea.  For starters, the spell
   target is a member variable of these classes, which means
   doWearOffEffects and doPerTickEffects flat out break if two people
   ever cast the same spell!
   
   Here's my suggestion.  Write a SpellFactory class with a method like
   this:

      Spell* getSpell(String spellName, critter& caster,
                      int i_th, String* vict)

   The getSpell method should check the critter's know spells and, if
   the critter knows a spell named spellName, it should return a new
   instance of the appropriate concrete class, otherwise it should
   return NULL.  Then, change the cast() method in commands.cc so that
   it calls SpellFactory::getSpell(), and, if the result is non-null,
   calls the Spell's onCast() method.

   Doing it this way, you cut way back on the number of things that
   cast() needs to know to work.  onCast() doesn't need all those
   parameters, they can be moved to Spell::Spell() instead.  You also
   open up the way for stateful spells.  For example, a protection spell
   that affords a calculated amount of benefit, perhaps based on level,
   can properly undo its actions even if the target's level changes. The
   only problem is that you need to destroy the Spell object when it
   wears off, but that's not really a big deal, since you have to remove
   it from the affected_by list of the victim already, anyway.  I don't
   think there will be a significant memory overhead, since you can make
   a lot of those member variables (msg_lost_con, actions, diversions,
   etc) static const and share them between all instances of the class.

Anyway, the only other thing I have to say is that this might be a good
time to look at how the spells are divided up among the source files.
It seems to me like things would be easiest to manage if all the base
Spell classes were in one file, then have each concrete spell in a file
named after itself.  You'd get quite a few files when you were all said
and done, but they could all be regulated to a ./Spells directory to
keep things tidy.

--
"Picture the sun as the origin of two intersecting 6-dimensional
hyperplanes from which we can deduce a certain transformational
sequence which gives us the terminal velocity of a rubber duck ..."

On 29 Jan 2002, Edward Roper wrote:

> ----- Forwarded message from gingon at WANfear.com -----
> From: gingon at WANfear.com
> To: eroper at WANfear.com
> Subject: new spell code
> Date: Mon, 28 Jan 2002 06:31:07 -0800 (PST)
> User-Agent: IMP/PHP IMAP webmail program 2.2.3
> 
> rewriting the spell code to eliminate redundant code, and generally make it 
> work better all around. all spells will be derived from the Spell class (though 
> usually not directly). should make coding new spells alot easier.  take a look 
> at it and tell me what you think: http://www.gingon.org/spells.h
> 
> --Gingon