Forge version 1.5.7

[drdev]

Max got me thinking, as I like his idea of highlighting all available targets, and I agree that having a button that automatically chooses a target for you takes away from the strategy of the game. I still believe having an Auto button for mana payment is a good idea, however, as how you tap your lands and empty your mana pool is rarely of consequence strategically, is usually fairly obvious, and involves a lot of clicking and mouse movement to do manually, which takes time away from playing the game.

I think what I might do, instead of doing an Auto button for targeting, is make it so everything but available targets are grayed out and unclickable, then have a visible indication of a "selected" target and use some very basic logic to determine which target to select by default without making any non-obvious choices for you:

• Positive effect: first valid target on your side of the field
• Negative effect: first valid target on opponent's side of the field

You could then use arrow keys or Tab/Shift+Tab to change the selected target using the keyboard if needed. Once the target you want is selected, you could then click the enabled OK button (or press Enter/Space) to confirm the currently selected target. This way, you could still use the keyboard to select targets without needing the mouse at all, but it wouldn't be making the choice for you.

The graying out of non-valid targets would also aid in using the mouse to click on targets, and of course if you click on a card it would both select and confirm the target as it does now.

The other nice thing about this approach is, in the case of something like the Hymm to Tourach example from my other post, your opponent's avatar would still be selected by default since it's a negative effect, and you could still just press Enter/Space to confirm without even needing to use the arrow keys or mouse.

Hows does that sound?

[friarsol]

You could then use arrow keys or Tab/Shift+Tab to change the selected target using the keyboard if needed. Once the target you want is selected, you could then click the enabled OK button (or press Enter/Space) to confirm the currently selected target. This way, you could still use the keyboard to select targets without needing the mouse at all, but it wouldn't be making the choice for you.

1) How would you deselect with just the keyboard? Since it seems like the OK button would be semi-overridden in some cases.
2) How would you handle Up to Targeting? If I can target 1-3 creatures, and I only target two. How does that schema work?

[drdev]

I just had another idea for improving the usability of mana payment and tapping lands, in addition to the Auto button. I can make it so, if you hold the Ctrl key and click on a land, it will automatically tap all lands in the same stack as that land. If you're in the middle of paying for a spell/ability, it would instead tap all lands in that stack until doing so would not reduce the cost any further. Similarly, if you Ctrl+click on a mana pool symbol while paying for a spell/ability, it would use mana of that color to pay off the cost until doing so would not reduce the cost any further.

I'll then add a message to the Prompt below the "Pay Mana Cost" to indicate the ability to do this as well as what the Auto button will do. I can show a different message if the cost isn't payable.

Between that support and the Auto button, I think people will be much happier with the usability of paying for spells/abilities. Let me know what you think.

Thanks.
-Dan

[Max mtg]

Max mtg, I'm sorry you doubt the appeal of auto mana payment, but I can tell you it's a very sought after feature by many, including myself, who would rather not have to manually click a card or mana pool icon once for every single mana in the cost of each spell/ability. If you don't want to use it, you don't have to.

I can look into Supplier<T>, but I created Evaluator<T> so I'd have a simple way to capture a return value from a Runnable in the many places that ask for a Runnable with Forge's codebase. I don't really see the harm in such a class existing.

As for getCostString(), I would argue the toString() ManaCost really should be of the form "{G}", since that's what it looks like in all Strings containing mana costs and other text, such as spell descriptions. toString() is also used more commonly that getCostString() right now, so for performance it makes sense that the extra replace operation should be on the less common case. Also, now that I see the purpose of getCostString() in the places now calling is to create a string that can be re-parsed into a new ManaCost or ManaCostBeingPaid object, it's worth noting that that didn't even actually work before in the cases of hybrid and phyrexian mana, since something like "3 GU" in the script would output as "3 G/U" from toString().

My current plan is to enhance the replace operation used by getCostString() to fix the bug with hybrid and phyrexian mana to ensure "G/U" is properly converted back to "GU" and "G/P" is properly converted back to "PG", but otherwise leave it as it stands.

Not a big problem about the presense of that auto payment feature, it was my personal opinion.

Many places, you say? Are you sure there are any in core or game modules? (the latter is stored in forge.game.* for now) I'm trying to prevent you from inventing you own infrastructure, while there exist libraries solving the same tasks, but widely used and thus thoroughly debugged and mainatined by most experienced developers.
We used to have our own functions and predicates in 2 years ago, that had no connection to guava. Soon when I needed more code to perform a wider set of operations on these, I realized that a library with same puposes already existed written by dedicated people at Google. It was a pity to have spent hours on development of something that was already existing in far better edition. Finally Forge uses guava's functions, not mine.

