Trigger discussion (was WheneverKeyword reference)

[Hellfish]

Patch: http://pastebin.com/1gvXjiZZ
Based on r5000.

The clumsy beginnings of the DamageDone mode is in there as well. I already know that is hideous, please focus on the big picture

EDIT: Also I havn't gotten around to properly commenting the code.. Just ask if you have questions!

EDIT2: Couldn't sleep, so I added a bunch of modes and requirements/restrictions!
Patch:

patch.zip

(8.99 KiB) Downloaded 411 times

Based on r5032.

New modes: SpellCast,LifeGained,LifeLost,BeginningOfPhase,EndOfPhase,Attacks & Blocks.
New requirements/restrictions: Metalcraft,Threshold,PlayersPoisoned,ControlsNoValid & ControlsValid.
Didn't manage to fix the text bug, though, got distracted by MBS spoilers. (Incidentally, what's the common opinion on when we start to implement cards from upcoming sets?I'm asking because I have theoretically sound code for all spoiled cards except Flayer Husk & technically Signal Pest, using the new modes mostly.)

[Hellfish]

Thinking closer on it, maybe I should just commit the source-part of the triggersystem? That way it won't interfere with the cards currently in but you'd have an easier time getting at the code than with a patch. EDIT: Did this, looks fine.

[friarsol]

(Incidentally, what's the common opinion on when we start to implement cards from upcoming sets?I'm asking because I have theoretically sound code for all spoiled cards except Flayer Husk & technically Signal Pest, using the new modes mostly.)

I would prefer that Spoiled cards not happen until the spoilers were in magiccards.info since the Scripts require it. But I bet I'll be overruled.

[slapshot5]

(Incidentally, what's the common opinion on when we start to implement cards from upcoming sets?I'm asking because I have theoretically sound code for all spoiled cards except Flayer Husk & technically Signal Pest, using the new modes mostly.)

I would prefer that Spoiled cards not happen until the spoilers were in magiccards.info since the Scripts require it. But I bet I'll be overruled.

We're talking Mirrodin:Beseiged, right? I would say we shouldn't add cards at least until WotC officially releases the set.

-slapshot5

[Hellfish]

We're talking Mirrodin:Beseiged, right?

Yep. I think we were a bit early with either Scars or ROE but my memory is hazy or the decisionmaking may have changed. I'm cool with either way.

[friarsol]

Hellfish, it looks like the area of getAbilityText() you added your triggers in is for Instants and Sorceries only. I'm not sure why there is a need for two different sections in this code, but there seems that there is. So scroll down further in this function and add it down there as well. I think instead of this ugly line of code...

Code: Select all

sb.append(trig.getMapParams().get("TriggerDescription").replace("CARDNAME", getName()) + "\r\n");


you can just do

Code: Select all

sb.append(trigger.toString())


and let trigger have a toString that looks ugly. The CARDNAME to getName() is handled down below, so no need for you to do that here too.

With that change I was able to get the trigger text working properly.

[sentient6]

I may be a bit off base with this, and considering how new I am to the project there is absolutely no need to listen to what I say. But:

It seems to me that some of the logic in the TriggerHandler class might better be moved into the Trigger class, with subclasses of the Trigger class for each Trigger condition or something of the sort, mirroring the structuring of the AbilityFactory classes. Then the tests of validity could be handled in each subclass. Also, perhaps the spell abilities could be created in the trigger instances instead of being recreated each time the ability is triggered. Maybe these could be put in a subpackage of the forge package. If that makes sense to you, feel free to consider any or none of these ideas. Looking good by the way. That's my 1.5-2.1 cents...

[Hellfish]

Mirrroring the AbilityFactories was my first intention, but I sort of drifted from that as I worked more towards "Just work" rather than "work good, look nice". But hey, that's what first drafts are for

Oh and ideas are usually worth listening to, no matter how new the source is

EDIT: The description fix worked! Thanks, Sol!

[Sloth]

I just saw that you added RequireCounters as a parameter and want to point out, that I think that ControlsValid could have handled that with a restriction like: ControlsValid$ Card.Self+countersGE8LEVEL.

I would also propose to replace ControlsValid with isPresent (and thus considering all cards on the battlefield) and check the controller with YouCtrl or YouDontCtrl in the restriction string.

[Hellfish]

Thanks for the tip! I'll commit the replacement in just a little while.

[Hellfish]

Threw together a bit of a tutorial for triggers here but I'd like a second pair of eyes to go over it since I've been waistdeep in the code for about 2 days straight.

