bug in targeting abilities

[pfps]

Here is a complex bug in game play.

In play:
Amok
Mentor of the Meek
Ballyrush Banneret


Activate Amok targeting Mentor of the Meek.
Cast Bind targeting the activation of Amok targeting Mentor of the Meek.
Activate Amok targeting Bellyrush Banneret.

Let the stack resolve.

The activation of Amok on Bellyrush Banneret resolves as expected.
The resolution of Bind does not counter the activation of Amok on Mentor of the Meek, but it does resolves and a card is drawn.
The activation of Amok on Bellyrush Banneret resolves, when it should have been countered.

I think that the problem stems from spell and ability targets being
SpellAbility, not SpellAbilityStackInstance, and when Amok's ability is
activated the second time it interferes with the connection between Bind and the other activation of Amok's ability.


The solution is probably going to involve changing spell and ability targets to SpellAbilityStackInstance. This is a major change, unfortunately. If SpellAbilityStackInstance can be modified to be a GameObject it might be possible to use SpellAbilityStackInstance when needed but coerce back to the underlying SpellAbility (with targets set up correctly) at other times so that less code needs to be changed.


This bug may also occur when a countered spell is copied, but I haven't checked this situation.

[pfps]

It appears to be possible to address this problem by making SpellAblility and SpellAbilityStackInstance both inherit from an abstract superclass. Most of the methods for SASI then defer to the attached SpellAbility. However, when it matters you can have a real SASI, permitting the correct distinguishing of stack entries.

I fiddled with the code to make this change, which is quite extensive, but not nearly as extensive as I feared.

[friarsol]

I fiddled with the code to make this change, which is quite extensive, but not nearly as extensive as I feared.

That's good to hear, I was a bit worried. People have a tendency to come in, take off too large of a task then get burnt out immediately.

[pfps]

The changes are still major.

SpellAbility.java and SpellAbilityStackInstance.java have dramatic changes. There is a new class, SpellAbilityPrototype, which is the old SpellAbility class. SpellAbility is now an abstract superclass of SpellAbilityStackInstance and SpellAbilityPrototype. SpellAbilityStackInstance has all the methods of SpellAbility, but most just defer to the attached SpellAbilityPrototype. The passing of information back to the SpellAbilityPrototype is still intact.

