BOK Offerings

[moomarc]

I'm almost done with the Offering ability, just a few final bits to implement that I need help with:

1 - How do I cancel an ability/cost payment from within ManaCostBeingPaid.java? I need to do this so that if you decide against making an offering the whole cost payment is cancelled rather than just the sacrifice input. Or should I just make the sac input mandatory seeing as for it to have allowed you to Offering-cast it you have to have a valid offering? Only reason I don't want to make it mandatory is that you might click on the Patron accidentally and want to cancel out, but if the sacrificed creature satisfies the full cost you never get to cancel out of the ability (not an issue though if there's mana left to be paid).

2 - I can't figure out how to get the ai payment working! Probably need to rework the whole ruddy thing. I'll post a patch at some point when I have the other issue sorted.

[friarsol]

Since Offering is an alternate cost, if you can cast a sorcery you should be prompted if you want to cast it normally, or with an Offering. So if you choose the Offering fork, the sacrifice is mandatory. If you click on the Patron accidentally and want to cancel out, you have the opportunity to do that before clicking on a creature to sacrifice.

[moomarc]

Since Offering is an alternate cost, if you can cast a sorcery you should be prompted if you want to cast it normally, or with an Offering. So if you choose the Offering fork, the sacrifice is mandatory. If you click on the Patron accidentally and want to cancel out, you have the opportunity to do that before clicking on a creature to sacrifice.

Currently what happens is:
- GameActionUtil:getAlternateCosts checks for keyword ending with "offering".
   - If you have a card in play of the correct subtype, an alternate SA is setup with the InstantSpeed spell ability restriction.
   - The alternate SA is tagged isOffering