EDIT: Ugh, posted the wrong version and deleted the actual finished one A section on trigger restrictions is forthcoming.

[friarsol]

Some comments just on what I'm reading since I haven't really looked too much into your code.

Attacks>Unblocked/ValidBlocker. A creature isn't Unblocked until after blockers are declared. But your docs suggest this triggers before then, so does this work properly? Does that trigger happen later? Maybe just specify that there is one for Attacking and one for being blocked/not being blocked.

Phase: Are there any cards that trigger at the end of a phase? I thought all cards were errata'd so triggers happen at the beginning of phases.

Example: Your Example has Contagion Clasp's ability putting a counter on itself (oops) since it's missing the target parameter.

General thought: If more than one trigger happens during the same trigger segment, how is the ordering handled? I'm guessing right now it's just in whatever order it happens to go.

We should make note that all triggers that happen at the same time follow the APNAP rule. Since whoever controls the trigger gets to choose the order, active players triggers go on first, then non-active players does the same.

Triggers

Lastly, and this will probably come in after the initial deployment, is the "intervening if clause" but I have a feeling this needs more work on the AF and the Trigger side. (Since the Trigger checks the conditional before it hits the stack, and the Ability needs to check the conditional as it's resolving)

[Sloth]

Threw together a bit of a tutorial for triggers here but I'd like a second pair of eyes to go over it since I've been waistdeep in the code for about 2 days straight.

EDIT: Ugh, posted the wrong version and deleted the actual finished one A section on trigger restrictions is forthcoming.

Looking at the Restrictions, I propose to merge CardsIn and IsPresent. I think everything could be done by three parameters IsPresent$ (using isValid), PresentCompare$ (LE5, EQ1, etc, defaults to GE1) and PresentZone$ (defaults to Battlefield). That way all restrictions are taken care of and we only need one syntax (which happens to be the same as the syntax of the AF's, but for PresentZone).

[Hellfish]

Attacks>Unblocked/ValidBlocker. A creature isn't Unblocked until after blockers are declared. But your docs suggest this triggers before then, so does this work properly? Does that trigger happen later? Maybe just specify that there is one for Attacking and one for being blocked/not being blocked.

I'll be the first to admit the combat code slightly makes my head spin. I am looking at some candidates for places to put the runTrigger calls,tough.

Phase: Are there any cards that trigger at the end of a phase? I thought all cards were errata'd so triggers happen at the beginning of phases.

Huh, could've sworn there were older cards that triggered on ends of phases. Must be my Shandalar-itis acting up

Example: Your Example has Contagion Clasp's ability putting a counter on itself (oops) since it's missing the target parameter.

Misspelling in a big way

General thought: If more than one trigger happens during the same trigger segment, how is the ordering handled? I'm guessing right now it's just in whatever order it happens to go.

We should make note that all triggers that happen at the same time follow the APNAP rule. Since whoever controls the trigger gets to choose the order, active players triggers go on first, then non-active players does the same.

Triggers

Currently they go off in the order they were registered with the triggerhandler which depends on the order the cards are in in the Deck object passed to GameAction.newGame. I'll look into it right away.

Lastly, and this will probably come in after the initial deployment, is the "intervening if clause" but I have a feeling this needs more work on the AF and the Trigger side. (Since the Trigger checks the conditional before it hits the stack, and the Ability needs to check the conditional as it's resolving)

Yeah, currently AFs are completely separate from Triggers (in the sense of no overlapping code I mean..I think.)

Looking at the Restrictions, I propose to merge CardsIn and IsPresent. I think everything could be done by three parameters IsPresent$ (using isValid), PresentCompare$ (LE5, EQ1, etc, defaults to GE1) and PresentZone$ (defaults to Battlefield). That way all restrictions are taken care of and we only need one syntax (which happens to be the same as the syntax of the AF's, but for PresentZone).

Good call. Consider it done!

[friarsol]

I'll be the first to admit the combat code slightly makes my head spin. I am looking at some candidates for places to put the runTrigger calls,tough.

I think we can probably just reuse the functions that are already being called instead of having them in the DeclareAttackers/DeclareBlockers functions.

In CombatUtil, there is checkDeclareAttackers(), checkUnblockedAttackers(), checkDeclareBlockers(), checkBlockedAttackers() that all use checkWheneverKeyword().

Since the goal is to have Triggers ultimately replace the wheneverKeyword, I would call those prime candidates.