Design of Area of Effect spells

Page: 1 2
ID:151696
 
Aug 9 2009, 5:19 pm (Edited by moderator on Aug 10 2009, 8:48 pm)
Yea, more likely you're going to want to use a datum for this. Below implements an area-of-effect datum, which you could use for, obviously, area-of-effect spells, as well as an effects datum, which actually does the damage to players.
#define SHAPE_SQUARE 1
#define SHAPE_CIRCLE 2

spell
    var/mob/caster // Owner of the spell.
    proc
        GetCaster()
            return caster
        GetCasterGroup()
            return caster.group

    effect
        var
            spell/master
            atom/effecter
            icon
            icon_state
            layer = MOB_LAYER + 1
            icon/overlay

        New(atom/a, spell/s)
            effecter = a
            master = s

            spawn()
                Damage()

        Del()
            effecter.overlays -= overlay

        proc
            AddOverlay()
                overlay = new(icon, icon_state)
                overlays.layer = layer
                effecter.overlays += overlay

            Damage()
               // For now, this is just a template proc.
                AddOverlay()

    area_of_effect
        var
            name
            atom/center
            radius = 5 // By default, it has a radius of
                       // five tiles.
            shape = SHAPE_CIRCLE // The shape of the area of
                                 // effect, which, by
                                 // default, takes the 
                                 // shape of a circle.
            list
                targets[0]
                effects[0]
            target_type = /mob // Attack any mob. This can
                               // also be a list.
            spell/effect/effect // The effect to use.
            time = 100 // Ten second spell.

        New(atom/loc, mob/m)
            owner = m
            center = loc

            GetTargets()
            DoEffect()
            spawn(time)
                // When the effect is finished, delete the
                // spell.
                del src

        Del()
            for(var/spell/effect/e in effects)
                // Cleaning up the effects.
                del e

        proc
            AffectedBySpell(atom/a)
                // Default action for this is that no one
                // in the caster's group is affected by the
                // spell, as well as the caster themself.
                return !((a == GetCaster()) || (a in GetCasterGroup())

            InRange(atom/a, v)
                if((shape == SHAPE_CIRCLE) && (get_dist(center, a) < radius))
                    return 1
                return a in (v || view(center, radius)

            GetTargets()
                var/list/t = view(center, radius)
                for(var/atom/movable/a in t)
                    // Presumably, you'll only want to
                    // attack mobs, but this will allow for
                    // attacking mobs and objs. If you want
                    // to, for some reason, attack turfs or
                    // objs, you'll have to edit this proc.

                    if(istype(target_type, /list) ? !(a.type in target_type) : !istype(a, target_type))
                        // If a isn't of the target type
                        // being attacked, keep going.
                        continue
                    if(!InRange(a, t))
                        // If the target is not in range,
                        // continue.
                        continue
                    if(!AffectedBySpell(t))
                        // If t is not affected by the spell,
                        // continue.
                        continue

                    targets += a // Add the target to the
                                 // targets list.

            DoEffect()
                for(var/atom/a in targets)
                    effects += new effect(a, src)


Here is an example implementation, for a circle-of-fire move:
mob/verb/CircleOfFire()
    // Create a new circle-of-fire spell, with src as the
    // caster, centered at the caster's location.
    new /spell/area_of_effect/circle_of_fire(src, loc)

spell
    area_of_effect/circle_of_fire
        name = "Circle of Fire"
        radius = 8
        target_type = list(/mob, /obj) // Will burn objs
                                       // as well.
        time = 50 // Only a five second spell.
        effect = /spell/effects/fire

        AffectedBySpell(atom/a)
            // This spell will hurt everyoneo but the
            // caster.
            return a != GetCaster()

    effects/fire
        icon = 'Effect Icons.dmi'
        icon_state = "fire"

        Damage()
            // And here is where Damage() is used.
            while(1)
                // This will keep looping so long as
                // src exists.

                sleep(3) // Do damage every three ticks.
                if(master.InRange(effecter))
                    // As long as the person being attacked
                    // by the spell is in range of the
                    // circle, keep doing a random amount
                    // of damage between 5 and 10 to them.
                    effecter.Damage(rand(5, 10))
Aug 10 2009, 8:48 am
Popisfizzy wrote:
Yea, more likely you're going to want to use a datum for this. Below implements an area-of-effect datum, which you could use for, obviously, area-of-effect spells, as well as an effects datum, which actually does the damage to players.

That's incredibly over complicated and partially pointless.
Aug 10 2009, 9:49 am
In response to Falacy
Your comments involving code design are really to be taken with more than a few grains of salt, given that you've demonstrated yourself to be quite incompetent in that regard before.
Aug 10 2009, 10:14 am
In response to Popisfizzy
Popisfizzy wrote:
Your comments involving code design are really to be taken with more than a few grains of salt, given that you've demonstrated yourself to be quite incompetent in that regard before.

Yes, those were some pretty accurate comments? If something enters something, you're not going to get a null runtime, and as just about always, your code is overly complicated and not even close to being fluidly readable.
You also forgot the &single=1 tag again =P

Lets go over some specifics of why your code fails, besides the generic statements already made, and the fact that your code won't even compile.
    proc
        GetCaster()
            return caster

There aren't private variables or pointers in DM, whats the point of this?

InRange(atom/a, v)
                if((shape == SHAPE_CIRCLE) && (get_dist(center, a) < radius))
                    return 1
                return a in (v || view(center, radius)

That won't evaluate a circle, in fact those 2 would return the exact same thing (a square). The best you could really do without getting into crazy maths would be to compare xs and ys to get a diamond shape.
And using get_dist() in the 2nd case would be more efficient than view(), unless you actually needed them to be in the literal "view".

New(atom/loc, mob/m)
new /spell/area_of_effect/circle_of_fire(src, loc)

Obviously those match up.
These datums are also never referenced on the map anywhere, only the reverse happens, so whats your plan for activating them?

Damage()
            while(1)
                sleep(3)
                //stuff done here

With the sleep at the beginning of the loop like that, not only will there be an odd delay in the effect starting, but reference variables could well become null before being accessed.

effecter.Damage(rand(5, 10))

Who damaged who? From where? With what? Guess nobody gets kill credit when spells are involved.

There's plenty more, but I think we get the point.
Aug 10 2009, 10:40 am
In response to Falacy
Falacy wrote:
Lets go over some specifics of why your code fails, besides the generic statements already made, and the fact that your code won't even compile.
    proc
>         GetCaster()
>             return caster
>

There aren't private variables or pointers in DM, whats the point of this?

InRange(atom/a, v)
>                 if((shape == SHAPE_CIRCLE) && (get_dist(center, a) < radius))
>                     return 1
>                 return a in (v || view(center, radius)
>

That won't evaluate a circle, in fact those 2 would return the exact same thing (a square). The best you could really do without getting into crazy maths would be to compare xs and ys to get a diamond shape.
And using get_dist() in the 2nd case would be more efficient than view(), unless you actually needed them to be in the literal "view".

> New(atom/loc, mob/m)
> new /spell/area_of_effect/circle_of_fire(src, loc)
>

Obviously those match up.
These datums are also never referenced on the map anywhere, only the reverse happens, so whats your plan for activating them?

> Damage()
>             while(1)
>                 sleep(3)
>                 //stuff done here
>

With the sleep at the beginning of the loop like that, not only will there be an odd delay in the effect starting, but reference variables could well become null before being accessed.

> effecter.Damage(rand(5, 10))
>

Who damaged who? From where? With what? Guess nobody gets kill credit when spells are involved.

There's plenty more, but I think we get the point.

As pissing contests go, I don't really expect much out of this. I'm not going to tackle some of the ones you raised, mostly because I want to talk code design, not code detail.

GetCaster() is a pretty simple one and it comes down to future proofing and polymorphism. For the base spell and simple casts this procedure looks pretty stupid, but let us assume you fancy a bit of chaos magic. Here's a spell: "Creates a vortex of chaos at the target location. The spell is cast by a random member of your party." Who's the caster? It's not the caster variable, it's a random pick of caster.group. By sub-typing and always using GetCaster() to get the person casting the spell though, Fizz can integrate this interesting little chao spell with no code changes to the spell execution. Neat huh? He can make the caster selection as complex as he likes, and his spell execution will deal with it without any fuss.

InRange is a code implementation thing, however I think you appreciate why InRange exists, regardless of implementation.

The map action is triggered through a call-stack to area_of_effect, calling GetTargets() (in the fire case, doing a view of 8 from the caster's location) which populates a target list, who are hit with effects. That chain is started by the verb, pretty simple stuff I would've thought.

Mmm, that's an implementation thing with Damage() really, but the "odd delay" of 3 tenths isn't too much to a user, particularly after network delay. The null issue applies whether it was at the begining or the end, there is a break after an iteration during which the variables references may be forceably nulled. Frankly if they do get forceably nulled and you don't expect it though, you have other code design issues.

This last one is probably the only code design issue I would agree with. I can think of implementation solutions involving usr that would not be 100% criminal, but you'd be better passing a reference to master.

On the whole though, it reads as a reasonably well designed unit of code, with some room for improvement (much like any first iteration of design). On the whole it's better than what I'd expect to read on these forums.
Aug 10 2009, 10:45 am
In response to Stephen001
Stephen001 wrote:
Mmm, that's an implementation thing with Damage() really, but the "odd delay" of 3 tenths isn't too much to a user, particularly after network delay. The null issue applies whether it was at the begining or the end, there is a break after an iteration during which the variables references may be forceably nulled. Frankly if they do get forceably nulled and you don't expect it though, you have other code design issues.

Practically, the sleep() should have been at the end of the loop and the while() should have verified any conditions required for the loop to continue. So, if caster needs to still exist to assign damage or whatever, it should be while(caster). If a sleep() is desired before any damage is done, then it should go before the while() loop.
Aug 10 2009, 10:48 am
In response to Garthor
Fair play on that, I hadn't considered that kind of use of the while condition. That is a sensible approach.
Aug 10 2009, 10:49 am
In response to Garthor
Garthor wrote:
Practically, the sleep() should have been at the end of the loop and the while() [..]

Yea, that bit was a bit mangled, as I apparently wasn't thinking clearly when I wrote it. sleep() should be at the end, but I think the while should more continue so long as the person being effected by the datum exists, and not the caster.
Aug 10 2009, 10:50 am
In response to Stephen001
Stephen001 wrote:
He can make the caster selection as complex as he likes, and his spell execution will deal with it without any fuss.

He could, but that's not what it does in his post.

InRange is a code implementation thing, however I think you appreciate why InRange exists, regardless of implementation.

I'm not complaining that he has an InRange() procedure, I'm complaining because his supposed SHAPE_CIRCLE calculations don't calculate anything different from the square ones, except that they ignore visibility.

The map action is triggered through a call-stack to area_of_effect, calling GetTargets() (in the fire case, doing a view of 8 from the caster's location) which populates a target list, who are hit with effects. That chain is started by the verb, pretty simple stuff I would've thought.

Which is only called one time when they spell is first created. The poster said they wanted a spell on the map that when somebody walks into it they take damage.

Mmm, that's an implementation thing with Damage() really, but the "odd delay" of 3 tenths isn't too much to a user, particularly after network delay. The null issue applies whether it was at the begining or the end, there is a break after an iteration during which the variables references may be forceably nulled. Frankly if they do get forceably nulled and you don't expect it though, you have other code design issues.

When/if effecter logs out you'd get a run-time. It's true that you'd need to spot check regardless of where the sleep() is with its current setup. Which just brings up that this entire loop should be rewritten.
Aug 10 2009, 10:55 am
In response to Falacy
I appreciate what you're saying, but that's all implementation and example, not design. You were originally suggesting there were some pretty big design faults in what he demonstrated, in the post I replied to, and that's all you seemed to discuss. I really must disagree with that for the reasons I went over in my last post.
Aug 10 2009, 10:55 am
In response to Falacy
Falacy wrote:
Yes, those were some pretty accurate comments?

No, but you're quite naive and think they were.

You also forgot the &single=1 tag again =P

Perhaps you should use the good forum view, and not the ass-ugly wall-of-text version.

and the fact that your code won't even compile.

If there are simple syntax errors, then they should be easy to figure out. I write my code on the forums, and not in the compiler, as most people who write their code on the forums do (which you seem to not actually realize). The point is not to have code that can be copy and pasted, but code that can be learned from and utilized.

There aren't private variables or pointers in DM, whats the point of this?

It may not always be the case that only one person casts the spell. Perhaps there are no casters, or a group of casters, or a very strange way of determining if something cast it. You see, this is a design feature called polymorphism, and something you should probably learn if you want to at least pretend you design code well.

That won't evaluate a circle, in fact those 2 would return the exact same thing (a square).

Indeed. I apparently mangled the GetCircle() function I was trying to call back from AbyssDragon's BasicMath library. Apparently I was off, both because get_dist() doesn't work like you'd think, and because I'm not sampling a large enough area to get a circle. If I were to edit the code, I'd change it so that InRange() checks against stored output from a GetArea() proc, or something of the sort.

Obviously those match up.
These datums are also never referenced on the map anywhere, only the reverse happens, so whats your plan for activating them?

As far as I can tell, this is absolute nonsense and has no semantic value whatsoever.

With the sleep at the beginning of the loop like that, not only will there be an odd delay in the effect starting, but reference variables could well become null before being accessed.

Yea, that was a mistake, though one easily fixed by testing the code. As I've already said, I didn't compile or run the code, and most don't, because it's not supposed to be a plug-and-run library but something to learn from.

> effecter.Damage(rand(5, 10))
>

Who damaged who? From where? With what? Guess nobody gets kill credit when spells are involved.

In this case? No. I was writing up an example of how you could use it, nothing more or less. Did you expect me to write up a full proc detailing damage, and death check, and display the damage done with floaty little numbers above the NPCs head? Maybe show them how to do leveling? Perhaps they should get a nice slice of pie and some ice cream when they've done 10 damage in a row.

Don't be so daft.
Aug 10 2009, 11:04 am
In response to Stephen001
Stephen001 wrote:
I appreciate what you're saying, but that's all implementation and example, not design. You were originally suggesting there were some pretty big design faults in what he demonstrated, in the post I replied to, and that's all you seemed to discuss. I really must disagree with that for the reasons I went over in my last post.

What are you referring to as design faults? Where did I say anything about them? What do you consider design faults? Calculating a circle as a square I'd say is a design fault. Having the spells only calculate targets when first cast, when they're supposed to be an AoE spell I'd say is a design fault. The fact that datums are being used for visual/mappable effects...
Overall this code is pretty fail. It could not only be re-written shorter, more efficiently, with better readability and better functionality, but I'd say the code doesn't even accomplish the desired results.
Aug 10 2009, 11:10 am
In response to Falacy
Falacy wrote:
What do you consider design faults? Calculating a circle as a square I'd say is a design fault.

No, that looks more like a code problem then anything to do with design.
Aug 10 2009, 11:12 am
In response to ANiChowy
ANiChowy wrote:
No, that looks more like a code problem then anything to do with design.

Which goes back to what are you considering "design faults"?
Aug 10 2009, 11:28 am
In response to Falacy
When you level a comment like "That's incredibly over complicated and partially pointless." and a full blown multiple object (datum or otherwise) code example without clarification, one would be led to assume you are commenting on it's design. If you said "That proc is incredibly over complicated and partially pointless.", one would be led to assume you are commenting on it's implementation. Without meaning to get more pissing contest about this, I base that on 6+ years of having my work reviewed, by and large by professional software engineers and software professors. If I misunderstood you, that's fine, but to me it looks like you fired a blanket statement about the entire example, then picked on design decisions like a GetCaster() proc, choice of datum and datum hierarchy etc.

Design (to the software world) concerns structure of software. There are a lot of levels to it, but it never goes down past a functional block level (proc/verb in DM's case). His choice to separate damage and target logic from visual logic is a design decision. It's not even a terribly rare one for most software either. He is essentially decoupling the view from it's respective controller. The benefits to this are pretty apparent, he is able to lever polymorphism to provide different views for the same control logic, different control logic for the same views or even just strip out one layer near entirely with relative ease. Spells that have no visual effect are pretty trivial with this separation of concerns, where-as to lever similar spells in an obj derived system would end up producing a rather odd "dummy obj" that didn't display on the screen and would need overrides to ensure it doesn't move / block / take focus where you don't want it to.

Essentially what Fizz has designed is of a level you don't see on these forums too often. It's not "omg kick-ass", however it is at least forward thinking and keeps some pretty interesting channels easily open from a gaming point of view. I'd like to see more of it, it's a sign of BYOND maturing as a community of developers.

His implementation was wonky in parts, however his implementation is 10 lines of code now that can be fixed by testing and debug, compared to the 50+ dodgy workaround lines on top of a basic system when one of your players thinks up a cool new spell. He's warding off what we all hate through better design, which is getting 6 months down the line and going "I really don't like this, I need a re-write". Space Station 13 is testament to that feature creep, naive design and wonky workaround code, before you even start to consider de-compilation on top.
Aug 10 2009, 11:57 am (Edited on Aug 17 2009, 9:15 am)
In response to Stephen001
 
obj/AoE_Spells
    var
        Duration
        Interval
        Damage
        Range
            
    New(Loc,mob/Caster)
        if(!Loc)    return
        src.loc=Loc
        spawn() while(Caster)
            for(var/mob/M in src.loc.contents-Caster)
                Caster.Damage(M,src.Damage)
            sleep(src.Interval)
        spawn(src.Duration) del src
    
    verb/Cast()
        set src in usr
        for(var/turf/T in oview(src.Range,usr))
            new src.type(T,usr)
    
    Fire_Storm
        icon='AoE.dmi'
        icon_state="FireStorm"
        Duration=50
        Interval=3
        Damage=10
        Range=3

//
mob/Login()
    src.loc=locate(5,5,1)
    src.contents+=new/obj/AoE_Spells/Fire_Storm

mob/proc/Damage(mob/M,Damage)
    return
Aug 10 2009, 12:06 pm
In response to Falacy
You really are obtuse.
Aug 10 2009, 12:11 pm
In response to Stephen001
Stephen001 wrote:
You really are obtuse.

Whatever floats your goats. I wrote a simple, functional, easy to use and read code that will actually do what this topic is requesting. Whats your problem with it?
Aug 10 2009, 12:13 pm
In response to Falacy
Aug 10 2009, 12:29 pm
In response to Stephen001
Stephen001 wrote:
[link]

Yes, its clearly worth it to waste time, space, readability, usability, and efficiency in exchange for unnecessary modularity.
Design for dynamic flexibility, then if you ever want to add spell specific features like his offers it will take no effort at all.
What would it take to add a GetTargets/AffectedBySpell procedure to mine? Replacing "src.loc.contents-Caster" with GetTargets(), then creating the GetTargets() proc with that line in it. Just about the same goes for everything else.
Page: 1 2