- If not a time you could cast the creature normally and you click on the creature in your hand, cost payment begins, otherwise you choose the mode first.
Assuming cast via offering:
- When the SA is passed through ManaCostBeingPaid:applySpellCostChange, and at about ln556 (just after static ability RaiseCost is applied) the SA is checked. If SA isSpell and isOffering > adjustCostByOffering
- adjustCostByOffering method:
   - uses the existing choosePermanentsToSacrifice input. (Before this input there is no chance to cancel besides mode selection if not selected at instant speed.)
   - the cost is adjusted based on the selected card's mana cost
   - mark card to sacrifice setUsedToPay(true) (which now also means that its abilities can't be activated etc.)
   - sets the card to sac setSacrificedAsOffering on the SA.
- finish mana payment if any (can cancel here for first time since mode choice but only if there is mana left to be paid)
- card sacrificed with triggers unsuppressed.

[moomarc]

I think I'm just going to make the input mandatory to make life easier. I'll post a patch soon, but the only issues I'm left with from testing are these:

1 - if the offering satisfies the full cost then InputPayMana.handleOfferings is never called and that's where the sacrifice with unsuppressed triggers happens. I tested and this is also the case for convoke where handleConvokedCards is never reached when the full cost is convoked - it just isn't visible because the cards are tapped anyway. To test, just convoke a Conclave's Blessing into play paying the full cost by tapping creatures where at least one of them is enchanted with Brink of Disaster. The done() override in InputPayManaSimple is skipped when there is no mana left to pay after cost adjustments it seems so these final calls for convoke and offerings aren't made. If someone can fix the Convoke issue, it should fix this offering issue too.

2 - AI: I have no idea what path the AI takes or where I'd need to even put in canPlayAi(false) to stop the AI from trying. (at least I finally know most of the route for a human activation now)

EDIT: It looks like the root cause of issue 1 is HumanPlay.payManaCostIfNeeded doesn't go through the input those keywords rely on if the ManaCost isPaid. I think I can fix it neatly... will let you know.
EDIT2: Nope. I was either wrong or implemented the fix poorly.

[moomarc]

Okay, I've got the first issue sorted out now so just sorting out the AI...

[moomarc]

All issues fixed. Committed r21857. Some things can probably be cleaned up or moved to more appropriate places, but I did the best I could.

What's done:
- offering implemented as an alternate cost
- functional for both human and AI (although the scripts for the patrons are tagged with RemAIDeck until someone more experienced can check if the AI is actually fine using it)
- Added all the Patrons except Patron of the Nezumi (mostly because I ran out of time).
- Sacrifice can be cancelled and now acts like cancelling any other payment (modified the choosePermanentsToSacrifice input with a canCancel boolean)

Tests performed:
- correct order in cost modification
- Cancel ability before selecting card to sacrifice
- cancel payment during remaining mana cost payment
- sacrificing a creature that fully satisfies the mana cost (eg: Goblin Dynamo for Patron of the Akki). Triggers go off when they should and applied a similar catch for convoke.
- sacrificing a creature with mixed mana symbols
- not being able to use a mana ability of the creature to be sacrificed while paying remaining mana
- tested with creatures with changeling
- basic AI testing

[Max mtg]

There is problem with current implementation:
* Ok and cancel buttons are handled by the same thread that presents UI, that is EDT. This thread must not be suspended under any circumstances. Note that 'done' and 'onCancel' methods both call handleOfferings(boolean), that in turn calls game.getAction().sacrifice(offering, saPaidFor). This method (as any method involving zone changes) must not be called from EDT, because any static ability reading something like "As a creature dies, reveal (anything) from your hand" or "As ... pay (any cost)" - that waits for user input would immetiatelly hang the game.

You need to sacrifice creature in game thread, that is possibly in CostPayMana or the code that calls it (because strictly speaking sacrificing a creature is not related to mana at all)

[moomarc]

Thanks for the feedback, Max. I was a bit worried that I might do something like that because when it comes to threading I'm clueless. But I'll have a look in the morning to see where I might be able to put it and hopefully I can get it right.

Is there, by any chance, some way that I can easily see whether a method is being called from EDT? (Beginner level explanation if possible).

[moomarc]

Is it just those handleOffering calls in inputPayManaCostSimple and inputPayManaCost that need to move due to the sacrifice? If so can they perhaps just be removed then let the catch already in costPayMana handle it? I put those lines in to catch it if the input was skipped because the cost was reduced to zero by the offering, but I guess there's no real need for both. I just tried to follow the general flow of convoke.

[Max mtg]

How to detect which thread actually calls a method:
1. Analytical approach - see callers. Whatever comes from handles of user actions is called in EDT. Whatever comes from phase changes, stack resolve, effects... is called in Game thread.
2. Expertimental approach - System.out.println(FThreads.debugGetCurrThreadId());

How to figure out which thread should execute some code - See what it does.
- UI elements and Swing components management must be done in EDT.
- Anything that changes game state must be executed in Game thread.


I see your idea... handleConvokes should also be performed outside of input class and in game thread. I was too busy with general refactoring to note this method.
I didn't understand now your second phrase about 'catch' because we've been... celebrated something today - will return to this clause tomorrow.

You may safely execute them after setInputAndWait() for mana payment input has returned - that will be the game thread which requested mana payment.

[moomarc]

Thanks for the help Max. That thread output makes life a whole lot easier for me.

With regards to the 'catch', it was a poor choice of words seeing as catch has a specific java meaning, and aside from that I was speaking about my code in CostPartMana#payHuman override not CostPayMana. And what I meant is that I added a couple of lines there that firstly made the full cost payment cancel if the sacrifice was cancelled, and secondly a 'catchall' in the form of few more to finalise the offerings and convoke payments that skipped InputPayManaCostOfPayment (where the offending calls are made).

I'm busy making the changes now which should make things cleaner.

EDIT: Done for the human. Now just to get it working for AI again.

[moomarc]

I'm done with this round of changes (cleaned up AI r21867). Please test and let me know if there's any issues. Also let me know whether the patrons should be enabled for the AI. I think the patrons' abilities and strength are generally better than the potential sacrifices, especially in the case of goblins vs Patron of the Akki.

[Max mtg]

Looks good from my point