thread-driven changes of input

[friarsol]

AI casts Slow Motion on my creature. During my upkeep, the enchantment triggers and I'm prompted to pay 2 mana. Clicking on my lands does nothing, so the only way I can continue is by canceling the unless payment and losing my creature.

Max, why did you rush to merge? I said I was going to test things tonight, and I've already found two serious issues. I would have much preferred to do that on the branch.

[friarsol]

AI has Collective Restraint and 5 basics. I attack with a creature and am unable to tap my land to pay the required 5 mana and have to cancel the input. Did you ever try paying Propaganda costs like I suggested earlier in this thread?

[Max mtg]

Crash for Winter Orb. I click my land, it untaps, my message window still says Waiting for Actions. I click on my other untapped land (cause yknow, what is it waiting for?) and then I got the following crash.

java.lang.RuntimeException: Trying to unlock input which is not locked! Do check when your threads terminate!
at forge.control.input.InputControl.unlock(InputControl.java:199)
at forge.FThreads$1.run(FThreads.java:102)
at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)

Yep, it is hardcoded and (that is why it used old input)
Have commited refactored Untap class adapted for new environment

[Max mtg]

AI casts Slow Motion on my creature. During my upkeep, the enchantment triggers and I'm prompted to pay 2 mana. Clicking on my lands does nothing, so the only way I can continue is by canceling the unless payment and losing my creature.

Max, why did you rush to merge? I said I was going to test things tonight, and I've already found two serious issues. I would have much preferred to do that on the branch.

I don't like to be isolated in a branch.
I have a lot of ideas and know how to implement them and I have time to do it.
Do I have to delay them all until some other people have free time to test a branch?

Propagandas work again. There was an issue related to InputPayManaWithCommands futher refactoring

[Max mtg]

What you are doing looks clever, but clever people have tried tricks like this before. Since I can't seem to convince you, perhaps this article can.

Myk, I don't want to access GUI from several threads. When it comes to reading player's input during a match, only EDT (which I invoke by SwingUtilities.invokeLater) works with UI. The pooled threads deal with game state and don't interact with interfaces. The same threads will drive the game state changes when its ready to work as a server without any GUI at all. So the problem discussed in the article does not apply to Forge.

This change is about isolation - let UI be handled by one set of threads (EDT in our case) and game by other ones (cached pool in FThreads), so that UI remains responsive. It's a common approach, I don't understand why you would like EDT to handle everything.

The event driven model is the one to use here. Not threads. This is the type of problem that gets reliably resolved by registering a Runnable callback with an input request and enqueueing it on the event loop. For example:

Code: Select all

final choices = ...;
SwingUtilities.invokeLater(new Runnable() {
  @Override public void run() {
    choice = getChoice(choices);
    doNextStep(choice);
  }});


Easy to write, easy to maintain, and easy to understand. No deadlocks or memory consistency issues. Guaranteed.

Look, you treat getChoice as if it were synchronous. But in Forge rev 20624 and below you had to return soon after you set a new Input. You don't know when you recieve the result, but you know where - the Input class will collect the answer from its field when user is done. That were input classes that performed actions on recieved data in many cases and yet had to invoke the next step. - How do you like this kind of roles distribution?

I've had enough of that, and made what is right.

Now I just set up a new input and have a background thread wait for reply, and use a single Input class to choose cards for any purpose. It's that easy.

[moomarc]

Finally had a chance to try it out for the first time. Load time was lightning fast (first run stats: Read cards: 12434 files in 458 ms (25 parts) using thread pool).

Came across my first related crash though after my second turn. I needed to discard down to 7 cards (should have mulliganed afterall) and got this crash when I clicked on the card to discard:

IllegalStateException | Open

Code: Select all

Forge Version:    SVN
Operating System: Windows 7 6.1 x86
Java Version:     1.6.0_35 Sun Microsystems Inc.

