Pyrel dev log, part 5

[debo]

Overall I think the approach should be: Try to make any new class immutable first. Only make it mutable if immutability brings with it impractical issues. Note that I'm only considering behavioral immutability (ie: just treat it as if it were immutable) rather than full code immutability, since all the code to make a class truly immutable is a pain to add in. Only add that if it can be shown to work as an immutable class.

FWIW -- I like to think of things in terms of "value objects" and "entities". If something has an actual identity that persists throughout the program, it's an entity and it's probably ok to mutate it. If it's just describing something and equality is based on e.g. field equality, then it's a value object and should probably be immutable.

Aggregates and services are another story

(Stolen from Domain Driven Design)

[mtadd]

My contribution to last year's discussion of LOS / FOV was this link on FOV using recursive shadowcasting. The article describes the algorithm nicely, as well as provides and implementation in python at the end.

[Magnate]

EDIT: also, if you'd care to suggest algorithms for doing LOS, that's my next step...

A couple of years ago there was a LOS thread on here that got so long and so detailed that someone went off and started an entire wiki discussion somewhere to carry it on. There were a lot of good ideas and even better rebuttals so it's probably worth re-reading. Sorry I don't have time to find the link!

[Derakon]

I've implemented basic pathfinding for creatures now. This includes:

* A system to simplify the game map down to an array of numbers, making it easier and more efficient to analyze; this will also be useful for calculating line-of-sight and possibly for other things.
* An implementation of the A* search algorithm, and a few associated unit tests.
* Addition of the new "creature update" proc trigger condition, and an associated BasicAIProc on all monsters (by way of a new "opponent" creature template that all enemy creatures inherit from).

I'd appreciate a code review if anyone's interested in providing it. You can see the commits here, starting from eb09d52. Unfortunately I can't figure out how to get Bitbucket to display a diff of multiple commits without having a pullreq, and I can't make a pullreq since my repo is master. I should probably do something about that...

EDIT: also, if you'd care to suggest algorithms for doing LOS, that's my next step...

[Derakon]

Kinematics' increasingly-inaccurately-named "grammar" pullreq is now in! Thanks again for all your work, Kinematics! With any luck the next time it won't take me so long to review your changes.

[Kinematics]

Anyway, while writing up thoughts on immutability, I realized there were various things that weren't well-defined in how procs interact with other stuff.

Example: Cast Fireball at a mob that's surrounded by scrolls and stuff. Naturally the scrolls burn up. How does that happen? Is this explicitly built into the fireball effect? Or does the fireball do a stat check on every item it touches, checking for fire vulnerability (which in turn allows for proc checks per item)?

The latter seems the simplest way of going about it, but if it does that, what do the procs on each item's stat check look like? EG: If there's a "Protection from Fire" spell cast on one of the scrolls (thus a proc), what sorts of parameters are passed into it when it's called from a stat check via a fireball effect generated from a wand held by the player?

Is a 'user' passed in as part of the parameters? If a player aimed a wand and caused it, is the player the user? If a dragon breathed, is the dragon the user? If the dragon breathed on the player, and it's affecting scrolls in inventory, which one is the user? If triggered from a wand, is the wand anywhere in the parameter list as the effect gets handled?