That's it. There's no problem about classes creation, that about avoiding some common mistakes and making code easier to understand for those who have already used those libs somewhere else.


getCostString() used to return space-delimited mana shards for a couple of years, and everyone was happy with that. You have changed that to save some work on presentation part, so now you are about to transform that product back and forth because that format does not suit all the needs the application has. That means you don't really know to which single format you want to convert the mana cost. You are trying to get a string out of manacost, then build a complex expression to transform it, and then you count ocurrencies or each color by counting characters. That's instead a of simple iteration over the shards present in that cost. You want to transform "G/U" to "UG" with literals replacement instead of calling getImageKey in ManaCostShard instances present in that cost. And... you never asked for advice on how to perform ceratin operation on core classes.
If noone is to stop you, where will such approach lead Forge? I mean introducing duplicate hardcoded values for mana parts, coupling card rules with the file they are stored in, static methods here and there... Probably back to its dark times, when cards were hardcoded, there was no chance to see more that two players in a match, not to speak of some player controlling another's actions? Just get check some older sources out to see where we came from, http://svn.slightlymagic.net/websvn/lis ... &peg=12868 .

The next destination where I want this project to get is multiplayer with remote players, with dedicated servers and AI acting as an independent client that connects to server. Meanwhile, any changes that add hardcodes and doubltful dependencies between entities (like methods in player controller) move the game further from that possible future.

[drdev]

You could then use arrow keys or Tab/Shift+Tab to change the selected target using the keyboard if needed. Once the target you want is selected, you could then click the enabled OK button (or press Enter/Space) to confirm the currently selected target. This way, you could still use the keyboard to select targets without needing the mouse at all, but it wouldn't be making the choice for you.

1) How would you deselect with just the keyboard? Since it seems like the OK button would be semi-overridden in some cases.
2) How would you handle Up to Targeting? If I can target 1-3 creatures, and I only target two. How does that schema work?

In the case of single target selection, you don't need to deselect ever, so you can just click Cancel or press Escape in that case.

For "Up to Targeting", I would not default select anything, but I would put a focus outline around the first valid target, have the arrow keys move that focus outline around, and use some unused key (Ctrl maybe?) to toggle the selected state of that target.

With that in mind, it's worth noting I can also apply this same functionality to declaring attackers and blockers, which could improve the usability of that as well.

Heck, I could even apply that functionality to the hand, battlefield, and other zones when the stack is empty to simulate clicking to cast spells and activate abilities. I could then make the Tab key move between the different zones, as well as assign each zone a shortcut to jump straight into that zone and focus its first card.

Suddenly the power user's dream of playing a game on Forge without using the mouse at all just became viable.

[drdev]

Not a big problem about the presense of that auto payment feature, it was my personal opinion.

Many places, you say? Are you sure there are any in core or game modules? (the latter is stored in forge.game.* for now) I'm trying to prevent you from inventing you own infrastructure, while there exist libraries solving the same tasks, but widely used and thus thoroughly debugged and mainatined by most experienced developers.
We used to have our own functions and predicates in 2 years ago, that had no connection to guava. Soon when I needed more code to perform a wider set of operations on these, I realized that a library with same puposes already existed written by dedicated people at Google. It was a pity to have spent hours on development of something that was already existing in far better edition. Finally Forge uses guava's functions, not mine.

That's it. There's no problem about classes creation, that about avoiding some common mistakes and making code easier to understand for those who have already used those libs somewhere else.


getCostString() used to return space-delimited mana shards for a couple of years, and everyone was happy with that. You have changed that to save some work on presentation part, so now you are about to transform that product back and forth because that format does not suit all the needs the application has. That means you don't really know to which single format you want to convert the mana cost. You are trying to get a string out of manacost, then build a complex expression to transform it, and then you count ocurrencies or each color by counting characters. That's instead a of simple iteration over the shards present in that cost. You want to transform "G/U" to "UG" with literals replacement instead of calling getImageKey in ManaCostShard instances present in that cost. And... you never asked for advice on how to perform ceratin operation on core classes.
If noone is to stop you, where will such approach lead Forge? I mean introducing duplicate hardcoded values for mana parts, coupling card rules with the file they are stored in, static methods here and there... Probably back to its dark times, when cards were hardcoded, there was no chance to see more that two players in a match, not to speak of some player controlling another's actions? Just get check some older sources out to see where we came from, http://svn.slightlymagic.net/websvn/lis ... &peg=12868 .