java.lang.IllegalStateException: Player.doDiscard may not be accessed from the event dispatch thread.
   at forge.FThreads.checkEDT(FThreads.java:49)
   at forge.game.player.Player.doDiscard(Player.java:1628)
   at forge.game.player.Player.discard(Player.java:1613)
   at forge.control.input.InputCleanup.selectCard(InputCleanup.java:79)
   at forge.gui.InputProxy.selectCard(InputProxy.java:127)
   at forge.gui.match.nonsingleton.CHand.cardclickAction(CHand.java:186)
   at forge.gui.match.nonsingleton.CHand.access$0(CHand.java:180)
   at forge.gui.match.nonsingleton.CHand$1.mousePressed(CHand.java:59)
   at java.awt.AWTEventMulticaster.mousePressed(Unknown Source)
   at java.awt.Component.processMouseEvent(Unknown Source)
   at javax.swing.JComponent.processMouseEvent(Unknown Source)
   at java.awt.Component.processEvent(Unknown Source)
   at java.awt.Container.processEvent(Unknown Source)
   at java.awt.Component.dispatchEventImpl(Unknown Source)
   at java.awt.Container.dispatchEventImpl(Unknown Source)
   at java.awt.Component.dispatchEvent(Unknown Source)
   at java.awt.LightweightDispatcher.retargetMouseEvent(Unknown Source)
   at java.awt.LightweightDispatcher.processMouseEvent(Unknown Source)
   at java.awt.LightweightDispatcher.dispatchEvent(Unknown Source)
   at java.awt.Container.dispatchEventImpl(Unknown Source)
   at java.awt.Window.dispatchEventImpl(Unknown Source)
   at java.awt.Component.dispatchEvent(Unknown Source)
   at java.awt.EventQueue.dispatchEventImpl(Unknown Source)
   at java.awt.EventQueue.access$400(Unknown Source)
   at java.awt.EventQueue$2.run(Unknown Source)
   at java.awt.EventQueue$2.run(Unknown Source)
   at java.security.AccessController.doPrivileged(Native Method)
   at java.security.AccessControlContext$1.doIntersectionPrivilege(Unknown Source)
   at java.security.AccessControlContext$1.doIntersectionPrivilege(Unknown Source)
   at java.awt.EventQueue$3.run(Unknown Source)
   at java.awt.EventQueue$3.run(Unknown Source)
   at java.security.AccessController.doPrivileged(Native Method)
   at java.security.AccessControlContext$1.doIntersectionPrivilege(Unknown Source)
   at java.awt.EventQueue.dispatchEvent(Unknown Source)
   at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
   at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
   at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
   at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
   at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
   at java.awt.EventDispatchThread.run(Unknown Source)


[Max mtg]

Thanks Marc, fixed in r20640.

Known potential bugs:
http://magiccards.info/query?q=mana%3E% ... rd&s=cname - cards/abilities with XX in cost
http://magiccards.info/query?q=o%3A%22s ... rd&s=cname - cards with color restriction on X

[Chris H.]

I played several games using Gos' decks and the AI used randomly constructed 2 color decks.

My initial tests ran OK. No jitters noticed and no crashes.

Gos' decks tend to use complicated cards and I played with some of the inputs. Cards that generate more than one color of mana, Kicker and the dialog that lets us order our abs and things worked OK.

There may be some corner case type inputs that do not come up very often so I do expect that some of us will find a few bugs that need fixing.

[friarsol]

Known potential bugs:
http://magiccards.info/query?q=mana%3E% ... rd&s=cname - cards/abilities with XX in cost

Probably a best way to resolve this is to add an Announce$ X parameter in SAs with XX in the cost, instead of trying to fix the input.

[Diogenes]

[edit:] Nevermind, Moomarc posted it. Sorry.

[Max mtg]

Sol, why don't we switch all X costs to announce? (so that the parameter Announce$ becomes not needed)

[friarsol]

Sol, why don't we switch all X costs to announce? (so that the parameter Announce$ becomes not needed)

We could try that, although I'd be worried about the Usability of some of the X costs which are derived from other things not working as seemlessly (which is why I didn't add it as an overarching thing to begin with).

Two quick examples:
Gridlock is a derived X. You choose a certain amount of nonland permanents to target and then X is derived. Simple.
Detonate is a derived X. You choose which artifact you want to target and then X is derived. Simple.

If you need to Announce X for these, then now you need to precalculate the value of X, type it in, then select your target. If you miscalculate or mistype, then all of a sudden you can't target as much or exactly what you want to.


Now that Announce is working well with a bunch of cards, we may be able switch all "SVar:X:Count$xPaid" X Costs over to Announce by default. That way we don't lose our seemless behavior above, but simplify a majority of the rest of X.

Edit: Changing an example so card highlighting works

[moomarc]

Sol, why don't we switch all X costs to announce? (so that the parameter Announce$ becomes not needed)

One reason would be that there is at least one card (Spoils of War) where X is calculated, not announced. So it would have to be implemented such that you're asked to announce X only if there is no value of X stipulated in the SVars or uses xPaid. That would allow us to slowly convert the remaining 200 (approx.) cards with X-costs at a leisurely pace.

Another possible reason (and Sol would have to confirm this) but I don't think the AI is set up to handle AnnounceX yet.

Gridlock and Detonate type cards should strictly be converted as well, but the game flow is just smoother the way it is at the moment, especially when there's hundreds of tokens that you might not want to count.

EDIT: I'll stop hijacking this thread now.

[friarsol]

Another possible reason (and Sol would have to confirm this) but I don't think the AI is set up to handle AnnounceX yet.

Actually I'm not sure about the AI handling X right now in general. But it's definitely something that needs to be considered before we just swap everything.

[Max mtg]

Oh, I didn't specify that I meant only cards with X present in mana cost.

Huh, read the posts, you gave examples of exactly such cards.