Also, do the items destroy themselves (ie: remove their reference from the game map), does the fireball effect destroy them (explicitly manipulate the game map to remove the specified item), or is some message sent to the game world to remove that item from existence (so that the only manipulation of an item's existance happens within the game map class)?

In general we don't want to allow one object to directly manipulate another object. That gets back to the whole 'immutability' aspect; the more immutable an object, the less we have to worry about bugs from other stuff messing with it.

Anyway, just some issues that were rather vague at the moment, and impact what sorts of things the procs argument lists need to handle (and by extension, whether named tuples can be considered generally useful for them).

[Kinematics]

Ok, have written out a lot of thoughts on immutability, but in the end I don't think it's completely useful. Python is, by default, a mutable language, though it does have a few immutable structures (numbers, strings, tuples).

I think there are certain areas where it would be helpful to make aspects of the code immutable, but there are a lot of areas where it's not easy, and would probably add a substantial amount of unnecessary overhead.

Procs, for example, can probably be done as immutables. Everything about a proc is defined on construction, so it doesn't need to change anything after that. On the other hand, there's generally nothing -to- change after that, so calling it immutable is a little redundant. Also, since the triggerMaps is a dict, there's a hole in immutability as well if that's implemented.

One could, perhaps, create a named tuple to store the state of the basic parameters being passed in (eg: (item, user, gameMap, cancelled)) that would simplify the parameter list a bit. If each proc was required to return that tuple that could potentially make it easier to keep a consistent state as each proc was called. However since our list of procs is a bit sparse at this point, it seems premature to say that it can really handle all use cases.

Overall I think the approach should be: Try to make any new class immutable first. Only make it mutable if immutability brings with it impractical issues. Note that I'm only considering behavioral immutability (ie: just treat it as if it were immutable) rather than full code immutability, since all the code to make a class truly immutable is a pain to add in. Only add that if it can be shown to work as an immutable class.

[Derakon]

In alphabetical order, those will occur as: "cursed", "explode"

"cursed" finds that you're in a valid location to drop the grenade. "You let go of the cursed bauble."
"explode" determines that you're in the water, and won't let you drop the grenade. It cancels. "You're unable to release the grenade here."

Conflict of messages due to the fact that the 'cursed' proc was called before the 'explode' proc solely due to alphabetization. If the first proc was called "haunted" instead of "cursed", the above check would have gone to "explode" first, leading to a proper cancellation and no errant message, but only getting it right due to chance.

Right, so, the problem in this example is that the "cursed" proc shouldn't print anything until you actually do drop the item; the first pass should just say "yes, as far as I'm concerned this is a legit place to drop" and let things pass on silently.

I think we're both on the same page though.

[Kinematics]

Failure messages are printed by cancellation procs; success messages are printed by do-something procs. There should never be ambiguity or a failure of feedback under that constraint.

I was replying to you saying that the "you can't lift that!" message was a success message.

Could you illustrate this with a concrete example, please? I'm having trouble seeing the problem.

I should note that the original comment related to the proposal you seemed to be making (but weren't actually, or changed your mind), of removing the explicit distinction between prep and action calls, and handling it all via tiering. So the conflict largely doesn't actually exist, except in miscommunication.

However, to provide an explicit example:

You have a grenade.
The grenade has procs for: "cursed", "explode", that both handle the "onDrop" trigger condition.

"cursed" prevents you from dropping the grenade except on holy land.
"explode" explodes if you drop the grenade on normal ground. It prevents you from dropping it in water.

Since they both have a cancel condition, they're both moved up a tier above normal proc conditions. Still, they end up at the same level as each other.

You're standing in a consecrated lake.
You attempt to drop the grenade.

In alphabetical order, those will occur as: "cursed", "explode"

"cursed" finds that you're in a valid location to drop the grenade. "You let go of the cursed bauble."
"explode" determines that you're in the water, and won't let you drop the grenade. It cancels. "You're unable to release the grenade here."

Conflict of messages due to the fact that the 'cursed' proc was called before the 'explode' proc solely due to alphabetization. If the first proc was called "haunted" instead of "cursed", the above check would have gone to "explode" first, leading to a proper cancellation and no errant message, but only getting it right due to chance.

[Derakon]

Except it's not a success message, it's a failure message. You didn't succeed at the conditional, so you never even got to the "do something" function. However you still need to provide feedback to the player so that they know that something was at least tried.

Failure messages are printed by cancellation procs; success messages are printed by do-something procs. There should never be ambiguity or a failure of feedback under that constraint.

Just because the race condition has a predictable outcome based on the current state doesn't mean it's not still a race condition. The issue is that the behavior can be 'unexpected' from the player point of view (even if reliably repeatable) simply due to the names chosen for the procs, not due to the code each executes.

Could you illustrate this with a concrete example, please? I'm having trouble seeing the problem.

[Kinematics]

Thus my advice would be simply "don't change state in cancellation Procs; always call all cancellation Procs before any state-changing Procs".

I was trying to adapt my idea from separate phases to a single phase based on what you seemed to be saying, but it looks like we're back at separate phases (which I agree is a much easier way of going about things).

But I could easily see e.g. a Proc that just does "perform a skill check, cancel with this message if failed" that would be attached to many actual "do X" Procs. That's part of why I prefer the tiering approach: we can separate out the conditional Procs from the actually-do-something Procs, and mix and match them as we like.

Just as a note: separating the 'cancel' procs from the 'doing' procs is sort of an inherent tiering, but it's a logical one that's pretty much required for predictable behavior. In the abstract sense you could consider each category a tier, but you don't need to create tier values for that, just an understanding that there will always be those two phases for just about any trigger -- all cancelables get called, then all actionables get called.

Surely a success message should be printed by the "do something" Proc, not the conditional Proc?

Except it's not a success message, it's a failure message. You didn't succeed at the conditional, so you never even got to the "do something" function. However you still need to provide feedback to the player so that they know that something was at least tried.

As for if two Procs would both cancel, the fact that there's a race condition doesn't bother me. The Procs will be sorted, so there's a consistent invocation order; the fact that it's arbitrary is a non-issue.

Just because the race condition has a predictable outcome based on the current state doesn't mean it's not still a race condition. The issue is that the behavior can be 'unexpected' from the player point of view (even if reliably repeatable) simply due to the names chosen for the procs, not due to the code each executes.

When I started writing Pyrel, a couple of people vaguely agitated for making the game state immutable (i.e. any permutations would be done by creating a new state). We didn't really take the time to hash out exactly how this would work, so I wasn't convinced, and didn't do it. This is one area where I could see it being handy. Unfortunately at this point I'm not really certain it'd be easy to retrofit the system.

Immutability is a wonderful thing, but hard to do right, and not always what we want.

~ Getting into a long tangent on immutability. Will save that for another post.

[Derakon]

I can see it such that each proc essentially encapsulates the "preparing" plus "doing" phases within itself. For example, the tooHeavyObject.trigger() first checks if you can even pick the item up, and if so bails out, giving a message and seeing proc.shouldCancel to True. Sending a message to the UI is safe(?) at that point, since any other 'preparing' phase procs will have either passed, or won't be run.

The issue I have with this is that we'd be coupling "can I do X" with "do X", with the assumption that we will never want the two to be separate. But I could easily see e.g. a Proc that just does "perform a skill check, cancel with this message if failed" that would be attached to many actual "do X" Procs. That's part of why I prefer the tiering approach: we can separate out the conditional Procs from the actually-do-something Procs, and mix and match them as we like.

However it leaves open a potential issue: Suppose one of the procs sends a UI message when it -succeeds-? If that gets run before the one that cancels, you'll get the succeed message followed by the cancel message. Certainly you can avoid this by using the tiering method, putting the canceling action at a tier that goes before the others, but then what if the other proc needs that as well, since it has its own cancel message? Then you're in a race condition for which one gets executed first.

Surely a success message should be printed by the "do something" Proc, not the conditional Proc?

As for if two Procs would both cancel, the fact that there's a race condition doesn't bother me. The Procs will be sorted, so there's a consistent invocation order; the fact that it's arbitrary is a non-issue.

And while the above example is for displaying a message, it applies for any event that somehow modifies the game state. You need to be sure that any modifications that were done prior to the cancelation are thrown out.

That gets tricky unless you don't allow modification of any game state aside from a variable that can be passed from one proc to the next that will be used to do the actual state modification. Then if any of the procs cancel, you can just toss that variable out and not worry about it.

When I started writing Pyrel, a couple of people vaguely agitated for making the game state immutable (i.e. any permutations would be done by creating a new state). We didn't really take the time to hash out exactly how this would work, so I wasn't convinced, and didn't do it. This is one area where I could see it being handy. Unfortunately at this point I'm not really certain it'd be easy to retrofit the system.

Thus my advice would be simply "don't change state in cancellation Procs; always call all cancellation Procs before any state-changing Procs".

[Kinematics]

Messaging should definitely be up to the Proc involved. Sometimes it will make sense to provide a message when a Proc chain must be cancelled; sometimes it won't.

Yeah, I was looking it over and realized it would be a lot easier that way. If it was done in the manner with my cancelArgs, then each one that was doing a prepare check could check if cancel was true, and if so just immediately bail out since any additional attempt to cancel would be redundant. That leaves the first one that cancels free to provide a message without worrying about spamming extra messages further down the line.

I really think that we're better off having tiered Procs that are invoked in tier order, rather than trying to come up with separate trigger conditions for preparing, executing, and cleaning up afterwards. So we have e.g. a default tier of 1, and if you want to go beforehand then you set the tier to 0, and if you want to go afterwards, then you set it to 2. Floating point of course so you can insert whatever tiers you need.

Hmm. Will that actually work?

I can see it such that each proc essentially encapsulates the "preparing" plus "doing" phases within itself. For example, the tooHeavyObject.trigger() first checks if you can even pick the item up, and if so bails out, giving a message and seeing proc.shouldCancel to True. Sending a message to the UI is safe(?) at that point, since any other 'preparing' phase procs will have either passed, or won't be run.

However it leaves open a potential issue: Suppose one of the procs sends a UI message when it -succeeds-? If that gets run before the one that cancels, you'll get the succeed message followed by the cancel message. Certainly you can avoid this by using the tiering method, putting the canceling action at a tier that goes before the others, but then what if the other proc needs that as well, since it has its own cancel message? Then you're in a race condition for which one gets executed first.

And while the above example is for displaying a message, it applies for any event that somehow modifies the game state. You need to be sure that any modifications that were done prior to the cancelation are thrown out.

That gets tricky unless you don't allow modification of any game state aside from a variable that can be passed from one proc to the next that will be used to do the actual state modification. Then if any of the procs cancel, you can just toss that variable out and not worry about it.

So, either you only allow 'state' modification of a provided variable that's easily discardable, or you need the multi-stage pipeline for preparing/doing to make sure nothing changes the global state until all of the procs have agreed it's ok.

The modify-the-Proc approach strikes me as rather hackish, but it bothers me to force every Proc to accept an extra argument (the cancelArgs thing) when the vast majority of them will not care about it. Hm.

Not all of them have to handle the extra argument; only the ones that care about cancelling an action you're about to do. 'Standard' proc triggers would still just get the normal [self.triggerProcs(user, gameMap, 'onPickup')]-type function call (which calls the function that loops through all the procs, calling each one with (item, user, gameMap)).

Of course if you use the non-pipelined approach then you don't have the easy separation there. However in that case you'll be required to provide some sort of state-holding var, regardless, so it might as well include its own cancel condition.

data = {"cancel": False, "someGameData": "stuff", etc}
proc.trigger(self, user, gameMap, data)

def trigger(self, sender, user, gameMap, data, *args, **kwargs):
    pass

However that requires setting up an agree-upon dict mapping for stuff that the trigger might need, and many won't really need anything else at all, aside from the basics already provided in sender/user/map.

The main trickiness is that those initial basics themselves already allow changing of game state without discardability. For example, onUse for a potion item may decrement the count on the sender object, completely bypassing the safety net of the data arg. You need to be sure that that can't happen if there's still any possibility of a cancellation.

[Derakon]

I really think that we're better off having tiered Procs that are invoked in tier order, rather than trying to come up with separate trigger conditions for preparing, executing, and cleaning up afterwards. So we have e.g. a default tier of 1, and if you want to go beforehand then you set the tier to 0, and if you want to go afterwards, then you set it to 2. Floating point of course so you can insert whatever tiers you need.

To handle canceling the invocation chain, you could have an extra argument to indicate whether cancellation happens. I don't think having an extra return value for the Proc is a great idea since Proc return values are already used for other things, and I'd rather they be consistent. A third possibility is to set a property on the Proc itself, which the caller would check after invoking the Proc:

def invokeProcs(procs, *args):
    for proc in sorted(procs, key = lambda p: p.tier):
        proc.shouldCancel = False
        proc.trigger(*args)
        if proc.shouldCancel:
            break

The modify-the-Proc approach strikes me as rather hackish, but it bothers me to force every Proc to accept an extra argument (the cancelArgs thing) when the vast majority of them will not care about it. Hm.

Messaging should definitely be up to the Proc involved. Sometimes it will make sense to provide a message when a Proc chain must be cancelled; sometimes it won't.

[Kinematics]

Preparing/Doing

Current:

    ## Respond to the item being picked up.
    def onPickup(self, user, gameMap):
        self.triggerProcs(user, gameMap, 'onPickup')

Revised:

    ## Respond to the item being picked up.
    def onPickup(self, user, gameMap):
        cancelArgs = cancelEventArgs()
        self.triggerProcs(user, gameMap, 'prepareToPickup', cancelArgs)
        if cancelArgs.cancel:
            return
        self.triggerProcs(user, gameMap, 'onPickup')


# Create a class for this since it's going
# to be pretty commonly used
class cancelEventArgs():
    __init__():
        self.cancel = False

Need consistent naming in the trigger conditions. Is "preparingToAction"/"prepareToAction" and "onAction" workable? Or "onActioning" vs "onAction" (eg: "onPickingUp" vs "onPickup"/"onPickUp")? I'm inclined more towards the former, since it feels less awkward for a lot of the verbs we'll be using. "prepareToFire" vs "onFiring", before "onFire".

Should there be space for a message if an action is cancelled? For example, if you try to pick something up and it's too heavy for you to lift, something like:

class cancelEventArgs():
    __init__():
        self.cancel = False
        self.message = []


class someHeavyItemProc():
    def prepareToPickup(self, item, user, gameMap, cancel, *args, **kwargs):
        if user.Strength < 18:
            cancel.cancel = True
            cancel.message.append("%s is too heavy for you to lift!" % self.name)

class item():
    def onPickup(self, user, gameMap):
        cancelArgs = cancelEventArgs()
        self.triggerProcs(user, gameMap, 'prepareToPickup', cancelArgs)
        if cancelArgs.cancel:
            if cancelArgs.message:
                gui.message(cancel.message)
            return
        self.triggerProcs(user, gameMap, 'onPickup')