Chosen letter support (Unsets) #6636
Conversation
Northmoc
added
Game Mechanics
Low priority
xD |
| List<String> letters = Lists.newArrayList("A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", | ||
| "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"); | ||
| List<String> consonants = Lists.newArrayList("B", "C", "D", "F", "G", "H", "J", "K", "L", "M", "N", "P", "Q", | ||
| "R", "S", "T", "V", "W", "X", "Y", "Z"); | ||
| List<String> choices = sa.hasParam("Consonant") ? consonants : letters; |
There was a problem hiding this comment.
Imo, I would have used Constants for this? And only make a new List if "Exclude" is used?
Also need extra list for Vowels
There was a problem hiding this comment.
Sounds good – I am not sure how to set that up so it would be good to learn
There was a problem hiding this comment.
Like this?
List<String> LETTERS = Lists.newArrayList();
//... inside function:
List<String> choices = LETTERS;
if (sa.hasParam("Exclude")) {
choices = Lists.newArrayList(choices); // makes copy
choices.removeAll(Arrays.asList(sa.getParam("Exclude").split(",")));
}
hm the chooseLetter implementations does remove the letter anyway from the list, so you can't give them the constant ? :(
| List<String> chosen = Lists.newArrayList(); | ||
| for (int i = 0; i <=n; i++) { | ||
| String choice = Aggregates.random(letters); | ||
| letters.remove(choice); |
There was a problem hiding this comment.
As Hanmac mentioned, it's probably safest to avoid transforming the input array in these methods. In this case you could just use the second parameter of Aggregates.random and set it to n.
| @@ -193,6 +193,8 @@ public final void reveal(List<CardView> cards, ZoneType zone, PlayerView owner, | |||
| public abstract PlayerZone chooseStartingHand(List<PlayerZone> zones); | |||
| public abstract Mana chooseManaFromPool(List<Mana> manaChoices); | |||
|
|
|||
| public abstract List<String> chooseLetter(int n, String ai, List<String> letters); | |||
There was a problem hiding this comment.
Maybe pass in the whole SpellAbility rather than just the AI parameter? Not sure that it matters here but most other methods in this class do it that way.
| Collections.sort(letters); | ||
| chosenLetters = letters; | ||
| view.updateChosenLetters(this); | ||
| } |
There was a problem hiding this comment.
Don't forget to transfer the chosen letters in CardCopyService, and similar places. Maybe do a few usage searches on similar methods.
It might be worth it someday to split Card's chosen_____ fields off into their own class. They each add a lot of similar boilerplate to Cards and related functions.
There was a problem hiding this comment.
It might be worth it someday to split
Card'schosen_____fields off into their own class. They each add a lot of similar boilerplate to Cards and related functions.
that is my department, i need to refactor that anyway for Linked Abilities stuff, where you could have different sets of Chosen Values.
| } else if (property.startsWith("ChosenLetter")) { | ||
| final List<String> letters = source.getChosenLetters(); | ||
| if (letters == null) return false; // some things check before letters have been chosen | ||
| String name = cardState.getName(); |
There was a problem hiding this comment.
Maybe apply .toUpperCase to name here so you don't have to check it twice below? Also in case we ever have any cards that start with a lowercase letter. Can't think of any right now besides "capital offense".
|
Apropos Letter in Card Name, we should normalize them for names like see: forge/forge-core/src/main/java/forge/util/TextUtil.java Lines 49 to 54 in 1bee1a4 |
A decent draft, I think