Some Triggered abilities that belong to player 2 offer the action to player 1

[GoogleCodeExporter]

I've seen several times in a Random AI VS AI, random 2 colors game, when a 
triggered ability triggers, and the AI seems to "freeze" while choosing a 
target.

After looking at the code, it seems that the problem is that these triggered 
abilities ask player 1 to choose a target, when it should be player 2.

Put in another way, the "GameObserver->currentlyActing" player is incorrectly 
set at the time these abilities trigger.

Typical scenario:
- Player 2 attacks
- As the player 1 "choose blockers" phase begins, a "attackers" trigger from 
player 2 triggers. Unfortunately, the "currentlyActing" player at this stage is 
(incorrectly) player 1, which is offered to "do something" with the trigger. 
Unfortunately, this fails a security check in 
ActivatedAbility::isReactingToClick(MTGCardInstance * card, ManaCost * mana), 
that makes sure the currentlyActing player is the same as the one that controls 
the source of the ability.

Suggested solution: a "hack" would involve changing the currently acting 
player. I believe some MTGAbility objects already do that but this needs to be 
confirmed.

Original issue reported on code.google.com by wagic.the.homebrew@gmail.com on 5 Dec 2010 at 9:33

[GoogleCodeExporter]

Granted I haven't spent that much time in this, but I think the root problem is 
the flip of 'currentPlayer' in GameObserver::Update() for the blocker phase.  
This is a gaping logic hole to fall through - anybody checking the observer for 
currentPlayer on a trigger effect will get the wrong info if they don't realize 
that it could be reversed 'cause we're on the block phase. I remember noticing 
& not liking this before when previously walking through the logic changes 
needed for trigger resolves during the combat phases with Z. 

Currently, when I look at GameObserver, I see this:

  Player * currentPlayer;
  Player * currentActionPlayer;
  Player * isInterrupting;
  Player * opponent();
  Player * currentlyActing();

And it's all public.  My suggestion: make the Player* protected/private, force 
all clients to use currentlyActing() if that's what they need, or else another 
GetPlayerWhoseTurnItActuallyIs() or something better named, but you get my 
drift.  I think it'll take a large time to go through all the places where the 
variable is accessed, but ultimately you'll have a cleaner meaning in all the 
client code where it's being utilized. Nobody on the client side should be 
stashing player pointers, they should always pull using these functions.  
That's probably where this bug is coming from - someone makes a local copy of 
what they think the current player is, and it gets out of sync at some point 
with the game phases. 

Original comment by wrenc...@gmail.com on 5 Dec 2010 at 3:59

[GoogleCodeExporter]

haha moot, both you and i suggested the SAME exact thing! :)
i was just replying to this in the "changes" here., ill post here to keep our 
general ideas about this pooled together.

"
the root cause is EXACTLY what you suspect, the method were using to get the 
currentplayer is just too "loose" we need a way to get the actual 
abilities/triggers/ects OWNER->opponent to return as the interruptee...

possibly in the base classes, have something like
"owner->opponent->offerinterrupt" so the abilities themselves handle it instead 
of it being handled in the actionlayer.

i suggest owners->opponent gets the interrupt because in MTG you are NEVER 
allowed to interrupt yourself in priority passing. its always
you
opponent->interrupt
you
opponent->interrupt

doesnt matter how many stack actions their are in the current stack.
it should ALWAYS be switching back and fourth.

look in gameobserver.cpp update(float dt) line 339, this is when the 
"currentPlayer" is overiden  for the blocker step.

"

this is exactly what i posted, of course you did a better job explaining it 
then me, we both basically said the same exact thing.

Original comment by omegabla...@gmail.com on 5 Dec 2010 at 4:08

[GoogleCodeExporter]

look here for exsample...this is in the base class of activated abilities....

