thread-driven changes of input

[Max mtg]

Ok, it's generally working, which tests should I perform to test possible weak spots of the input system?

[friarsol]

Ok, it's generally working, which tests should I perform to test possible weak spots of the input system?

Here's a brief list:

- Unless costs
- Propaganda costs
- Paying a trigger cost (especially ones where you are auto-skipping that phase)
- Selecting a target for triggers (especially ones where you are auto-skipping that phase)
- Simultaneous triggers hitting the stack one with a target the rest without. (When selecting the order of the triggers make sure the one with a target isn't last, and confirm the order matches what you select afterwards) [Note: This currently doesn't work correctly, the targetted trigger ends up being delayed to last]

[Max mtg]

Thanks, i've tested the first two - Mana Leak and Ghostly Prison controlled by ai work as expected.
Will you please hint me any cards or combinations with that triggers?

Dragon's Claw works, AEther Barrier works
AEther Barrier + Wooden Sphere allow me to order effects properly (when I cast a Brushstrider)

what else should I test?

[friarsol]

Sure thing, Use this gamestate file:

gamestate.txt | Open

humancardsinplay=Bottomless Pit;Bottomless Pit;Nath of the Gilt-Leaf

Load the file in, and during your upkeep when prompted to order the triggers, set the Nath trigger in between the two Bottomless Pits.

[Max mtg]

Sol, your sample works!
The first pit's trigger goes to stack, then I am asked for target of Nath's ability, and later the last pit's trigger gets added to the top of stack.

Have also tried with 5 triggers for upkeep. My branch really fixes the issue you've described (that triggers requiring a custom interactive input are put into stack after the ones that use dialog windows only)

So I suggest everyone who is interested tests the branch (or gives me ideas how to make it crash or behave incorrectly) and it will be reintegrated right on or after 26th (that is as soon as myk deletes old folders from res files)

[Max mtg]

Taking into consideration lack of test and low trunk acvtivity (than means there is a smaller chance to face conflicts reintegrating the branch and no reason to hurry) I'll make more improvements in the branch (convert more inputs to work in synchronous mode) and delay the merge until after the beta.

[myk]

My first impression is that it is still a little jerky and sluggish. The UI has a lot of quickly flashing state changes that are distracting, such as the message prompt window visibly changing text multiple times when I tap mana to cast a spell and the phase highlights flashing on and off as they are skipped instead of just having the ultimately active phase toggled as it did before.

Would it be possible to smooth out the UI presentation? All the flashing makes me feel tense.

On the plus side, I haven't run into any hangs yet. @other devs who were interested in the non-blocking input: do the changes here meet your expectations/needs?

edit: also, the diff is over 10,000 lines long. Where should we look for core functional changes?

[friarsol]

On the plus side, I haven't run into any hangs yet. @other devs who were interested in the non-blocking input: do the changes here meet your expectations/needs?

edit: also, the diff is over 10,000 lines long. Where should we look for core functional changes?

I'm interested, but not everyone has as much free time as Max does. I probably won't be able to look at it till at best tomorrow, due to family obligations.

[Max mtg]

I can keep the old message when input gets locked (instead of showing message 'waiting for actions')
Will look smoother, but inaccurate. It may display the old input's message while input is already locked and cannot be used as the on-screen message reads.

> over 9000 lines long
Yes, of course, because the changes are related to spell ability casting, costs payment and all related input.
Look for usages of FThreads members in code - that will be the places where functional changes ocurred.

Deck I use to test some special cases like flashback or madness | Open

2005 Ext UG Madness
4 x Aquamoeba
4 x Arrogant Wurm
4 x Basking Rootwalla
4 x Careful Study
4 x Chrome Mox
4 x Circular Logic
3 x Daze
3 x Deep Analysis
6 x Forest
1 x Genesis
1 x Intuition
8 x Island
1 x Merfolk Looter
2 x Roar of the Wurm
1 x Thought Courier
4 x Wild Mongrel
2 x Wonder
4 x Yavimaya Coast

[myk]

I can keep the old message when input gets locked (instead of showing message 'waiting for actions')
Will look smoother, but inaccurate. It may display the old input's message while input is already locked and cannot be used as the on-screen message reads.

