Trigger code question (re: suppression)

[Sloth]

Also, I just tested with Blood Moon and City of Brass. With both in play, I tapped City of Brass and the trigger *did* fire.

-slapshot5

I did some more testing with the setup game state command:
- If I start with City of Brass on the Battlefield and play Blood Moon, the trigger is suppressed correctly.
- If I start with City of Brass in hand and play it and Blood Moon, the trigger is suppressed correctly.
- If I draw City of Brass from the library and play it and Blood Moon, the trigger is not suppressed correctly.

That means (once again) that changing zones (other than Battlefield) messes up triggers.

[Sloth]

OK, I found the problem:

In this part of changeZone the triggers are set to a new host:

Code: Select all

        for (Trigger trigger : c.getTriggers()) {
            trigger.setHostCard(copied);
        }


The problem is that the triggers are not added to the trigger list of the copied card and the getTriggers() function in card will not return them.

Several quick fixes come to mind, but I would like to know if the whole double storing (trigger->hostCard, card->hostedTriggers) can be avoided?

The getTrigger function can just look like this:

| Open

Code: Select all

public final ArrayList<Trigger> getTriggers() {
       
        ArrayList<Trigger> trigs = new ArrayList<Trigger>();
       
        for(Trigger trig : AllZone.getTriggerHandler().getRegisteredTriggers()) {
            if(trig.getHostCard().equals(this)) {
                trigs.add(trig);
            }
        }
           
        return trigs;
    }

and the whole ArrayList<Trigger> can be removed from the card class.

[Or we could store all triggers in the card class and get rid of the central storing in RegisteredTriggers. But that would be more work.]

[Hellfish]

The reason they are stored in both the Card class and the TriggerHandler class is an optimization at heart. (Not a heap space optimization,obviously, but a computation one) The cards register their triggers at the start of a match and whenever they gain one and when they do, TriggerHandler sort of makes a note that the mode of that trigger is actually used. When a particular trigger mode is run, this list is checked first and only it finds the mode there it loops through all the triggers and checks their conditions.

It could be altered to go through all cards in the game and check for triggers quite easily. (At a speed penalty, however slight, I don't know)

EDIT: Forgot to say, I'm all on board with any rewrite that would help make the system more usable.

[Sloth]

The reason they are stored in both the Card class and the TriggerHandler class is an optimization at heart. (Not a heap space optimization,obviously, but a computation one) The cards register their triggers at the start of a match and whenever they gain one and when they do, TriggerHandler sort of makes a note that the mode of that trigger is actually used. When a particular trigger mode is run, this list is checked first and only it finds the mode there it loops through all the triggers and checks their conditions.

It could be altered to go through all cards in the game and check for triggers quite easily. (At a speed penalty, however slight, I don't know)

EDIT: Forgot to say, I'm all on board with any rewrite that would help make the system more usable.

I guess the easiest way to change this, is removing the stored triggers from the card class. I will tinker around with it and if no problems arise I will submit it. I hope you can help me test and evalue it then.

[Sloth]

Well, I found a simpler solution to fix this problem. I still wish the trigger handling could be simpler, but I won't touch this any time soon (it's really not an easy task).

Can you test Mystic Compass again Slapshot?

[slapshot5]

Can you test Mystic Compass again Slapshot?

Yeah, this works now. I'll test further and submit later tonight.

-slapshot5

[Sloth]

I've also submitted a fix for the Graft keyword. I replaced the use of overridingAbility with the "normal" use of an SVar and adding an Execute parameter to the trigger.

If someone can do another test, I would feel better (I don't trust the triggers anymore).

[Hellfish]

That does make sense. The anonymous SpellAbility class created before uses a specific Card object that is snapshotted at the time the class is declared and is probably a different one than the one that finally ends up in the game. Execute SVars on the other hand are only parsed/created when they're about to be run. Thanks for solving that mystery! Hopefully with some rewrites little Triggy will regain your trust

Sadly, this means that triggers with overriding spellabilities just became somewhat useless..

[slapshot5]

I've also submitted a fix for the Graft keyword. I replaced the use of overridingAbility with the "normal" use of an SVar and adding an Execute parameter to the trigger.

If someone can do another test, I would feel better (I don't trust the triggers anymore).

Yep, it looks like Graft works again in the current SVN. Do you want to resolve the bug, or should I?

http://cardforge.org/bugz/view.php?id=231

-slapshot5