int ActivatedAbility::reactToClick(MTGCardInstance * card)
{
    //  if (cost) cost->setExtraCostsAction(this, card);
    if (!isReactingToClick(card))
        return 0;
    Player * player = game->currentlyActing();
<----- see this.....this could cause all sort of probelms because 
currentlyacting is flipflopped alot during combat.

currentlyacting is the turns owner in attackers, then currentlyacting is the 
opponent in blockers, but to add to this mess, currently acting also flipflops 
when an ability is used, to allow for targetting with targetchooser when a 
player decides to interupt....

here

 if (isInterrupting)
        player = isInterrupting;
    mLayers->Update(dt, player);

and here

Player * GameObserver::currentlyActing()
{
    if (isInterrupting)
        return isInterrupting;
    return currentActionPlayer;
}

and isInterupting is decided in a very shady way....here

    if (mode == ACTIONSTACK_STANDARD)
    {
        modal = 0;
        if (getLatest(NOT_RESOLVED))
        {
            int currentPlayerId = 0;
            int otherPlayerId = 1;
            if (game->currentlyActing() != game->players[0])
            {
                currentPlayerId = 1;
                otherPlayerId = 0;
            }
            if (interruptDecision[currentPlayerId] == NOT_DECIDED)
            {
                askIfWishesToInterrupt = game->players[currentPlayerId];
                game->isInterrupting = game->players[currentPlayerId];
                modal = 1;
            }
            else if (interruptDecision[currentPlayerId] == INTERRUPT)
            {
                game->isInterrupting = game->players[currentPlayerId];

            }
            else
            {
                if (interruptDecision[otherPlayerId] == NOT_DECIDED)
                {
                    askIfWishesToInterrupt = game->players[otherPlayerId];
                    game->isInterrupting = game->players[otherPlayerId];
                    modal = 1;
                }
                else if (interruptDecision[otherPlayerId] == INTERRUPT)
                {
                    game->isInterrupting = game->players[otherPlayerId];
                }
                else
                {
                    resolve();
                }
            }
        }
    }

Original comment by omegabla...@gmail.com on 5 Dec 2010 at 4:22

[GoogleCodeExporter]

hold off on killing yourself to solve this, i think i have it fixed, its my 
fault, trigger step was passing priority with the false "combat damage" you 
were seeing before, then youre offered the interupt on it and on the real one. 
i corrected the issues where it was still go go go go go. now when things 
triggers, youre picking a target, or looking at a menu, the call for next step 
holds off. 

i must add, the function "gamestates()" is a beautiful little function. we need 
to make more use of this, i know you mention in the source a few places that "X 
and Y should be in gamestates()" now i understand why :)

Original comment by omegabla...@gmail.com on 5 Dec 2010 at 9:47

[GoogleCodeExporter]

by gamestates i meant "stateEffects()" sorry!

Original comment by omegabla...@gmail.com on 5 Dec 2010 at 9:49

[GoogleCodeExporter]

Ah, thanks, I'll have a look at your fix then :)
And I agree with both of you.
the "currentPlayer" was initially a simple variable made public, then I 
realized I needed something more complex to make a difference betwen the player 
whose turn it is, versus the player who currently controls (hence the 
"currentlyActing"). Because all of this was public and the functions are not 
properly named, this quickly became a huge mess.
Then the stack, interrupts, triggers, etc... came in, and it became an even 
bigger mess.

What should we do in terms of functions?
We need to be able to know
- who's turn it is
- who's currently playing
- who gets to play after the next action...

Should we have a stack of player pointers?
like
it's player A's turn
add player B to the stack when choose blockers starts
remove player B from the stack when blockers are chosen
add player X whenever player X's abilities trigger

Would that solve this whole mess ?

Original comment by wagic.the.homebrew@gmail.com on 6 Dec 2010 at 1:12

[GoogleCodeExporter]

my fix corrects the issue only during combat if you are seeing this issue 
outside of combat, then im afaid my fix wont do anything to help it. 

Original comment by omegabla...@gmail.com on 6 Dec 2010 at 3:01

[GoogleCodeExporter]

So far I'm pretty sure I saw this mostly in combat, when creatures would go to 
the graveyard for example. Obviously your fix will at least mitigate a good 
part of the problem :)

Original comment by wagic.the.homebrew@gmail.com on 6 Dec 2010 at 4:11

[GoogleCodeExporter]

please try my latest commit and see if this issue is resolved.

Original comment by omegabla...@gmail.com on 7 Dec 2010 at 6:09

[GoogleCodeExporter]

im having a heck of a time trying to find where exactly the controller of the 
targetchooser is set...i mean very very hard time...no sign of targetchooser 
control switching in any class containing "target" in its name, this makes me a 
little dizzy..where exactly is wagic determining the controller of the 
targetter?

Original comment by omegabla...@gmail.com on 8 Dec 2010 at 6:20

[GoogleCodeExporter]

Usually wagic just watches for "getCurrentlyActing" and cards.
The owner of an ability is in most cases the controller of the card that 
triggered it.
Therefore to say if you are allowed to "reply" to a triggered ability,, Wagic 
compare "getcurrentlyActing" and the controller of the ability's source.

I agree this is far from perfect.

Original comment by wagic.the.homebrew@gmail.com on 8 Dec 2010 at 11:52

[GoogleCodeExporter]

it might be fixable to have abilities and triggers contain a 
"abilityController" variable to compare it to, instead. this way on action 
stack resolves we can set the targetchoosers and menu items to the 
"abilityController" instead of comparing the global variables. so when ie: 
ability 3 on the stack resolves, it sets the currently acting to whatever that 
abilities "abilityController" is...i am currently trying to correct a 100% 
repro of this right now. 

auto=@damaged(this):damage:thatmuch target(creature,player)

use this exact coding on a card, play evil twin against ai...lightining bolt 
the creature which AI controls with the trigger on your turn. YOU will be 
offered the targetchooser instead of ai.

Original comment by omegabla...@gmail.com on 22 Mar 2011 at 2:43

[GoogleCodeExporter]

repro is easy...its "may" abilities when it is added to the game in a phase 
such as blockers, when you have priority during ai turn, if ai controller the 
card with the "may" the menu and targetting are offered to you instead.

Original comment by omegabla...@gmail.com on 17 Apr 2011 at 7:13

[GoogleCodeExporter]

i think i've tracked down the cause of this bug, tho a fix will be very hard.
may abilities force "isInterrupting = source->controller" for the purpose of 
making a menu selection, however TC does not care about who the interrupting 
player is...it only cares about the activePlayer. in blockers, the active 
player is the opponent.
so in one corner you have may ability saying that i should choose an action, 
while targetchoosers say the targetting should be done by the activeplayer. 
there is a hole somewhere where we are not checking that the 
game->isInterrupting play == game->currentlyActive...in this hole is where this 
bug appears.

Original comment by omegabla...@gmail.com on 28 Apr 2011 at 10:11

[GoogleCodeExporter]

void GameObserver::Update(float dt)
{
    Player * player = currentPlayer;
    if (Constants::MTG_PHASE_COMBATBLOCKERS == currentGamePhase && BLOCKERS == combatStep) 
        player = player->opponent();

this also contributes to this bug...if an actionelement has a cardwaiting , 
this update messes up the entire target selection and menu selection process.

Original comment by omegabla...@gmail.com on 21 Jul 2011 at 10:44