WagicProject
Some Triggered abilities that belong to player 2 offer the action to player 1
From [email protected] on December 05, 2010 10:33:32
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: http://code.google.com/p/wagic/issues/detail?id=548
From [email protected] on December 05, 2010 07:59:18
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.
From [email protected] on December 05, 2010 08:08:05
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.
From [email protected] on December 05, 2010 08:22:49
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();
}
}
}
}
From [email protected] on December 05, 2010 13:47:54
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 :)
From [email protected] on December 05, 2010 17:12:48
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 ?
From [email protected] on December 05, 2010 19:01:25
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.
From [email protected] on December 05, 2010 20:11:24
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 :)
From [email protected] on December 07, 2010 10:09:50
please try my latest commit and see if this issue is resolved.
From [email protected] on December 08, 2010 10:20:25
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?
From [email protected] on December 08, 2010 15:52:19
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.
From [email protected] on March 21, 2011 19:43:24
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.
From [email protected] on April 17, 2011 12:13:40
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.
From [email protected] on April 28, 2011 03:11:24
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.
From [email protected] on July 21, 2011 15:44:57
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.