how about if "waiting for actions" is shown only if we're still waiting after a small delay, say 0.5 seconds? that way we can avoid the jitter but still have the message appear when necessary. A similar technique might smooth out the phase highlights.

The cursor hover is no longer re-evaluated after a tap, so multiple lands can no longer be tapped without first jiggling the cursor. The current trunk behavior is to allow a second land to be tapped if it becomes hovered after the land on top of it was tapped.

When multiple counters are added at once, such as with Gavonry Township, they do not appear all at once, but rather are added in one by one, causing visual jitter.

The assign damage screen no longer shows pictures:

Edit: The WinLose screen fails to gain input focus at end of match.

[Max mtg]

I don't know how to make that kind of delay, I've supressed the message, hope that's enough.

I agree, battlefield is now updated after mana ability was played.

That was not changed. Previously EDT was completelly busy resolving 'add counter' effect of an ability for every valid card, so that despite updateObservers() was called N times, EDT could not immediatelly redraw the battlefield, it managed to draw it only after the last counter was added. Now, when a different thread adds counters, EDT is free and is able to redraw every time battlefield.updateObservers() is called from inside Card.addCounter().

Pictures handling was not changed in this branch. Although it works in trunk - will have a look.

[myk]

I used a synchronization pattern in class _OperationLogAsyncUpdater in DialogMigrateProfile.java that would be applicable here. It would allow you to set a pending operation (setting the message text), but only execute it if stop() was not called while a delay timer was still counting down. You'd have to add in the timer part, but the basic structure is already there.

for the counters and other similar effects that are now "staggered", maybe set a flag to indicate multiple changes are in progress, and only allow paint operations to proceed when the flag is off again? That sounds like a service Swing might already provide.

[Chris H.]

As the release dev I like to keep my local copy aimed at the trunk. Once we get past this early stage testing and it is merged into the trunk I will give it some testing and will report back at that time.

[Max mtg]

Won't work - I have a single instance of LockUI, also don't like old-fashioned synchronization.

I don't want to improve UX right now, because the primary thing to test were crashes/exceptions when playing spells with exotic costs or triggers on phase/zone changes.


It's PlayArea.setupPlayZone() method that gets called too many times.
So which method is supposed to reset the flag you mention?
The what needs to be done: accept the first request to redraw, wait some 20ms for others (if any) to come and then have EDT redraw the field. Risks: get ConcurrentModificationException, as other thread may change the list of cards on battlefield

Pictures in VAssignDamage - resolved by moving call to EDT. Must have been broken because of forge/view/arcane/CardPanel.java:647 that probably evaluated to false when run from other thread than edt. (it didn't reach ImageCache.getImage)

[myk]

@Chris: it's pretty easy to have multiple copies checked out without them interfering with one another. You don't need to switch a checkout between branches if you don't want to -- you can just check out a branch into its own directory to test it. You don't even need to load it into eclipse (which can get confusing if you have multiple branches loaded). For example:

Code: Select all

mkdir branches && cd branches
svn co http://svn.slightlymagic.net/forge/branches/input_sync && cd input_sync
mvn package && java -Xmx1024m -jar target/forge-*-dependencies.jar


Won't work - I have a single instance of LockUI, also don't like old-fashioned synchronization.

well, whatever solution you come up with, just make sure it's solid.

I don't want to work on UX right now, because the primary thing to test were crashes/exceptions when playing spells with exotic costs or triggers on phase/zone changes.

It is fine to focus on other bugs for now, but the branch should not be merged until UX (user experience, for those not familiar with the terminology) issues are resolved. This change has a heavy impact on UX, and these are important UX regressions to resolve before the code is merged.

Pictures in VAssignDamage - resolved by moving call to EDT. Must have been broken because of forge/view/arcane/CardPanel.java:647 that probably evaluated to false when run from other thread than edt. (it didn't reach ImageCache.getImage)

This points to crossover between classes that operate with threads and classes that interact with GUI widgets, which is worrying. Do we have architecture in place to enforce the requirement that no GUI widgets are interacted with in any way from non-EDT threads? Swing is not thread safe (except for a very few specific method calls), and we need to be very careful about keeping threads away from any swing-related classes.