Most places that used SpellAbility can use the new SpellAbility. There are some exceptions that needed to be fixed, mostly places that use AbilitySub. (Most of these are in the AI code, and probably don't need to know that they have an AbilitySub.) Code that creates spell abilities has to determine what to create. Deep copying has not been totally fixed - it always creates a SpellAbilityPrototpye. The process of creating a spell/ability might be fixed up as well to move more information onto the SpellAbliityStackInstance from the beginning.

All this to allow spell/ability targeting to really grab the spell/target! There are only three real changes: 1/ Selecting a target from the stack really selects the spell/activity on the stack (at least for the human, I don't know where to fix the AI selection yet). 2/ Countering a spell/ability on the stack now works correctly. (There was a comment asking whether that worked right before - it didn't.) 3/ Triggers that target spells/activities also really select the spell/activity.

What's next? This is a big change. Should it just be dumped back on the trunk?

[friarsol]

What's next? This is a big change. Should it just be dumped back on the trunk?

Mind waiting till after we get at least a snapshot release out with FRF cards? That way people interested in playing with new cards will have something, and then everyone on the trunk can start playing around with these changes to make sure any debilitating issues are ironed out before the next full release.

There doesn't seem to be any other major changes going on right now, so hopefully that means there shouldn't be an issue of conflicts by delaying to commit a few days?

[elcnesh]

Good that you took this upon you! This part of the code has bugged me for a while as well. But maybe we can discuss a bit more?

For one, I'm not exactly sure why a StackInstance has to be a subclass of SpellAbility. It's basically an independent game object, that references a SpellAbility (ie, there can be multiple copies of the same activated ability on the stack simultaneously), but it's not SpellAbilities in the sense that it can't be activated the way a spell or ability can. In fact, I think StackInstance should be a subclass of GameObject (it's a targetable entity), while CardTraitBase and hence also SpellAbility need not, as they don't represent actual game objects, only abilities that appear on cards. I'm not sure if I'm right, I'm just saying what I think matches the Magic rules as closely as possible

[pfps]

Good that you took this upon you! This part of the code has bugged me for a while as well. But maybe we can discuss a bit more?

For one, I'm not exactly sure why a StackInstance has to be a subclass of SpellAbility. It's basically an independent game object, that references a SpellAbility (ie, there can be multiple copies of the same activated ability on the stack simultaneously), but it's not SpellAbilities in the sense that it can't be activated the way a spell or ability can. In fact, I think StackInstance should be a subclass of GameObject (it's a targetable entity), while CardTraitBase and hence also SpellAbility need not, as they don't represent actual game objects, only abilities that appear on cards. I'm not sure if I'm right, I'm just saying what I think matches the Magic rules as closely as possible

That's the other way of going. However, there are quite a few places where a SpellAbility was standing in for a StackInstance. Maybe it would be cleaner to proceed as you suggest, but there might also be more places that have to be changed.

This way of doing things would work as follows, I think. Targeting picks up the StackInstance, which is kept as a StackInstance as long as possible because you don't know that is going to be done with it. If it is used for countering then the StackInstance itself is needed. If it is used for copying then the SpellAbility is needed. You also may want to look at the SpellAbility at various times in between. Holding on to a SpellAbility for any period of time is problematic because they can be modified. If you don't have the StackInstance to set the modifiable values then you are stuck.

The advantage of making StackInstance a sub of SpellAbility is that you can just treat the StackInstance as a SpellAbility until you need to counter with it or copy it or recast it. This is immune to modifications to the underlying SpellAbility.

PS: I wonder what is supposed to happen if the underlying SpellAbility is really modified (e.g., via the color-word changing spell on cards) while a StackInstance is on the stack.

[elcnesh]

PS: I wonder what is supposed to happen if the underlying SpellAbility is really modified (e.g., via the color-word changing spell on cards) while a StackInstance is on the stack.

I think I fixed that a while ago by having the SpellAbility referenced by a StackInstance be a copy of the original SpellAbility

You're probably right that it can be fixed both ways, although my preference goes to doing it the "clean" way. Seeing how much code that'd change, it'd probably require a branch. I'm not sure if the traversable game state project has any relation to this? (Krazy?) I'd hate for it to be overwritten by that code soon, whatever approach we take here.

[pfps]

PS: I wonder what is supposed to happen if the underlying SpellAbility is really modified (e.g., via the color-word changing spell on cards) while a StackInstance is on the stack.

I think I fixed that a while ago by having the SpellAbility referenced by a StackInstance be a copy of the original SpellAbility

Is this actually what happens? I don't see this in the current code. As far as I can see, creating a StackInstance pulls some information off the SpellAbility, but does point back to the original SpellAbility.

[elcnesh]

Hmm... Then I probably intended to but never did it... Something to keep in mind when making these changes, would be nice to have that working as well.

[pfps]

Hmm... Then I probably intended to but never did it... Something to keep in mind when making these changes, would be nice to have that working as well.

The way to go then, I think, would be to make the copy of the SpellAbility (and its subabilities) when the StackInstance is created, essentially freezing the spell. Information would not be copied out of the SpellAbility back to the StackInstance (and back) because the SpellAbility copy would not be modified by futher castings of the original SpellAbility. This would require fixing the problem with XManaCost, I think.

This would be cleaner, I agree. It would be nice if there was some way to enforce (in Java) that the copied SpellAbility could not be changed. This would catch places where such SpellAbilibities are changed, like when making a copy with a new controller, which currently modifies a SpellAbility and then copies it.

It might also be possible to just copy and set a flag saying that this SpellAbility is a StackInstance. This really would require some support to ensure that inadvertent changes are not made to these SpellAbilities.

[pfps]

For one, I'm not exactly sure why a StackInstance has to be a subclass of SpellAbility. It's basically an independent game object, that references a SpellAbility (ie, there can be multiple copies of the same activated ability on the stack simultaneously), but it's not SpellAbilities in the sense that it can't be activated the way a spell or ability can. In fact, I think StackInstance should be a subclass of GameObject (it's a targetable entity), while CardTraitBase and hence also SpellAbility need not, as they don't represent actual game objects, only abilities that appear on cards. I'm not sure if I'm right, I'm just saying what I think matches the Magic rules as closely as possible

This looks very doable. I took at look at some of the code, and found a few wrinkles in the utility functions that return a mix of cards, players, and spell abilities. I think that these can all be overcome, because most of the uses of the results of these functions only look at players and cards.

When would be a good time to give this a try?