The next destination where I want this project to get is multiplayer with remote players, with dedicated servers and AI acting as an independent client that connects to server. Meanwhile, any changes that add hardcodes and doubltful dependencies between entities (like methods in player controller) move the game further from that possible future.

Ok, timeout. How have I introduced duplicate hardcoded values for mana parts? Forge was already using the "{G}" type mana formatting in Oracle text when I got here. All I did was decide it made sense to use that formatting for String representations of mana costs since it reads nicer when present inside other text by keeping it all together without each character being separated by a space and looking like its own word, not to mention being easier to pick out which parts are text and which parts are mana shards for the sake of the HTML formatting logic that lives elsewhere.

If anything, I would argue that, once a space delimited mana cost has been parsed into a mana cost from the script, you should not need to recreate that space delimited string again. So the places that are now calling getCostString() to extract and then re-parse a space delimited cost string should instead just use a constructor that creates a new ManaCost from a ManaCostBeingPaid or vice versa, or a copy constructor to create a new ManaCost from another ManaCost or a ManaCostBeingPaid from another ManaCostBeingPaid. As someone who cares a lot about coding style and structure, I would expect you to be with me on that.

I'm fine with people on this forum providing suggestions on better ways to do things, as I know I'm still relatively new to this project, and particularly to some of these core classes I'm now starting to work with. That's part of why I'm a regular poster on these forums when I work on these projects, usually looking for feedback. The last thing I want is to muddy the code base and take Forge back to the dark ages, or delay network play which I'm personally quite excited for. But that doesn't mean I won't challenge "the way things are" if I feel there's a better way, particularly if it allows desirable features and enhancements to be designed and implemented.

[Max mtg]

I judge from what I saw this far:
first you added braces into ctor of ManaCostShard, then strip these away in forge.card.mana.ManaCostParser.stripFormatting(String).
With that approach it's worth expecting that you'd just as easily add a method like manacoststring.replace("G/U", "UG").replace("G/W", "WG")... and so on.

At best you should have created a method somewhere near its consumers to return braces-enclosed costs (for the pupose of presentation costs in gui app) and leave original toString of manacost and shards untouched. But the code is already changed, let's see how it's possible to live with that.

ManaCost cannot declare a constructor with parameter of type ManaCostBeingPaid, because they belong to different layers. Those classes in core module must not refer to anything used to play games. That core module is the minimum set of classes needed to build a deckeditor or manage card collections (like trade cards with other players). That's why such ctor is impossible.

In fact the method you were asking about is forge.game.mana.ManaCostBeingPaid.toManaCost(), added 7 months ago.

ManaCosts are immutable, that's why you don't need a copy constrcutor for those. And there is already a copy constructor for costs being paid. Feel free to use them and please remove that second toCostString method if those constructors suffice your needs.


There are a few concepts and ideas, that should be followed to keep the code well-structured, despite of enticement to add some direct references from human player controller to AI one, or from general player to AI playercontroller.
About enticements: there used to be concrete implementations for AI and Human Player classes (not controllers) and it was impossible to give control over players (Mindslaver and Sorin Markov were frequently requested but impossible to implement). And I don't want to fix that or similar issues again. That's why I am so stressed with special methods in general pupose classes.

As for concepts and ideas: just keep in mind the aim to build a competitor for Magic online (in terms of features). An app that allows people to log in and find their own cards, bulid decks, share them, communicate via chat, then play games, tournaments against each other or AI opponents (that can also be connected via network, not just built into server) and have some sort of single player campaigh (that's current quest mode)
Now we are drilling deeper into code. Which classes are needed for each part of server, and which should exist only on client? Obviously, no part of GUI may be used by server code, That means any calls to swing.* are banned in forge.game.* code that handles rules enforcement. It seems natural to maintain the base PlayerController on server as an interface to ask remote clients about their next action. Does the Player class belong to server or client? If it belongs to server, can it refer to any special playercontrollers? (While it's known that human controller is directly calling gui methods and can by no means stay on server, and AI is known to cheat sometimes - by looking into player's hand)

[drdev]

Alright, I've now added 3 more game play usability improvements:

• You can now press Ctrl+Z to undo tapping mana sources.
• You can now press Escape to Cancel the current prompt even if the OK button has focus.
• Cards will no longer disappear from your hand while paying their mana cost or choosing targets.

Note that undo currently only works for tapping mana sources before clicking a spell/ability to activate, though I plan to fix it to support paying off an active cost soon. You can still click Cancel while paying a cost to undo all mana sources tapped for that cost, just as before, so Ctrl+Z would just add a way to undo only the most recent mana source used in that case.

Ideally, I'm also hoping to support undoing the top spell/ability on the stack if it was just put there and no other spells/abilities have been played on top of it. In this case, the Cancel button on the Prompt would be enabled with the caption "Undo", so either Escape or Ctrl+Z would work for that. I'm guessing this will be tricky, but if it's possible it would be a great mis-click saver. Anybody have any suggestions for what I'd need to do to make such support work correctly?

Thanks.
-Dan

[moomarc]

[*]Cards will no longer disappear from your hand while paying their mana cost or choosing targets.

Thanks for the implementation Dan, I've often had to cancel a spell just to reread the card text. There is an issue however. Currently if you play Abandon Hope you can discard Abandon hope as part of the additional cost.

I think there's an easy fix though. You should just be able to mark the source card with Card.setUsedToPay(true) then set it back to false when it actually moves to the stack. That should disallow selecting the card for such payments.

[friarsol]

[*]Cards will no longer disappear from your hand while paying their mana cost or choosing targets.

Thanks for the implementation Dan, I've often had to cancel a spell just to reread the card text. There is an issue however. Currently if you play Abandon Hope you can discard Abandon hope as part of the additional cost.

Yea this is definitely wrong. The card goes onto the Stack as soon as you start casting it.

http://www.gatheringmagic.com/do-you-re ... st-spells/

[drdev]

In that case, can I just mark the card as being on the stack without actually removing it from the hand area? Otherwise, would there be any problem with implementing moomarc's suggested fix? Regardless of where the card should reside rules wise while paying costs and choosing targets, the card should continue to display where it was activated from for the sake of usability.

[friarsol]

In that case, can I just mark the card as being on the stack without actually removing it from the hand area? Otherwise, would there be any problem with implementing moomarc's suggested fix. Regardless of where the card should display rules wise while paying costs and choosing targets, the card should continue to display where it was for usability.

It really shouldn't be in more than one zone at the same time. I'm not sure if moomarc's suggestion would prevent all issues with targeting/paying costs. If we switch to the stack view, does it show up on the stack? If so, can't you see the card from there? If not, that's where it should be getting resolved. Make sure it shows up on the stack view, say something like (Casting) if it's still in the process of being cast.

[moomarc]

In that case, can I just mark the card as being on the stack without actually removing it from the hand area? Otherwise, would there be any problem with implementing moomarc's suggested fix. Regardless of where the card should display rules wise while paying costs and choosing targets, the card should continue to display where it was for usability.

It really shouldn't be in more than one zone at the same time. I'm not sure if moomarc's suggestion would prevent all issues with targeting/paying costs. If we switch to the stack view, does it show up on the stack? If so, can't you see the card from there? If not, that's where it should be getting resolved. Make sure it shows up on the stack view, say something like (Casting) if it's still in the process of being cast.

isUsedToPay should work for all situations that can arise while paying costs as a card marked as such can't be targeted at all, although if theres a cost that requires exiling or discarding your entire hand it would be an issue. Also, if it is physically in both zones, what is the effect on triggers and replacement effects when it checks zoneFrom? I think Sol is correct in saying we should rather handle the display in the stack.

How is stack view going, by the way? I played a few games with it enabled but found it quite difficult when you don't know which ability on a card is being played. Will there be some sort of hybrid view that has the card display with textual stack below it? The only other issue I had with it is that it's not yet hooked up to right clicking optional triggers to set Always, never or ask to run the triggers. I assume that was one of the things that made you hold back the feature in the previous beta.

[friarsol]

isUsedToPay should work for all situations that can arise while paying costs as a card marked as such can't be targeted at all, although if theres a cost that requires exiling or discarding your entire hand it would be an issue. Also, if it is physically in both zones, what is the effect on triggers and replacement effects when it checks zoneFrom? I think Sol is correct in saying we should rather handle the display in the stack.

Yea these were some of the issues I was worried about. Possibly even being able to cast an Elvish Spirit Guide by paying mana through exiling that same Guide.

[Marek14]

There's also the Guul Draz Specter/Reprisal corner case. (Simply said, if Reprisal is last card in your hand, it can target Guul Draz Specter because as soon as you start to cast it, it moves to stack and Gul Draaz Specter gets bonus for your empty hand, and this all happens before you select the targets.)